Merge branch '31268-manually-renaming-environments-breaks-future-deployments-2' into 'master'

Disallow editing environment name

See merge request gitlab-org/gitlab!68550

app/assets/javascripts/environments/components/edit_environment.vue

@@ -18,6 +18,7 @@ export default {
   data() {
     return {
       formEnvironment: {
+        id: this.environment.id,
         name: this.environment.name,
         externalUrl: this.environment.external_url,
       },
@@ -33,7 +34,6 @@ export default {
       axios
         .put(this.updateEnvironmentPath, {
           id: this.environment.id,
-          name: this.formEnvironment.name,
           external_url: this.formEnvironment.externalUrl,
         })
         .then(({ data: { path } }) => visitUrl(path))

app/assets/javascripts/environments/components/environment_form.vue

@@ -39,12 +39,17 @@ export default {
     ),
     nameLabel: __('Name'),
     nameFeedback: __('This field is required'),
+    nameDisabledHelp: __("You cannot rename an environment after it's created."),
+    nameDisabledLinkText: __('How do I rename an environment?'),
     urlLabel: __('External URL'),
     urlFeedback: __('The URL should start with http:// or https://'),
     save: __('Save'),
     cancel: __('Cancel'),
   },
   helpPagePath: helpPagePath('ci/environments/index.md'),
+  renamingDisabledHelpPagePath: helpPagePath('ci/environments/index.md', {
+    anchor: 'rename-an-environment',
+  }),
   data() {
     return {
       visited: {
@@ -54,6 +59,9 @@ export default {
     };
   },
   computed: {
+    isNameDisabled() {
+      return Boolean(this.environment.id);
+    },
     valid() {
       return {
         name: this.visited.name && this.environment.name !== '',
@@ -102,10 +110,17 @@ export default {
           :state="valid.name"
           :invalid-feedback="$options.i18n.nameFeedback"
         >
+          <template v-if="isNameDisabled" #description>
+            {{ $options.i18n.nameDisabledHelp }}
+            <gl-link :href="$options.renamingDisabledHelpPagePath" target="_blank">
+              {{ $options.i18n.nameDisabledLinkText }}
+            </gl-link>
+          </template>
           <gl-form-input
             id="environment_name"
             :value="environment.name"
             :state="valid.name"
+            :disabled="isNameDisabled"
             name="environment[name]"
             required
             @input="onChange({ ...environment, name: $event })"

app/controllers/projects/environments_controller.rb

@@ -213,8 +213,14 @@ class Projects::EnvironmentsController < Projects::ApplicationController
     end
   end
 
+  def allowed_environment_attributes
+    attributes = [:external_url]
+    attributes << :name if action_name == "create"
+    attributes
+  end
+
   def environment_params
-    params.require(:environment).permit(:name, :external_url)
+    params.require(:environment).permit(allowed_environment_attributes)
   end
 
   def environment

doc/api/environments.md

@@ -194,7 +194,7 @@ PUT /projects/:id/environments/:environments_id
 | --------------- | ------- | --------------------------------- | -------------------------------  |
 | `id`            | integer/string | yes                               | The ID or [URL-encoded path of the project](index.md#namespaced-path-encoding) owned by the authenticated user            |
 | `environment_id` | integer | yes | The ID of the environment  |
-| `name`          | string  | no                                | The new name of the environment  |
+| `name`          | string  | no                                | [Deprecated and will be removed in GitLab 15.0](https://gitlab.com/gitlab-org/gitlab/-/issues/338897) |
 | `external_url`  | string  | no                                | The new `external_url`           |
 
 ```shell

doc/ci/environments/index.md

@@ -728,6 +728,19 @@ like [Review Apps](../review_apps/index.md) (`review/*`).
 The most specific spec takes precedence over the other wildcard matching. In this case,
 the `review/feature-1` spec takes precedence over `review/*` and `*` specs.
 
+### Rename an environment
+
+> Renaming environments through the UI [was removed in GitLab 14.3](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68550). Renaming environments through the API was deprected and [will be removed in GitLab 15.0](https://gitlab.com/gitlab-org/gitlab/-/issues/338897).
+
+Renaming an environment through the UI is not possible.
+Instead, you need to delete the old environment and create a new one:
+
+1. On the top bar, select **Menu > Projects** and find your project.
+1. On the left sidebar, select **Deployments > Environments**.
+1. Find the environment and stop it.
+1. Delete the environment.
+1. Create a new environment with your preferred name.
+
 ## Related topics
 
 - [Use GitLab CI to deploy to multiple environments (blog post)](https://about.gitlab.com/blog/2021/02/05/ci-deployment-and-environments/)

lib/api/environments.rb

@@ -58,7 +58,8 @@ module API
       end
       params do
         requires :environment_id, type: Integer,  desc: 'The environment ID'
-        optional :name,           type: String,   desc: 'The new environment name'
+        # TODO: disallow renaming via the API https://gitlab.com/gitlab-org/gitlab/-/issues/338897
+        optional :name,           type: String,   desc: 'DEPRECATED: Renaming environment can lead to errors, this will be removed in 15.0'
         optional :external_url,   type: String,   desc: 'The new URL on which this deployment is viewable'
         optional :slug, absence: { message: "is automatically generated and cannot be changed" }
       end

locale/gitlab.pot

@@ -16635,6 +16635,9 @@ msgstr ""
 msgid "How do I mirror repositories?"
 msgstr ""
 
+msgid "How do I rename an environment?"
+msgstr ""
+
 msgid "How do I set up a Google Chat webhook?"
 msgstr ""
 
@@ -38260,6 +38263,9 @@ msgstr ""
 msgid "You cannot play this scheduled pipeline at the moment. Please wait a minute."
 msgstr ""
 
+msgid "You cannot rename an environment after it's created."
+msgstr ""
+
 msgid "You cannot write to a read-only secondary GitLab Geo instance. Please use %{link_to_primary_node} instead."
 msgstr ""
 

spec/controllers/projects/environments_controller_spec.rb

@@ -222,6 +222,16 @@ RSpec.describe Projects::EnvironmentsController do
         expect(response).to have_gitlab_http_status(:bad_request)
       end
     end
+
+    context 'when name is passed' do
+      let(:params) { environment_params.merge(environment: { name: "new name" }) }
+
+      it 'ignores name' do
+        expect do
+          subject
+        end.not_to change { environment.reload.name }
+      end
+    end
   end
 
   describe 'PATCH #stop' do

spec/frontend/environments/edit_environment_spec.js

@@ -15,15 +15,12 @@ const DEFAULT_OPTS = {
     projectEnvironmentsPath: '/projects/environments',
     updateEnvironmentPath: '/proejcts/environments/1',
   },
-  propsData: { environment: { name: 'foo', externalUrl: 'https://foo.example.com' } },
+  propsData: { environment: { id: '0', name: 'foo', external_url: 'https://foo.example.com' } },
 };
 
 describe('~/environments/components/edit.vue', () => {
   let wrapper;
   let mock;
-  let name;
-  let url;
-  let form;
 
   const createWrapper = (opts = {}) =>
     mountExtended(EditEnvironment, {
@@ -34,9 +31,6 @@ describe('~/environments/components/edit.vue', () => {
   beforeEach(() => {
     mock = new MockAdapter(axios);
     wrapper = createWrapper();
-    name = wrapper.findByLabelText('Name');
-    url = wrapper.findByLabelText('External URL');
-    form = wrapper.findByRole('form', { name: 'Edit environment' });
   });
 
   afterEach(() => {
@@ -44,19 +38,22 @@ describe('~/environments/components/edit.vue', () => {
     wrapper.destroy();
   });
 
+  const findNameInput = () => wrapper.findByLabelText('Name');
+  const findExternalUrlInput = () => wrapper.findByLabelText('External URL');
+  const findForm = () => wrapper.findByRole('form', { name: 'Edit environment' });
+
   const showsLoading = () => wrapper.find(GlLoadingIcon).exists();
 
   const submitForm = async (expected, response) => {
     mock
       .onPut(DEFAULT_OPTS.provide.updateEnvironmentPath, {
-        name: expected.name,
         external_url: expected.url,
+        id: '0',
       })
       .reply(...response);
-    await name.setValue(expected.name);
-    await url.setValue(expected.url);
+    await findExternalUrlInput().setValue(expected.url);
 
-    await form.trigger('submit');
+    await findForm().trigger('submit');
     await waitForPromises();
   };
 
@@ -65,18 +62,8 @@ describe('~/environments/components/edit.vue', () => {
     expect(header.exists()).toBe(true);
   });
 
-  it.each`
-    input         | value
-    ${() => name} | ${'test'}
-    ${() => url}  | ${'https://example.org'}
-  `('it changes the value of the input to $value', async ({ input, value }) => {
-    await input().setValue(value);
-
-    expect(input().element.value).toBe(value);
-  });
-
   it('shows loader after form is submitted', async () => {
-    const expected = { name: 'test', url: 'https://google.ca' };
+    const expected = { url: 'https://google.ca' };
 
     expect(showsLoading()).toBe(false);
 
@@ -86,7 +73,7 @@ describe('~/environments/components/edit.vue', () => {
   });
 
   it('submits the updated environment on submit', async () => {
-    const expected = { name: 'test', url: 'https://google.ca' };
+    const expected = { url: 'https://google.ca' };
 
     await submitForm(expected, [200, { path: '/test' }]);
 
@@ -94,11 +81,24 @@ describe('~/environments/components/edit.vue', () => {
   });
 
   it('shows errors on error', async () => {
-    const expected = { name: 'test', url: 'https://google.ca' };
+    const expected = { url: 'https://google.ca' };
 
-    await submitForm(expected, [400, { message: ['name taken'] }]);
+    await submitForm(expected, [400, { message: ['uh oh!'] }]);
 
-    expect(createFlash).toHaveBeenCalledWith({ message: 'name taken' });
+    expect(createFlash).toHaveBeenCalledWith({ message: 'uh oh!' });
     expect(showsLoading()).toBe(false);
   });
+
+  it('renders a disabled "Name" field', () => {
+    const nameInput = findNameInput();
+
+    expect(nameInput.attributes().disabled).toBe('disabled');
+    expect(nameInput.element.value).toBe('foo');
+  });
+
+  it('renders an "External URL" field', () => {
+    const urlInput = findExternalUrlInput();
+
+    expect(urlInput.element.value).toBe('https://foo.example.com');
+  });
 });

spec/frontend/environments/environment_form_spec.js

@@ -102,4 +102,52 @@ describe('~/environments/components/form.vue', () => {
     wrapper = createWrapper({ loading: true });
     expect(wrapper.findComponent(GlLoadingIcon).exists()).toBe(true);
   });
+  describe('when a new environment is being created', () => {
+    beforeEach(() => {
+      wrapper = createWrapper({
+        environment: {
+          name: '',
+          externalUrl: '',
+        },
+      });
+    });
+
+    it('renders an enabled "Name" field', () => {
+      const nameInput = wrapper.findByLabelText('Name');
+
+      expect(nameInput.attributes().disabled).toBeUndefined();
+      expect(nameInput.element.value).toBe('');
+    });
+
+    it('renders an "External URL" field', () => {
+      const urlInput = wrapper.findByLabelText('External URL');
+
+      expect(urlInput.element.value).toBe('');
+    });
+  });
+
+  describe('when an existing environment is being edited', () => {
+    beforeEach(() => {
+      wrapper = createWrapper({
+        environment: {
+          id: 1,
+          name: 'test',
+          externalUrl: 'https://example.com',
+        },
+      });
+    });
+
+    it('renders a disabled "Name" field', () => {
+      const nameInput = wrapper.findByLabelText('Name');
+
+      expect(nameInput.attributes().disabled).toBe('disabled');
+      expect(nameInput.element.value).toBe('test');
+    });
+
+    it('renders an "External URL" field', () => {
+      const urlInput = wrapper.findByLabelText('External URL');
+
+      expect(urlInput.element.value).toBe('https://example.com');
+    });
+  });
 });