From bc8886522abf9a3bb387b81289c0862a186b69c6 Mon Sep 17 00:00:00 2001 From: Allison Browne Date: Thu, 31 Aug 2023 16:20:25 -0400 Subject: [PATCH 1/8] Add the skip merge train setting Add the setting merge_trains_skip_train_allowed which will allow a merge request to bypass the train request on merge to master. Changelog: added EE: true Add the ee request spec --- app/graphql/types/ci/ci_cd_setting_type.rb | 3 + .../development/merge_trains_skip_train.yml | 8 +++ ...add_merge_immediately_to_ci_cd_settings.rb | 13 +++++ db/schema_migrations/20230816194153 | 1 + db/structure.sql | 3 +- doc/api/graphql/reference/index.md | 2 + .../settings/merge_requests_controller.rb | 1 + ee/app/controllers/ee/projects_controller.rb | 1 + .../ci/project_ci_cd_settings_update.rb | 4 ++ .../graphql/ee/types/ci/ci_cd_setting_type.rb | 19 +++++++ ee/app/models/ee/project.rb | 1 + .../_merge_trains_settings.html.haml | 5 ++ ee/lib/ee/api/entities/project.rb | 3 + ee/lib/ee/api/helpers/projects_helpers.rb | 2 + ee/lib/ee/api/projects.rb | 1 + .../merge_requests_controller_spec.rb | 31 +++++++++++ .../controllers/projects_controller_spec.rb | 30 ++++++++++ ...le_merge_trains_skip_train_allowed_spec.rb | 51 +++++++++++++++++ .../ci/project_ci_cd_settings_update_spec.rb | 19 ++++++- ee/spec/models/ee/project_spec.rb | 3 +- .../api/graphql/ci/ci_cd_setting_spec.rb | 55 +++++++++++++++++++ locale/gitlab.pot | 6 ++ spec/models/project_spec.rb | 1 + spec/requests/api/project_attributes.yml | 1 + spec/support/rspec_order_todo.yml | 1 - 25 files changed, 260 insertions(+), 5 deletions(-) create mode 100644 config/feature_flags/development/merge_trains_skip_train.yml create mode 100644 db/migrate/20230816194153_add_merge_immediately_to_ci_cd_settings.rb create mode 100644 db/schema_migrations/20230816194153 create mode 100644 ee/app/graphql/ee/types/ci/ci_cd_setting_type.rb create mode 100644 ee/spec/features/projects/settings/merge_requests/merge_trains/enable_merge_trains_skip_train_allowed_spec.rb create mode 100644 ee/spec/requests/api/graphql/ci/ci_cd_setting_spec.rb diff --git a/app/graphql/types/ci/ci_cd_setting_type.rb b/app/graphql/types/ci/ci_cd_setting_type.rb index 45ecbf5c084a2f..8a49c5a6a951d0 100644 --- a/app/graphql/types/ci/ci_cd_setting_type.rb +++ b/app/graphql/types/ci/ci_cd_setting_type.rb @@ -29,6 +29,7 @@ class CiCdSettingType < BaseObject null: true, description: 'Whether merge pipelines are enabled.', method: :merge_pipelines_enabled? + # TODO(Issue 422295): this is EE only and should be moved to the EE file field :merge_trains_enabled, GraphQL::Types::Boolean, null: true, @@ -41,3 +42,5 @@ class CiCdSettingType < BaseObject end end end + +Types::Ci::CiCdSettingType.prepend_mod_with('Types::Ci::CiCdSettingType') diff --git a/config/feature_flags/development/merge_trains_skip_train.yml b/config/feature_flags/development/merge_trains_skip_train.yml new file mode 100644 index 00000000000000..3d60acef457959 --- /dev/null +++ b/config/feature_flags/development/merge_trains_skip_train.yml @@ -0,0 +1,8 @@ +--- +name: merge_trains_skip_train +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129422 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/422111 +milestone: '16.4' +type: development +group: group::pipeline execution +default_enabled: false diff --git a/db/migrate/20230816194153_add_merge_immediately_to_ci_cd_settings.rb b/db/migrate/20230816194153_add_merge_immediately_to_ci_cd_settings.rb new file mode 100644 index 00000000000000..eee1ba6f78176c --- /dev/null +++ b/db/migrate/20230816194153_add_merge_immediately_to_ci_cd_settings.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class AddMergeImmediatelyToCiCdSettings < Gitlab::Database::Migration[2.1] + enable_lock_retries! + + def up + add_column :project_ci_cd_settings, :merge_trains_skip_train_allowed, :boolean, default: false, null: false + end + + def down + remove_column :project_ci_cd_settings, :merge_trains_skip_train_allowed + end +end diff --git a/db/schema_migrations/20230816194153 b/db/schema_migrations/20230816194153 new file mode 100644 index 00000000000000..3814eab38cfcb2 --- /dev/null +++ b/db/schema_migrations/20230816194153 @@ -0,0 +1 @@ +dac4ef39d622a6a2f1552fcce285828fce97a1cbfe17eacc26b31a5d933e0162 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 622c5988d2ac94..57d77b05a03a70 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -21348,7 +21348,8 @@ CREATE TABLE project_ci_cd_settings ( separated_caches boolean DEFAULT true NOT NULL, allow_fork_pipelines_to_run_in_parent_project boolean DEFAULT true NOT NULL, inbound_job_token_scope_enabled boolean DEFAULT true NOT NULL, - forward_deployment_rollback_allowed boolean DEFAULT true NOT NULL + forward_deployment_rollback_allowed boolean DEFAULT true NOT NULL, + merge_trains_skip_train_allowed boolean DEFAULT false NOT NULL ); CREATE SEQUENCE project_ci_cd_settings_id_seq diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 17033349edfc5b..2a49e7ec1135cd 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -5543,6 +5543,7 @@ Input type: `ProjectCiCdSettingsUpdateInput` | `keepLatestArtifact` | [`Boolean`](#boolean) | Indicates if the latest artifact should be kept for the project. | | `mergePipelinesEnabled` | [`Boolean`](#boolean) | Indicates if merge pipelines are enabled for the project. | | `mergeTrainsEnabled` | [`Boolean`](#boolean) | Indicates if merge trains are enabled for the project. | +| `mergeTrainsSkipTrainAllowed` | [`Boolean`](#boolean) | Indicates wheather an option is allowed to merge without refreshing the merge train. | #### Fields @@ -23268,6 +23269,7 @@ four standard [pagination arguments](#connection-pagination-arguments): | `keepLatestArtifact` | [`Boolean`](#boolean) | Whether to keep the latest builds artifacts. | | `mergePipelinesEnabled` | [`Boolean`](#boolean) | Whether merge pipelines are enabled. | | `mergeTrainsEnabled` | [`Boolean`](#boolean) | Whether merge trains are enabled. | +| `mergeTrainsSkipTrainAllowed` | [`Boolean!`](#boolean) | Whether merge immediately is allowed for merge trains. | | `project` | [`Project`](#project) | Project the CI/CD settings belong to. | ### `ProjectConversations` diff --git a/ee/app/controllers/ee/projects/settings/merge_requests_controller.rb b/ee/app/controllers/ee/projects/settings/merge_requests_controller.rb index 91f0eab2a74280..1a0f79a0dc20ce 100644 --- a/ee/app/controllers/ee/projects/settings/merge_requests_controller.rb +++ b/ee/app/controllers/ee/projects/settings/merge_requests_controller.rb @@ -50,6 +50,7 @@ def project_params_ee attrs << %i[merge_pipelines_enabled] if allow_merge_pipelines_params? attrs << %i[merge_trains_enabled] if allow_merge_trains_params? + attrs << %i[merge_trains_skip_train_allowed] if allow_merge_trains_params? attrs << %i[only_allow_merge_if_all_status_checks_passed] if allow_external_status_checks? attrs += merge_request_rules_params diff --git a/ee/app/controllers/ee/projects_controller.rb b/ee/app/controllers/ee/projects_controller.rb index cd299ca893199f..e901a242fa2590 100644 --- a/ee/app/controllers/ee/projects_controller.rb +++ b/ee/app/controllers/ee/projects_controller.rb @@ -129,6 +129,7 @@ def project_params_ee attrs << %i[merge_pipelines_enabled] if allow_merge_pipelines_params? attrs << %i[merge_trains_enabled] if allow_merge_trains_params? + attrs << %i[merge_trains_skip_train_allowed] if allow_merge_trains_params? attrs += merge_request_rules_params diff --git a/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb b/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb index 7a401485be50e9..031a998b849569 100644 --- a/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb +++ b/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb @@ -15,6 +15,10 @@ module ProjectCiCdSettingsUpdate argument :merge_trains_enabled, GraphQL::Types::Boolean, required: false, description: 'Indicates if merge trains are enabled for the project.' + + argument :merge_trains_skip_train_allowed, GraphQL::Types::Boolean, + required: false, + description: 'Indicates wheather an option is allowed to merge without refreshing the merge train.' end override :resolve diff --git a/ee/app/graphql/ee/types/ci/ci_cd_setting_type.rb b/ee/app/graphql/ee/types/ci/ci_cd_setting_type.rb new file mode 100644 index 00000000000000..f1b96f5a361a91 --- /dev/null +++ b/ee/app/graphql/ee/types/ci/ci_cd_setting_type.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module EE + module Types + module Ci + module CiCdSettingType + extend ActiveSupport::Concern + + prepended do + field :merge_trains_skip_train_allowed, + GraphQL::Types::Boolean, + null: false, + description: 'Whether merge immediately is allowed for merge trains.', + method: :merge_trains_skip_train_allowed? + end + end + end + end +end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index a05346d4b9dc53..95c02e2c84b9cf 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -333,6 +333,7 @@ def lock_for_confirmation!(id) delegate :merge_pipelines_enabled, :merge_pipelines_enabled=, to: :ci_cd_settings, allow_nil: true delegate :merge_trains_enabled, :merge_trains_enabled=, to: :ci_cd_settings, allow_nil: true + delegate :merge_trains_skip_train_allowed, :merge_trains_skip_train_allowed=, to: :ci_cd_settings, allow_nil: true delegate :auto_rollback_enabled, :auto_rollback_enabled=, to: :ci_cd_settings, allow_nil: true diff --git a/ee/app/views/projects/settings/merge_requests/_merge_trains_settings.html.haml b/ee/app/views/projects/settings/merge_requests/_merge_trains_settings.html.haml index fc12aba5e6c8d3..a5f4c7af684057 100644 --- a/ee/app/views/projects/settings/merge_requests/_merge_trains_settings.html.haml +++ b/ee/app/views/projects/settings/merge_requests/_merge_trains_settings.html.haml @@ -7,3 +7,8 @@ s_('ProjectSettings|Enable merge trains'), help_text: s_('ProjectSettings|Merge requests approved for merge are queued, and pipelines validate the combined results of the source and target branches before merge. %{link_start}What are merge trains?%{link_end}').html_safe % { link_start: merge_trains_help_link_start, link_end: ''.html_safe }, checkbox_options: { class: 'js-merge-options-merge-trains', data: { qa_selector: 'merge_trains_checkbox' } } + - if Feature.enabled?(:merge_trains_skip_train) + = form.gitlab_ui_checkbox_component :merge_trains_skip_train_allowed, + s_('ProjectSettings|Allow skipping the merge train'), + help_text: s_('ProjectSettings|Merge requests can be merged immediately without interrupting the train.'), + checkbox_options: { class: 'js-merge-options-merge-trains-skip-train-allowed' } diff --git a/ee/lib/ee/api/entities/project.rb b/ee/lib/ee/api/entities/project.rb index 346d27cf201d98..01cd607c91c072 100644 --- a/ee/lib/ee/api/entities/project.rb +++ b/ee/lib/ee/api/entities/project.rb @@ -51,6 +51,9 @@ def preload_relation(projects_relation, options = {}) end expose :merge_pipelines_enabled?, as: :merge_pipelines_enabled, if: ->(project, _) { project.feature_available?(:merge_pipelines) } expose :merge_trains_enabled?, as: :merge_trains_enabled, if: ->(project, _) { project.feature_available?(:merge_pipelines) } + expose :merge_trains_skip_train_allowed, if: ->(project, _) { project.feature_available?(:merge_pipelines) } do |project| + project.ci_cd_settings.merge_trains_skip_train_allowed + end expose :only_allow_merge_if_all_status_checks_passed, if: ->(project, _) { project.feature_available?(:external_status_checks) } expose :allow_pipeline_trigger_approve_deployment, documentation: { type: 'boolean' }, if: ->(project, _) { project.feature_available?(:protected_environments) } end diff --git a/ee/lib/ee/api/helpers/projects_helpers.rb b/ee/lib/ee/api/helpers/projects_helpers.rb index eb4e53550ed127..d02515cef67ae9 100644 --- a/ee/lib/ee/api/helpers/projects_helpers.rb +++ b/ee/lib/ee/api/helpers/projects_helpers.rb @@ -42,6 +42,7 @@ module ProjectsHelpers optional :merge_requests_template, type: String, desc: 'Default description for merge requests. Description is parsed with GitLab Flavored Markdown.' optional :merge_pipelines_enabled, type: Grape::API::Boolean, desc: 'Enable merged results pipelines.' optional :merge_trains_enabled, type: Grape::API::Boolean, desc: 'Enable merge trains.' + optional :merge_trains_skip_train_allowed, type: Grape::API::Boolean, desc: 'Allow merge train merge requests to be merged without waiting for pipelines to finish.' end end @@ -65,6 +66,7 @@ def update_params_at_least_one_of :merge_requests_template, :merge_pipelines_enabled, :merge_trains_enabled, + :merge_trains_skip_train_allowed, :requirements_access_level ] end diff --git a/ee/lib/ee/api/projects.rb b/ee/lib/ee/api/projects.rb index 2cf9a1cf754a02..56fe90eff7cf4a 100644 --- a/ee/lib/ee/api/projects.rb +++ b/ee/lib/ee/api/projects.rb @@ -127,6 +127,7 @@ def verify_merge_pipelines_attrs!(project, attrs) attrs.delete(:merge_pipelines_enabled) unless project.feature_available?(:merge_pipelines) attrs.delete(:merge_trains_enabled) unless project.feature_available?(:merge_trains) + attrs.delete(:merge_trains_skip_train_allowed) unless project.feature_available?(:merge_trains) end def check_audit_events_available!(project) diff --git a/ee/spec/controllers/projects/settings/merge_requests_controller_spec.rb b/ee/spec/controllers/projects/settings/merge_requests_controller_spec.rb index 1471a6ca55794e..3f2dbbfb6fae24 100644 --- a/ee/spec/controllers/projects/settings/merge_requests_controller_spec.rb +++ b/ee/spec/controllers/projects/settings/merge_requests_controller_spec.rb @@ -129,6 +129,37 @@ end end + context 'when merge_trains_skip_train_allowed param is specified' do + let(:params) { { merge_trains_skip_train_allowed: true } } + let(:ci_settings) { project.ci_cd_settings } + + let(:request) do + put :update, params: { namespace_id: project.namespace, project_id: project, project: params } + end + + before do + stub_licensed_features(merge_pipelines: true, merge_trains: true) + end + + it 'updates the attribute' do + request + + expect(ci_settings.merge_trains_skip_train_allowed).to be_truthy + end + + context 'when license is not sufficient' do + before do + stub_licensed_features(merge_trains: false) + end + + it 'does not update the attribute' do + request + + expect(ci_settings.merge_trains_skip_train_allowed).to be_falsy + end + end + end + context 'when only_allow_merge_if_all_status_checks_passed param is specified' do let(:params) { { project_setting_attributes: { only_allow_merge_if_all_status_checks_passed: true } } } diff --git a/ee/spec/controllers/projects_controller_spec.rb b/ee/spec/controllers/projects_controller_spec.rb index 4ffc59e1a9d7bd..4a062fc9222838 100644 --- a/ee/spec/controllers/projects_controller_spec.rb +++ b/ee/spec/controllers/projects_controller_spec.rb @@ -422,6 +422,36 @@ end end + context 'when merge_trains_skip_train_allowed param is specified' do + let(:params) { { merge_trains_skip_train_allowed: true } } + + let(:request) do + put :update, params: { namespace_id: project.namespace, id: project, project: params } + end + + before do + stub_licensed_features(merge_pipelines: true, merge_trains: true) + end + + it 'updates the attribute' do + request + + expect(project.merge_trains_skip_train_allowed).to be_truthy + end + + context 'when license is not sufficient' do + before do + stub_licensed_features(merge_trains: false) + end + + it 'does not update the attribute' do + request + + expect(project.merge_trains_skip_train_allowed).to be_falsy + end + end + end + context 'when auto_rollback_enabled param is specified' do let(:params) { { auto_rollback_enabled: true } } diff --git a/ee/spec/features/projects/settings/merge_requests/merge_trains/enable_merge_trains_skip_train_allowed_spec.rb b/ee/spec/features/projects/settings/merge_requests/merge_trains/enable_merge_trains_skip_train_allowed_spec.rb new file mode 100644 index 00000000000000..aea7dc4eb26a70 --- /dev/null +++ b/ee/spec/features/projects/settings/merge_requests/merge_trains/enable_merge_trains_skip_train_allowed_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Merge Trains Skip Train Setting', :js, feature_category: :merge_trains do + let_it_be_with_reload(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + before_all do + project.add_maintainer(user) + end + + before do + stub_licensed_features(merge_pipelines: true, merge_trains: true) + sign_in(user) + end + + context 'when visiting the project settings page' do + before do + visit project_settings_merge_requests_path(project) + wait_for_requests + end + + it 'is unchecked by default' do + expect(find('#project_merge_trains_skip_train_allowed')).not_to be_checked + end + + it 'can be enabled' do + page.within('#project-merge-options') do + check _('Allow skipping the merge train') + expect(find('#project_merge_trains_skip_train_allowed')).to be_checked + end + + click_button('Save changes') + + wait_for_requests + + expect(project.ci_cd_settings.merge_trains_skip_train_allowed).to eq(true) + end + end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(merge_trains_skip_train: false) + end + + it 'does not show the checkbox' do + expect(page).not_to have_checked_field('#project_merge_trains_skip_train_allowed') + end + end +end diff --git a/ee/spec/graphql/ee/mutations/ci/project_ci_cd_settings_update_spec.rb b/ee/spec/graphql/ee/mutations/ci/project_ci_cd_settings_update_spec.rb index d35d538414c777..62cdb81e697e9d 100644 --- a/ee/spec/graphql/ee/mutations/ci/project_ci_cd_settings_update_spec.rb +++ b/ee/spec/graphql/ee/mutations/ci/project_ci_cd_settings_update_spec.rb @@ -15,8 +15,11 @@ before do stub_licensed_features(merge_pipelines: true, merge_trains: true) stub_feature_flags(disable_merge_trains: false) - project.merge_pipelines_enabled = nil - project.merge_trains_enabled = false + project.update!( + merge_pipelines_enabled: nil, + merge_trains_enabled: false, + merge_trains_skip_train_allowed: false + ) end describe '#resolve' do @@ -53,6 +56,18 @@ expect(project.merge_trains_enabled?).to eq(true) end end + + context 'when merge_trains_skip_train_allowed is set to true' do + let(:mutation_params) do + { + merge_trains_skip_train_allowed: true + } + end + + it 'updates the value' do + expect(project.ci_cd_settings.merge_trains_skip_train_allowed?).to eq(true) + end + end end describe 'when the inbound_job_token_scope parameter is not provided' do diff --git a/ee/spec/models/ee/project_spec.rb b/ee/spec/models/ee/project_spec.rb index 6d70b3e131fa93..9b9b40f6a66d67 100644 --- a/ee/spec/models/ee/project_spec.rb +++ b/ee/spec/models/ee/project_spec.rb @@ -92,7 +92,8 @@ # EE only 'auto_rollback_enabled' => '', 'merge_pipelines_enabled' => '', - 'merge_trains_enabled' => '' + 'merge_trains_enabled' => '', + 'merge_trains_skip_train_allowed' => '' } end diff --git a/ee/spec/requests/api/graphql/ci/ci_cd_setting_spec.rb b/ee/spec/requests/api/graphql/ci/ci_cd_setting_spec.rb new file mode 100644 index 00000000000000..1469eb53fff5dc --- /dev/null +++ b/ee/spec/requests/api/graphql/ci/ci_cd_setting_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Getting Ci Cd Setting', feature_category: :continuous_integration do + include GraphqlHelpers + + let_it_be_with_reload(:project) { create(:project, :repository) } + let_it_be(:owner) { project.first_owner } + let_it_be(:user) { create(:user) } + + let(:fields) do + <<~QUERY + #{all_graphql_fields_for('ProjectCiCdSetting', max_depth: 1)} + QUERY + end + + let(:query) do + graphql_query_for( + 'project', + { 'fullPath' => project.full_path }, + query_graphql_field('ciCdSettings', {}, fields) + ) + end + + let(:settings_data) { graphql_data['project']['ciCdSettings'] } + + context 'without permissions' do + before_all do + project.add_reporter(user) + end + + before do + post_graphql(query, current_user: user) + end + + it_behaves_like 'a working graphql query' + + specify { expect(settings_data).to be nil } + end + + context 'with project permissions' do + before do + post_graphql(query, current_user: owner) + end + + let(:skip_train_setting) { project.ci_cd_settings.merge_trains_skip_train_allowed? } + + it_behaves_like 'a working graphql query' + + it 'fetches the settings data' do + expect(settings_data['mergeTrainsSkipTrainAllowed']).to eq skip_train_setting + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 840af45c1fbdc8..395c72d4265d2c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -36854,6 +36854,9 @@ msgstr "" msgid "ProjectSettings|Allow anyone to pull from Package Registry" msgstr "" +msgid "ProjectSettings|Allow skipping the merge train" +msgstr "" + msgid "ProjectSettings|Always show thumbs-up and thumbs-down emoji buttons on issues, merge requests, and snippets." msgstr "" @@ -37073,6 +37076,9 @@ msgstr "" msgid "ProjectSettings|Merge requests approved for merge are queued, and pipelines validate the combined results of the source and target branches before merge. %{link_start}What are merge trains?%{link_end}" msgstr "" +msgid "ProjectSettings|Merge requests can be merged immediately without interrupting the train." +msgstr "" + msgid "ProjectSettings|Merge suggestions" msgstr "" diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 6193a565d76fb4..877da5e81ce3e3 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1158,6 +1158,7 @@ merge_pipelines_enabled merge_trains_enabled auto_rollback_enabled + merge_trains_skip_train_allowed ) end end diff --git a/spec/requests/api/project_attributes.yml b/spec/requests/api/project_attributes.yml index 677cb243a7cab0..d95f96c25d6414 100644 --- a/spec/requests/api/project_attributes.yml +++ b/spec/requests/api/project_attributes.yml @@ -94,6 +94,7 @@ ci_cd_settings: - id - project_id - merge_trains_enabled + - merge_trains_skip_train_allowed - merge_pipelines_enabled - auto_rollback_enabled - inbound_job_token_scope_enabled diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index c2cb29064b69bf..0c530f63e598cf 100644 --- a/spec/support/rspec_order_todo.yml +++ b/spec/support/rspec_order_todo.yml @@ -591,7 +591,6 @@ - './ee/spec/graphql/api/vulnerabilities_spec.rb' - './ee/spec/graphql/ee/mutations/boards/issues/issue_move_list_spec.rb' - './ee/spec/graphql/ee/mutations/boards/lists/create_spec.rb' -- './ee/spec/graphql/ee/mutations/ci/project_ci_cd_settings_update_spec.rb' - './ee/spec/graphql/ee/mutations/ci/runner/update_spec.rb' - './ee/spec/graphql/ee/mutations/concerns/mutations/resolves_issuable_spec.rb' - './ee/spec/graphql/ee/resolvers/board_list_issues_resolver_spec.rb' -- GitLab From 8117256d8a21d48d5b14bcd2e32e941f5074a9de Mon Sep 17 00:00:00 2001 From: Allison Browne Date: Wed, 6 Sep 2023 16:03:10 -0400 Subject: [PATCH 2/8] Code Review: Fix typo in graphql desc --- doc/api/graphql/reference/index.md | 2 +- ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 2a49e7ec1135cd..2e1a2fe22c7d79 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -5543,7 +5543,7 @@ Input type: `ProjectCiCdSettingsUpdateInput` | `keepLatestArtifact` | [`Boolean`](#boolean) | Indicates if the latest artifact should be kept for the project. | | `mergePipelinesEnabled` | [`Boolean`](#boolean) | Indicates if merge pipelines are enabled for the project. | | `mergeTrainsEnabled` | [`Boolean`](#boolean) | Indicates if merge trains are enabled for the project. | -| `mergeTrainsSkipTrainAllowed` | [`Boolean`](#boolean) | Indicates wheather an option is allowed to merge without refreshing the merge train. | +| `mergeTrainsSkipTrainAllowed` | [`Boolean`](#boolean) | Indicates whether an option is allowed to merge without refreshing the merge train. | #### Fields diff --git a/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb b/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb index 031a998b849569..73d89cddd3adc8 100644 --- a/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb +++ b/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb @@ -18,7 +18,7 @@ module ProjectCiCdSettingsUpdate argument :merge_trains_skip_train_allowed, GraphQL::Types::Boolean, required: false, - description: 'Indicates wheather an option is allowed to merge without refreshing the merge train.' + description: 'Indicates whether an option is allowed to merge without refreshing the merge train.' end override :resolve -- GitLab From 68cbc711465a8dccb3ed34ccda79bbc6977d062d Mon Sep 17 00:00:00 2001 From: drew Date: Mon, 11 Sep 2023 23:32:14 -0400 Subject: [PATCH 3/8] Shorten API entity field definition --- ee/lib/ee/api/entities/project.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ee/lib/ee/api/entities/project.rb b/ee/lib/ee/api/entities/project.rb index 01cd607c91c072..b122e5ecc93b35 100644 --- a/ee/lib/ee/api/entities/project.rb +++ b/ee/lib/ee/api/entities/project.rb @@ -51,9 +51,7 @@ def preload_relation(projects_relation, options = {}) end expose :merge_pipelines_enabled?, as: :merge_pipelines_enabled, if: ->(project, _) { project.feature_available?(:merge_pipelines) } expose :merge_trains_enabled?, as: :merge_trains_enabled, if: ->(project, _) { project.feature_available?(:merge_pipelines) } - expose :merge_trains_skip_train_allowed, if: ->(project, _) { project.feature_available?(:merge_pipelines) } do |project| - project.ci_cd_settings.merge_trains_skip_train_allowed - end + expose :merge_trains_skip_train_allowed, as: :merge_trains_skip_train_allowed, if: ->(project, _) { project.feature_available?(:merge_pipelines) } expose :only_allow_merge_if_all_status_checks_passed, if: ->(project, _) { project.feature_available?(:external_status_checks) } expose :allow_pipeline_trigger_approve_deployment, documentation: { type: 'boolean' }, if: ->(project, _) { project.feature_available?(:protected_environments) } end -- GitLab From cc344f891a894ffda34bc60ede6b1d3d868ba68c Mon Sep 17 00:00:00 2001 From: drew Date: Mon, 11 Sep 2023 23:47:49 -0400 Subject: [PATCH 4/8] Add feature flag check to skip merge train setting via GraphQL --- .../mutations/ci/project_ci_cd_settings_update.rb | 13 ++++++++++--- doc/api/graphql/reference/index.md | 2 +- .../mutations/ci/project_ci_cd_settings_update.rb | 7 ++++++- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/app/graphql/mutations/ci/project_ci_cd_settings_update.rb b/app/graphql/mutations/ci/project_ci_cd_settings_update.rb index 082c345adf6d5e..7df277641bf32e 100644 --- a/app/graphql/mutations/ci/project_ci_cd_settings_update.rb +++ b/app/graphql/mutations/ci/project_ci_cd_settings_update.rb @@ -6,6 +6,7 @@ class ProjectCiCdSettingsUpdate < BaseMutation graphql_name 'ProjectCiCdSettingsUpdate' include FindsProject + include Gitlab::Utils::StrongMemoize authorize :admin_project @@ -37,13 +38,11 @@ class ProjectCiCdSettingsUpdate < BaseMutation description: 'CI/CD settings after mutation.' def resolve(full_path:, **args) - project = authorized_find!(full_path) - if args[:job_token_scope_enabled] raise Gitlab::Graphql::Errors::ArgumentError, 'job_token_scope_enabled can only be set to false' end - settings = project.ci_cd_settings + settings = project(full_path).ci_cd_settings settings.update(args) { @@ -51,6 +50,14 @@ def resolve(full_path:, **args) errors: errors_on_object(settings) } end + + private + + def project(full_path) + strong_memoize_with(:project, full_path) do + authorized_find!(full_path) + end + end end end end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 2e1a2fe22c7d79..040a11c649bfdc 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -5543,7 +5543,7 @@ Input type: `ProjectCiCdSettingsUpdateInput` | `keepLatestArtifact` | [`Boolean`](#boolean) | Indicates if the latest artifact should be kept for the project. | | `mergePipelinesEnabled` | [`Boolean`](#boolean) | Indicates if merge pipelines are enabled for the project. | | `mergeTrainsEnabled` | [`Boolean`](#boolean) | Indicates if merge trains are enabled for the project. | -| `mergeTrainsSkipTrainAllowed` | [`Boolean`](#boolean) | Indicates whether an option is allowed to merge without refreshing the merge train. | +| `mergeTrainsSkipTrainAllowed` | [`Boolean`](#boolean) | Indicates whether an option is allowed to merge without refreshing the merge train. Ignored unless the `merge_trains_skip_train` feature flag is also enabled. | #### Fields diff --git a/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb b/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb index 73d89cddd3adc8..475f8655dca717 100644 --- a/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb +++ b/ee/app/graphql/ee/mutations/ci/project_ci_cd_settings_update.rb @@ -18,11 +18,16 @@ module ProjectCiCdSettingsUpdate argument :merge_trains_skip_train_allowed, GraphQL::Types::Boolean, required: false, - description: 'Indicates whether an option is allowed to merge without refreshing the merge train.' + description: 'Indicates whether an option is allowed to merge without refreshing the merge train. ' \ + 'Ignored unless the `merge_trains_skip_train` feature flag is also enabled.' end override :resolve def resolve(full_path:, **args) + if ::Feature.disabled?(:merge_trains_skip_train, project(full_path)) + args.delete(:merge_trains_skip_train_allowed) + end + super.tap do |result| ci_cd_settings = result[:ci_cd_settings] audit_project = ci_cd_settings.project -- GitLab From 19958610980c9858fc0ddea7b21fca13265bbd45 Mon Sep 17 00:00:00 2001 From: drew Date: Tue, 12 Sep 2023 16:35:49 -0400 Subject: [PATCH 5/8] Use model-defined accessor methods Add #merge_train_skip_train_allowed? to Project and ProjectCiCdSetting --- ee/app/models/ee/project.rb | 6 ++++++ ee/app/models/ee/project_ci_cd_setting.rb | 4 ++++ ee/lib/ee/api/entities/project.rb | 2 +- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index 95c02e2c84b9cf..3fde1178da93af 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -1112,6 +1112,12 @@ def merge_trains_enabled? ci_cd_settings.merge_trains_enabled? end + def merge_trains_skip_train_allowed? + return false unless ci_cd_settings + + ci_cd_settings.merge_trains_skip_train_allowed? + end + def auto_rollback_enabled? return false unless ci_cd_settings diff --git a/ee/app/models/ee/project_ci_cd_setting.rb b/ee/app/models/ee/project_ci_cd_setting.rb index 7884c3888dcf61..1d7e03009e1881 100644 --- a/ee/app/models/ee/project_ci_cd_setting.rb +++ b/ee/app/models/ee/project_ci_cd_setting.rb @@ -28,6 +28,10 @@ def auto_rollback_enabled? super && project.feature_available?(:auto_rollback) end + def merge_trains_skip_train_allowed? + merge_trains_skip_train_allowed && ::Feature.enabled?(:merge_trains_skip_train, project) + end + private def merge_trains_disabled?(project) diff --git a/ee/lib/ee/api/entities/project.rb b/ee/lib/ee/api/entities/project.rb index b122e5ecc93b35..a2d62c4126395a 100644 --- a/ee/lib/ee/api/entities/project.rb +++ b/ee/lib/ee/api/entities/project.rb @@ -51,7 +51,7 @@ def preload_relation(projects_relation, options = {}) end expose :merge_pipelines_enabled?, as: :merge_pipelines_enabled, if: ->(project, _) { project.feature_available?(:merge_pipelines) } expose :merge_trains_enabled?, as: :merge_trains_enabled, if: ->(project, _) { project.feature_available?(:merge_pipelines) } - expose :merge_trains_skip_train_allowed, as: :merge_trains_skip_train_allowed, if: ->(project, _) { project.feature_available?(:merge_pipelines) } + expose :merge_trains_skip_train_allowed?, as: :merge_trains_skip_train_allowed, if: ->(project, _) { project.feature_available?(:merge_pipelines) } expose :only_allow_merge_if_all_status_checks_passed, if: ->(project, _) { project.feature_available?(:external_status_checks) } expose :allow_pipeline_trigger_approve_deployment, documentation: { type: 'boolean' }, if: ->(project, _) { project.feature_available?(:protected_environments) } end -- GitLab From 2a74262c5ffd82a4e135a1dc93d93821c3a160e2 Mon Sep 17 00:00:00 2001 From: drew Date: Wed, 13 Sep 2023 17:58:52 -0400 Subject: [PATCH 6/8] Update skip-train setting description --- .../settings/merge_requests/_merge_trains_settings.html.haml | 2 +- locale/gitlab.pot | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/views/projects/settings/merge_requests/_merge_trains_settings.html.haml b/ee/app/views/projects/settings/merge_requests/_merge_trains_settings.html.haml index a5f4c7af684057..41c420a6ba3c41 100644 --- a/ee/app/views/projects/settings/merge_requests/_merge_trains_settings.html.haml +++ b/ee/app/views/projects/settings/merge_requests/_merge_trains_settings.html.haml @@ -10,5 +10,5 @@ - if Feature.enabled?(:merge_trains_skip_train) = form.gitlab_ui_checkbox_component :merge_trains_skip_train_allowed, s_('ProjectSettings|Allow skipping the merge train'), - help_text: s_('ProjectSettings|Merge requests can be merged immediately without interrupting the train.'), + help_text: s_('ProjectSettings|Merge requests can be set to merge immediately without interrupting the merge train. Commits in earlier merge train pipelines might not get validated with immediately merged commits.'), checkbox_options: { class: 'js-merge-options-merge-trains-skip-train-allowed' } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 395c72d4265d2c..860ec9cad7c7da 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -37076,7 +37076,7 @@ msgstr "" msgid "ProjectSettings|Merge requests approved for merge are queued, and pipelines validate the combined results of the source and target branches before merge. %{link_start}What are merge trains?%{link_end}" msgstr "" -msgid "ProjectSettings|Merge requests can be merged immediately without interrupting the train." +msgid "ProjectSettings|Merge requests can be set to merge immediately without interrupting the merge train. Commits in earlier merge train pipelines might not get validated with immediately merged commits." msgstr "" msgid "ProjectSettings|Merge suggestions" -- GitLab From be4f6d47b42c93ca00c7c2315adf4099c248caf9 Mon Sep 17 00:00:00 2001 From: drew Date: Mon, 18 Sep 2023 00:59:09 -0400 Subject: [PATCH 7/8] Validate presence of default column value --- app/models/project_ci_cd_setting.rb | 1 + ee/spec/models/project_ci_cd_setting_spec.rb | 2 ++ 2 files changed, 3 insertions(+) diff --git a/app/models/project_ci_cd_setting.rb b/app/models/project_ci_cd_setting.rb index cc9003423be687..8d049b8d1b1e54 100644 --- a/app/models/project_ci_cd_setting.rb +++ b/app/models/project_ci_cd_setting.rb @@ -19,6 +19,7 @@ class ProjectCiCdSetting < ApplicationRecord attribute :forward_deployment_enabled, default: true attribute :separated_caches, default: true + validates :merge_trains_skip_train_allowed, inclusion: { in: [true, false] } chronic_duration_attr :runner_token_expiration_interval_human_readable, :runner_token_expiration_interval diff --git a/ee/spec/models/project_ci_cd_setting_spec.rb b/ee/spec/models/project_ci_cd_setting_spec.rb index 68db5d8d8545eb..81d4ce1e7094b2 100644 --- a/ee/spec/models/project_ci_cd_setting_spec.rb +++ b/ee/spec/models/project_ci_cd_setting_spec.rb @@ -9,6 +9,8 @@ stub_feature_flags(disable_merge_trains: false) end + it { is_expected.to validate_inclusion_of(:merge_trains_skip_train_allowed).in_array([true, false]) } + describe '#merge_pipelines_enabled?' do subject { project.merge_pipelines_enabled? } -- GitLab From 28f2029968eff9b60e9bbb07219b0ab297e54ea2 Mon Sep 17 00:00:00 2001 From: drew Date: Mon, 18 Sep 2023 14:02:02 -0400 Subject: [PATCH 8/8] Update migration timestamps --- ...=> 20230918194153_add_merge_immediately_to_ci_cd_settings.rb} | 0 db/schema_migrations/20230816194153 | 1 - db/schema_migrations/20230918194153 | 1 + 3 files changed, 1 insertion(+), 1 deletion(-) rename db/migrate/{20230816194153_add_merge_immediately_to_ci_cd_settings.rb => 20230918194153_add_merge_immediately_to_ci_cd_settings.rb} (100%) delete mode 100644 db/schema_migrations/20230816194153 create mode 100644 db/schema_migrations/20230918194153 diff --git a/db/migrate/20230816194153_add_merge_immediately_to_ci_cd_settings.rb b/db/migrate/20230918194153_add_merge_immediately_to_ci_cd_settings.rb similarity index 100% rename from db/migrate/20230816194153_add_merge_immediately_to_ci_cd_settings.rb rename to db/migrate/20230918194153_add_merge_immediately_to_ci_cd_settings.rb diff --git a/db/schema_migrations/20230816194153 b/db/schema_migrations/20230816194153 deleted file mode 100644 index 3814eab38cfcb2..00000000000000 --- a/db/schema_migrations/20230816194153 +++ /dev/null @@ -1 +0,0 @@ -dac4ef39d622a6a2f1552fcce285828fce97a1cbfe17eacc26b31a5d933e0162 \ No newline at end of file diff --git a/db/schema_migrations/20230918194153 b/db/schema_migrations/20230918194153 new file mode 100644 index 00000000000000..2fd045aa9edf53 --- /dev/null +++ b/db/schema_migrations/20230918194153 @@ -0,0 +1 @@ +1ec3edbe609cd0142790b28c3b55e50ae36be4119f741ce970b36fd8a788a2ce \ No newline at end of file -- GitLab