From c60cdf7d4bf15e5420e607ba2ccd7f08b3d71476 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Wed, 31 May 2023 18:55:53 +0100 Subject: [PATCH] Remove feature flag pages_unique_domain Related to: https://gitlab.com/gitlab-org/gitlab/-/issues/364127 Changelog: other --- app/controllers/projects/pages_controller.rb | 10 +- app/services/projects/update_service.rb | 6 - .../projects/pages/_pages_settings.html.haml | 17 +-- .../development/pages_unique_domain.yml | 8 -- doc/user/project/pages/introduction.md | 1 + lib/gitlab/pages/url_builder.rb | 3 +- lib/gitlab/pages/virtual_host_finder.rb | 1 - .../projects/pages_controller_spec.rb | 47 +++---- spec/lib/gitlab/pages/url_builder_spec.rb | 72 ++-------- spec/requests/api/internal/pages_spec.rb | 14 -- spec/services/projects/update_service_spec.rb | 131 +++++++----------- .../pages/_pages_settings.html.haml_spec.rb | 10 -- 12 files changed, 89 insertions(+), 231 deletions(-) delete mode 100644 config/feature_flags/development/pages_unique_domain.yml diff --git a/app/controllers/projects/pages_controller.rb b/app/controllers/projects/pages_controller.rb index 6cfbb61fbb29a2..02579cd42837e4 100644 --- a/app/controllers/projects/pages_controller.rb +++ b/app/controllers/projects/pages_controller.rb @@ -65,15 +65,7 @@ def project_params end def project_params_attributes - attributes = %i[pages_https_only] - - return attributes unless Feature.enabled?(:pages_unique_domain, @project) - - attributes + [ - project_setting_attributes: [ - :pages_unique_domain_enabled - ] - ] + [:pages_https_only, { project_setting_attributes: [:pages_unique_domain_enabled] }] end end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index c470cedf0b820a..8639e2f833f273 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -51,12 +51,6 @@ def run_auto_devops_pipeline? private def add_pages_unique_domain - if Feature.disabled?(:pages_unique_domain, project) - params[:project_setting_attributes]&.delete(:pages_unique_domain_enabled) - - return - end - return unless params.dig(:project_setting_attributes, :pages_unique_domain_enabled) # If the project used a unique domain once, it'll always use the same diff --git a/app/views/projects/pages/_pages_settings.html.haml b/app/views/projects/pages/_pages_settings.html.haml index 914292eea026c3..b1ec7a362b7bff 100644 --- a/app/views/projects/pages/_pages_settings.html.haml +++ b/app/views/projects/pages/_pages_settings.html.haml @@ -1,8 +1,6 @@ - can_edit_max_page_size = can?(current_user, :update_max_pages_size) - can_enforce_https_only = Gitlab.config.pages.external_http || Gitlab.config.pages.external_https -- can_edit_unique_domain = Feature.enabled?(:pages_unique_domain, @project) -- return unless can_edit_max_page_size || can_enforce_https_only || can_edit_unique_domain = gitlab_ui_form_for @project, url: project_pages_path(@project), html: { class: 'inline', title: pages_https_only_title } do |f| - if can_edit_max_page_size = render_if_exists 'shared/pages/max_pages_size_input', form: f @@ -18,14 +16,13 @@ %p.gl-pl-6 = s_("GitLabPages|When enabled, all attempts to visit your website through HTTP are automatically redirected to HTTPS using a response with status code 301. Requires a valid certificate for all domains. %{docs_link_start}Learn more.%{link_end}").html_safe % { docs_link_start: docs_link_start, link_end: link_end } - - if can_edit_unique_domain - .form-group - = f.fields_for :project_setting do |settings| - = settings.gitlab_ui_checkbox_component :pages_unique_domain_enabled, - s_('GitLabPages|Use unique domain'), - label_options: { class: 'label-bold' } - %p.gl-pl-6 - = s_("GitLabPages|When enabled, a unique domain is generated to access pages.").html_safe + .form-group + = f.fields_for :project_setting do |settings| + = settings.gitlab_ui_checkbox_component :pages_unique_domain_enabled, + s_('GitLabPages|Use unique domain'), + label_options: { class: 'label-bold' } + %p.gl-pl-6 + = s_("GitLabPages|When enabled, a unique domain is generated to access pages.").html_safe .gl-mt-3 = f.submit s_('GitLabPages|Save changes'), pajamas_button: true diff --git a/config/feature_flags/development/pages_unique_domain.yml b/config/feature_flags/development/pages_unique_domain.yml deleted file mode 100644 index 39a1db5712f5aa..00000000000000 --- a/config/feature_flags/development/pages_unique_domain.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: pages_unique_domain -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/109011 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/388151 -milestone: '15.9' -type: development -group: group::editor -default_enabled: true diff --git a/doc/user/project/pages/introduction.md b/doc/user/project/pages/introduction.md index d00af81c10eda4..c79df365c63190 100644 --- a/doc/user/project/pages/introduction.md +++ b/doc/user/project/pages/introduction.md @@ -93,6 +93,7 @@ you can create your project first and access it under `http(s)://namespace.examp > - [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/9347) in GitLab 15.9 [with a flag](../../../administration/feature_flags.md) named `pages_unique_domain`. Disabled by default. > - [Enabled by default](https://gitlab.com/gitlab-org/gitlab/-/issues/388151) in GitLab 15.11. +> - [Feature flag removed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/122229) in GitLab 16.3. By default, every project in a group shares the same domain, for example, `group.gitlab.io`. This means that cookies are also shared for all projects in a group. diff --git a/lib/gitlab/pages/url_builder.rb b/lib/gitlab/pages/url_builder.rb index 215154b7248287..5a28a5ffd2371c 100644 --- a/lib/gitlab/pages/url_builder.rb +++ b/lib/gitlab/pages/url_builder.rb @@ -82,8 +82,7 @@ def portless(url) end def unique_domain_enabled? - Feature.enabled?(:pages_unique_domain, project) && - project.project_setting.pages_unique_domain_enabled? + project.project_setting.pages_unique_domain_enabled? end def config diff --git a/lib/gitlab/pages/virtual_host_finder.rb b/lib/gitlab/pages/virtual_host_finder.rb index 5fec60188f8513..667e67246ae0e7 100644 --- a/lib/gitlab/pages/virtual_host_finder.rb +++ b/lib/gitlab/pages/virtual_host_finder.rb @@ -29,7 +29,6 @@ def execute def by_unique_domain(name) project = Project.by_pages_enabled_unique_domain(name) - return unless Feature.enabled?(:pages_unique_domain, project) return unless project&.pages_deployed? ::Pages::VirtualDomain.new(projects: [project]) diff --git a/spec/controllers/projects/pages_controller_spec.rb b/spec/controllers/projects/pages_controller_spec.rb index ded5dd57e3e91d..34ec8d8d575367 100644 --- a/spec/controllers/projects/pages_controller_spec.rb +++ b/spec/controllers/projects/pages_controller_spec.rb @@ -182,44 +182,29 @@ create(:project_setting, project: project, pages_unique_domain_enabled: false) end - context 'with pages_unique_domain feature flag disabled' do - it 'does not update pages unique domain' do - stub_feature_flags(pages_unique_domain: false) + it 'updates pages_https_only and pages_unique_domain and redirects back to pages settings' do + expect { patch :update, params: request_params } + .to change { project.project_setting.reload.pages_unique_domain_enabled } + .from(false).to(true) - expect { patch :update, params: request_params } - .not_to change { project.project_setting.reload.pages_unique_domain_enabled } - end + expect(project.project_setting.pages_unique_domain).not_to be_nil + expect(response).to have_gitlab_http_status(:found) + expect(response).to redirect_to(project_pages_path(project)) end - context 'with pages_unique_domain feature flag enabled' do - before do - stub_feature_flags(pages_unique_domain: true) - end + context 'when it fails to update' do + it 'adds an error message' do + expect_next_instance_of(Projects::UpdateService) do |service| + expect(service) + .to receive(:execute) + .and_return(status: :error, message: 'some error happened') + end - it 'updates pages_https_only and pages_unique_domain and redirects back to pages settings' do expect { patch :update, params: request_params } - .to change { project.project_setting.reload.pages_unique_domain_enabled } - .from(false).to(true) + .not_to change { project.project_setting.reload.pages_unique_domain_enabled } - expect(project.project_setting.pages_unique_domain).not_to be_nil - expect(response).to have_gitlab_http_status(:found) expect(response).to redirect_to(project_pages_path(project)) - end - - context 'when it fails to update' do - it 'adds an error message' do - expect_next_instance_of(Projects::UpdateService) do |service| - expect(service) - .to receive(:execute) - .and_return(status: :error, message: 'some error happened') - end - - expect { patch :update, params: request_params } - .not_to change { project.project_setting.reload.pages_unique_domain_enabled } - - expect(response).to redirect_to(project_pages_path(project)) - expect(flash[:alert]).to eq('some error happened') - end + expect(flash[:alert]).to eq('some error happened') end end end diff --git a/spec/lib/gitlab/pages/url_builder_spec.rb b/spec/lib/gitlab/pages/url_builder_spec.rb index 8e1581704cb75d..ae94bbadffe618 100644 --- a/spec/lib/gitlab/pages/url_builder_spec.rb +++ b/spec/lib/gitlab/pages/url_builder_spec.rb @@ -83,60 +83,32 @@ context 'when not using pages_unique_domain' do subject(:pages_url) { builder.pages_url(with_unique_domain: false) } - context 'when pages_unique_domain feature flag is disabled' do - before do - stub_feature_flags(pages_unique_domain: false) - end + context 'when pages_unique_domain_enabled is false' do + let(:unique_domain_enabled) { false } it { is_expected.to eq('http://group.example.com/project') } end - context 'when pages_unique_domain feature flag is enabled' do - before do - stub_feature_flags(pages_unique_domain: true) - end - - context 'when pages_unique_domain_enabled is false' do - let(:unique_domain_enabled) { false } - - it { is_expected.to eq('http://group.example.com/project') } - end - - context 'when pages_unique_domain_enabled is true' do - let(:unique_domain_enabled) { true } + context 'when pages_unique_domain_enabled is true' do + let(:unique_domain_enabled) { true } - it { is_expected.to eq('http://group.example.com/project') } - end + it { is_expected.to eq('http://group.example.com/project') } end end context 'when using pages_unique_domain' do subject(:pages_url) { builder.pages_url(with_unique_domain: true) } - context 'when pages_unique_domain feature flag is disabled' do - before do - stub_feature_flags(pages_unique_domain: false) - end + context 'when pages_unique_domain_enabled is false' do + let(:unique_domain_enabled) { false } it { is_expected.to eq('http://group.example.com/project') } end - context 'when pages_unique_domain feature flag is enabled' do - before do - stub_feature_flags(pages_unique_domain: true) - end - - context 'when pages_unique_domain_enabled is false' do - let(:unique_domain_enabled) { false } - - it { is_expected.to eq('http://group.example.com/project') } - end - - context 'when pages_unique_domain_enabled is true' do - let(:unique_domain_enabled) { true } + context 'when pages_unique_domain_enabled is true' do + let(:unique_domain_enabled) { true } - it { is_expected.to eq('http://unique-domain.example.com') } - end + it { is_expected.to eq('http://unique-domain.example.com') } end end end @@ -144,30 +116,16 @@ describe '#unique_host' do subject(:unique_host) { builder.unique_host } - context 'when pages_unique_domain feature flag is disabled' do - before do - stub_feature_flags(pages_unique_domain: false) - end + context 'when pages_unique_domain_enabled is false' do + let(:unique_domain_enabled) { false } it { is_expected.to be_nil } end - context 'when pages_unique_domain feature flag is enabled' do - before do - stub_feature_flags(pages_unique_domain: true) - end + context 'when pages_unique_domain_enabled is true' do + let(:unique_domain_enabled) { true } - context 'when pages_unique_domain_enabled is false' do - let(:unique_domain_enabled) { false } - - it { is_expected.to be_nil } - end - - context 'when pages_unique_domain_enabled is true' do - let(:unique_domain_enabled) { true } - - it { is_expected.to eq('unique-domain.example.com') } - end + it { is_expected.to eq('unique-domain.example.com') } end end diff --git a/spec/requests/api/internal/pages_spec.rb b/spec/requests/api/internal/pages_spec.rb index 1006319eabf128..65aa2326af5c6f 100644 --- a/spec/requests/api/internal/pages_spec.rb +++ b/spec/requests/api/internal/pages_spec.rb @@ -151,20 +151,6 @@ project.mark_pages_as_deployed end - context 'when the feature flag is disabled' do - before do - stub_feature_flags(pages_unique_domain: false) - end - - context 'when there are no pages deployed for the related project' do - it 'responds with 204 No Content' do - get api('/internal/pages'), headers: auth_header, params: { host: 'unique-domain.example.com' } - - expect(response).to have_gitlab_http_status(:no_content) - end - end - end - context 'when the unique domain is disabled' do before do project.project_setting.update!(pages_unique_domain_enabled: false) diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 2186e7aa06cf22..d9090b87514216 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -794,104 +794,69 @@ let(:group) { create(:group, path: 'group') } let(:project) { create(:project, path: 'project', group: group) } - context 'with pages_unique_domain feature flag disabled' do - before do - stub_feature_flags(pages_unique_domain: false) - end - - it 'does not change pages unique domain' do - expect(project) - .to receive(:update) - .with({ project_setting_attributes: { has_confluence: true } }) - .and_call_original - - expect do - update_project(project, user, project_setting_attributes: { - has_confluence: true, - pages_unique_domain_enabled: true - }) - end.not_to change { project.project_setting.pages_unique_domain_enabled } - end + it 'updates project pages unique domain' do + expect do + update_project(project, user, project_setting_attributes: { + pages_unique_domain_enabled: true + }) + end.to change { project.project_setting.pages_unique_domain_enabled } - it 'does not remove other attributes' do - expect(project) - .to receive(:update) - .with({ name: 'True' }) - .and_call_original - - update_project(project, user, name: 'True') - end + expect(project.project_setting.pages_unique_domain_enabled).to eq true + expect(project.project_setting.pages_unique_domain).to match %r{project-group-\w+} end - context 'with pages_unique_domain feature flag enabled' do - before do - stub_feature_flags(pages_unique_domain: true) - end + it 'does not changes unique domain when it already exists' do + project.project_setting.update!( + pages_unique_domain_enabled: false, + pages_unique_domain: 'unique-domain' + ) - it 'updates project pages unique domain' do - expect do - update_project(project, user, project_setting_attributes: { - pages_unique_domain_enabled: true - }) - end.to change { project.project_setting.pages_unique_domain_enabled } + expect do + update_project(project, user, project_setting_attributes: { + pages_unique_domain_enabled: true + }) + end.to change { project.project_setting.pages_unique_domain_enabled } - expect(project.project_setting.pages_unique_domain_enabled).to eq true - expect(project.project_setting.pages_unique_domain).to match %r{project-group-\w+} - end + expect(project.project_setting.pages_unique_domain_enabled).to eq true + expect(project.project_setting.pages_unique_domain).to eq 'unique-domain' + end - it 'does not changes unique domain when it already exists' do - project.project_setting.update!( - pages_unique_domain_enabled: false, - pages_unique_domain: 'unique-domain' - ) + it 'does not changes unique domain when it disabling unique domain' do + project.project_setting.update!( + pages_unique_domain_enabled: true, + pages_unique_domain: 'unique-domain' + ) - expect do - update_project(project, user, project_setting_attributes: { - pages_unique_domain_enabled: true - }) - end.to change { project.project_setting.pages_unique_domain_enabled } + expect do + update_project(project, user, project_setting_attributes: { + pages_unique_domain_enabled: false + }) + end.not_to change { project.project_setting.pages_unique_domain } - expect(project.project_setting.pages_unique_domain_enabled).to eq true - expect(project.project_setting.pages_unique_domain).to eq 'unique-domain' - end + expect(project.project_setting.pages_unique_domain_enabled).to eq false + expect(project.project_setting.pages_unique_domain).to eq 'unique-domain' + end - it 'does not changes unique domain when it disabling unique domain' do - project.project_setting.update!( + context 'when there is another project with the unique domain' do + it 'fails pages unique domain already exists' do + create( + :project_setting, pages_unique_domain_enabled: true, pages_unique_domain: 'unique-domain' ) - expect do - update_project(project, user, project_setting_attributes: { - pages_unique_domain_enabled: false - }) - end.not_to change { project.project_setting.pages_unique_domain } + allow(Gitlab::Pages::RandomDomain) + .to receive(:generate) + .and_return('unique-domain') - expect(project.project_setting.pages_unique_domain_enabled).to eq false - expect(project.project_setting.pages_unique_domain).to eq 'unique-domain' - end + result = update_project(project, user, project_setting_attributes: { + pages_unique_domain_enabled: true + }) - context 'when there is another project with the unique domain' do - it 'fails pages unique domain already exists' do - create( - :project_setting, - pages_unique_domain_enabled: true, - pages_unique_domain: 'unique-domain' - ) - - allow(Gitlab::Pages::RandomDomain) - .to receive(:generate) - .and_return('unique-domain') - - result = update_project(project, user, project_setting_attributes: { - pages_unique_domain_enabled: true - }) - - expect(result).to eq( - status: :error, - message: 'Project setting pages unique domain has already been taken' - ) - end + expect(result).to eq( + status: :error, + message: 'Project setting pages unique domain has already been taken' + ) end end end diff --git a/spec/views/projects/pages/_pages_settings.html.haml_spec.rb b/spec/views/projects/pages/_pages_settings.html.haml_spec.rb index 36dd918cfa3d5b..4f54ddbdb60a3c 100644 --- a/spec/views/projects/pages/_pages_settings.html.haml_spec.rb +++ b/spec/views/projects/pages/_pages_settings.html.haml_spec.rb @@ -17,15 +17,5 @@ expect(rendered).to have_content('Use unique domain') end - - context 'when pages_unique_domain feature flag is disabled' do - it 'does not show the unique domain toggle' do - stub_feature_flags(pages_unique_domain: false) - - # We have to use `view.render` because `render` causes issues - # https://github.com/rails/rails/issues/41320 - expect(view.render('projects/pages/pages_settings')).to be_nil - end - end end end -- GitLab