From cad818ba957c169c982bb132b226f4e7fe49832d Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Thu, 19 Aug 2021 16:37:40 +0300 Subject: [PATCH 1/7] Disallow editing the environment name When environment gets updated, slug and tier aren't properly updated. And we actually can't update slug as it's used by deployment jobs(e.g. AutoDevOps) and should stay the same for the environment. https://gitlab.com/gitlab-org/gitlab/-/issues/31268 Changelog: fixed --- .../environments/components/edit_environment.vue | 2 +- .../environments/components/environment_form.vue | 1 + app/controllers/projects/environments_controller.rb | 8 +++++++- lib/api/environments.rb | 3 ++- .../projects/environments_controller_spec.rb | 10 ++++++++++ 5 files changed, 21 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/environments/components/edit_environment.vue b/app/assets/javascripts/environments/components/edit_environment.vue index 1cd960d7cd64ca..96742a11ebb454 100644 --- a/app/assets/javascripts/environments/components/edit_environment.vue +++ b/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)) diff --git a/app/assets/javascripts/environments/components/environment_form.vue b/app/assets/javascripts/environments/components/environment_form.vue index 6db8fe24e72653..b68378fa708749 100644 --- a/app/assets/javascripts/environments/components/environment_form.vue +++ b/app/assets/javascripts/environments/components/environment_form.vue @@ -106,6 +106,7 @@ export default { id="environment_name" :value="environment.name" :state="valid.name" + :disabled="environment.id" name="environment[name]" required @input="onChange({ ...environment, name: $event })" diff --git a/app/controllers/projects/environments_controller.rb b/app/controllers/projects/environments_controller.rb index cac0aa9d51377d..23dabd885c8b10 100644 --- a/app/controllers/projects/environments_controller.rb +++ b/app/controllers/projects/environments_controller.rb @@ -213,8 +213,14 @@ def expire_etag_cache 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 diff --git a/lib/api/environments.rb b/lib/api/environments.rb index e50da4264b5093..c032b80e39b7fa 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -58,7 +58,8 @@ class Environments < ::API::Base 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 diff --git a/spec/controllers/projects/environments_controller_spec.rb b/spec/controllers/projects/environments_controller_spec.rb index 7103d7df5c58dd..0fcdeb2edde282 100644 --- a/spec/controllers/projects/environments_controller_spec.rb +++ b/spec/controllers/projects/environments_controller_spec.rb @@ -222,6 +222,16 @@ 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 -- GitLab From 2a0501abe7121199bf69c679f8f60d648c0ad2f2 Mon Sep 17 00:00:00 2001 From: Nathan Friend Date: Thu, 19 Aug 2021 20:39:57 -0400 Subject: [PATCH 2/7] Fix edit_environment_spec.js and Vue warning This commit fixes a broken Jest test and also fixes a Vue console console warning about a prop type mismatch. --- .../components/environment_form.vue | 5 +- .../environments/edit_environment_spec.js | 92 +++++++++++++------ 2 files changed, 70 insertions(+), 27 deletions(-) diff --git a/app/assets/javascripts/environments/components/environment_form.vue b/app/assets/javascripts/environments/components/environment_form.vue index b68378fa708749..14f173f8834c58 100644 --- a/app/assets/javascripts/environments/components/environment_form.vue +++ b/app/assets/javascripts/environments/components/environment_form.vue @@ -54,6 +54,9 @@ export default { }; }, computed: { + isNameDisabled() { + return Boolean(this.environment.id); + }, valid() { return { name: this.visited.name && this.environment.name !== '', @@ -106,7 +109,7 @@ export default { id="environment_name" :value="environment.name" :state="valid.name" - :disabled="environment.id" + :disabled="isNameDisabled" name="environment[name]" required @input="onChange({ ...environment, name: $event })" diff --git a/spec/frontend/environments/edit_environment_spec.js b/spec/frontend/environments/edit_environment_spec.js index 3e7f5dd5ff449d..b51ad6de1c1129 100644 --- a/spec/frontend/environments/edit_environment_spec.js +++ b/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: { 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, }) .reply(...response); - await name.setValue(expected.name); - await url.setValue(expected.url); + await findNameInput().setValue(expected.name); + 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,64 @@ 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); }); + + describe('when a new environment is being created', () => { + beforeEach(() => { + wrapper = createWrapper({ + propsData: { + environment: { + name: '', + external_url: '', + }, + }, + }); + }); + + it('renders an enabled "Name" field', () => { + const nameInput = findNameInput(); + + expect(nameInput.attributes().disabled).toBeUndefined(); + expect(nameInput.element.value).toBe(''); + }); + + it('renders an "External URL" field', () => { + const urlInput = findExternalUrlInput(); + + expect(urlInput.element.value).toBe(''); + }); + }); + + describe('when an existing environment is being edited', () => { + beforeEach(() => { + wrapper = createWrapper({ + propsData: { + environment: { + id: 1, + name: 'test', + external_url: 'https://example.com/test', + }, + }, + }); + }); + + it('renders a disabled "Name" field', () => { + const nameInput = findNameInput(); + + expect(nameInput.attributes().disabled).toBe('disabled'); + expect(nameInput.element.value).toBe('test'); + }); + + it('renders an "External URL" field', () => { + const urlInput = findExternalUrlInput(); + + expect(urlInput.element.value).toBe('https://example.com/test'); + }); + }); }); -- GitLab From 3611ca257d472423c0659ebb624ab98f6d29fd61 Mon Sep 17 00:00:00 2001 From: Andrew Fontaine Date: Fri, 20 Aug 2021 16:22:59 -0400 Subject: [PATCH 3/7] Move Environment Jest Specs around The form unit tests should cover whether or not the name input is disabled, and the edit tests should be tweaked slightly to ensure an ID is passed to axios. --- .../environments/edit_environment_spec.js | 57 +++---------------- .../environments/environment_form_spec.js | 48 ++++++++++++++++ 2 files changed, 57 insertions(+), 48 deletions(-) diff --git a/spec/frontend/environments/edit_environment_spec.js b/spec/frontend/environments/edit_environment_spec.js index b51ad6de1c1129..284b4d6203dfc5 100644 --- a/spec/frontend/environments/edit_environment_spec.js +++ b/spec/frontend/environments/edit_environment_spec.js @@ -15,7 +15,7 @@ const DEFAULT_OPTS = { projectEnvironmentsPath: '/projects/environments', updateEnvironmentPath: '/proejcts/environments/1', }, - propsData: { environment: { name: 'foo', external_url: 'https://foo.example.com' } }, + propsData: { environment: { id: '0', name: 'foo', external_url: 'https://foo.example.com' } }, }; describe('~/environments/components/edit.vue', () => { @@ -48,6 +48,7 @@ describe('~/environments/components/edit.vue', () => { mock .onPut(DEFAULT_OPTS.provide.updateEnvironmentPath, { external_url: expected.url, + id: '0', }) .reply(...response); await findNameInput().setValue(expected.name); @@ -89,56 +90,16 @@ describe('~/environments/components/edit.vue', () => { expect(showsLoading()).toBe(false); }); - describe('when a new environment is being created', () => { - beforeEach(() => { - wrapper = createWrapper({ - propsData: { - environment: { - name: '', - external_url: '', - }, - }, - }); - }); - - it('renders an enabled "Name" field', () => { - const nameInput = findNameInput(); - - expect(nameInput.attributes().disabled).toBeUndefined(); - expect(nameInput.element.value).toBe(''); - }); - - it('renders an "External URL" field', () => { - const urlInput = findExternalUrlInput(); + it('renders a disabled "Name" field', () => { + const nameInput = findNameInput(); - expect(urlInput.element.value).toBe(''); - }); + expect(nameInput.attributes().disabled).toBe('disabled'); + expect(nameInput.element.value).toBe('foo'); }); - describe('when an existing environment is being edited', () => { - beforeEach(() => { - wrapper = createWrapper({ - propsData: { - environment: { - id: 1, - name: 'test', - external_url: 'https://example.com/test', - }, - }, - }); - }); - - it('renders a disabled "Name" field', () => { - const nameInput = findNameInput(); - - expect(nameInput.attributes().disabled).toBe('disabled'); - expect(nameInput.element.value).toBe('test'); - }); - - it('renders an "External URL" field', () => { - const urlInput = findExternalUrlInput(); + it('renders an "External URL" field', () => { + const urlInput = findExternalUrlInput(); - expect(urlInput.element.value).toBe('https://example.com/test'); - }); + expect(urlInput.element.value).toBe('https://foo.example.com'); }); }); diff --git a/spec/frontend/environments/environment_form_spec.js b/spec/frontend/environments/environment_form_spec.js index ed8fda71dab598..f1af08bcf326e9 100644 --- a/spec/frontend/environments/environment_form_spec.js +++ b/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'); + }); + }); }); -- GitLab From ae66f9aa09c53956761739c514b9456b777e6820 Mon Sep 17 00:00:00 2001 From: Andrew Fontaine Date: Fri, 20 Aug 2021 16:26:57 -0400 Subject: [PATCH 4/7] Drop Environment Name Value in Edit Spec This isn't allowed anymore. --- spec/frontend/environments/edit_environment_spec.js | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/frontend/environments/edit_environment_spec.js b/spec/frontend/environments/edit_environment_spec.js index 284b4d6203dfc5..2c8c054ccbd436 100644 --- a/spec/frontend/environments/edit_environment_spec.js +++ b/spec/frontend/environments/edit_environment_spec.js @@ -51,7 +51,6 @@ describe('~/environments/components/edit.vue', () => { id: '0', }) .reply(...response); - await findNameInput().setValue(expected.name); await findExternalUrlInput().setValue(expected.url); await findForm().trigger('submit'); -- GitLab From f3264d844e7025f1b84c3289e9122f2797526e84 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Tue, 24 Aug 2021 14:04:12 +0300 Subject: [PATCH 5/7] Add a helper text to disabled name input for environments Update renaming environments docs Mark environment.name filed in the API as deprecated --- .../environments/components/environment_form.vue | 15 +++++++++++++++ doc/api/environments.md | 2 +- doc/ci/environments/index.md | 13 +++++++++++++ locale/gitlab.pot | 6 ++++++ 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/environments/components/environment_form.vue b/app/assets/javascripts/environments/components/environment_form.vue index 14f173f8834c58..4ed0d181db31a4 100644 --- a/app/assets/javascripts/environments/components/environment_form.vue +++ b/app/assets/javascripts/environments/components/environment_form.vue @@ -39,12 +39,21 @@ export default { ), nameLabel: __('Name'), nameFeedback: __('This field is required'), + nameDisabledHelp: __( + "You can not 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: { @@ -105,6 +114,12 @@ export default { :state="valid.name" :invalid-feedback="$options.i18n.nameFeedback" > + 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/) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0a7fb8add7e6e7..98d627b55e72e1 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -16629,6 +16629,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 "" @@ -38161,6 +38164,9 @@ msgstr "" msgid "You can move around the graph by using the arrow keys." msgstr "" +msgid "You can not rename an environment after it's created." +msgstr "" + msgid "You can notify the app / group or a project by sending them an email notification" msgstr "" -- GitLab From 394b6bd8212aaaab89c6555c85f053b19b4d0b30 Mon Sep 17 00:00:00 2001 From: Andrew Fontaine Date: Wed, 25 Aug 2021 15:12:17 -0400 Subject: [PATCH 6/7] Run Formatter for Frontend Files --- .../environments/components/environment_form.vue | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/environments/components/environment_form.vue b/app/assets/javascripts/environments/components/environment_form.vue index 4ed0d181db31a4..904a07991a30df 100644 --- a/app/assets/javascripts/environments/components/environment_form.vue +++ b/app/assets/javascripts/environments/components/environment_form.vue @@ -39,12 +39,8 @@ export default { ), nameLabel: __('Name'), nameFeedback: __('This field is required'), - nameDisabledHelp: __( - "You can not rename an environment after it's created.", - ), - nameDisabledLinkText: __( - "How do I rename an environment?" - ), + nameDisabledHelp: __("You can not 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'), -- GitLab From bd59a859f954b0d90a678821029b61106090265e Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Thu, 26 Aug 2021 16:06:13 +0300 Subject: [PATCH 7/7] Use "cannot" instead of "can not" --- .../environments/components/environment_form.vue | 2 +- locale/gitlab.pot | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/environments/components/environment_form.vue b/app/assets/javascripts/environments/components/environment_form.vue index 904a07991a30df..1d1d8d61b66798 100644 --- a/app/assets/javascripts/environments/components/environment_form.vue +++ b/app/assets/javascripts/environments/components/environment_form.vue @@ -39,7 +39,7 @@ export default { ), nameLabel: __('Name'), nameFeedback: __('This field is required'), - nameDisabledHelp: __("You can not rename an environment after it's created."), + 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://'), diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 98d627b55e72e1..0be7ac729fcc68 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -38164,9 +38164,6 @@ msgstr "" msgid "You can move around the graph by using the arrow keys." msgstr "" -msgid "You can not rename an environment after it's created." -msgstr "" - msgid "You can notify the app / group or a project by sending them an email notification" msgstr "" @@ -38242,6 +38239,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 "" -- GitLab