From ca7c7dcd9a4d7c6e9bfff5ee98ebfeca1c44341d Mon Sep 17 00:00:00 2001 From: Marc Shaw Date: Mon, 2 Dec 2024 10:49:02 +0100 Subject: [PATCH 1/2] Remove merge_when_checks_pass_merge_train feature flag MR: gitlab.com/gitlab-org/gitlab/-/merge_requests/174357 Changelog: added --- doc/ci/pipelines/merge_trains.md | 1 + ...to_merge_train_when_checks_pass_service.rb | 1 - ...ge_train_when_pipeline_succeeds_service.rb | 9 +- .../merge_trains/add_merge_request_service.rb | 8 +- .../merge_when_checks_pass_merge_train.yml | 9 -- ..._adds_merge_request_to_merge_train_spec.rb | 108 ------------------ ...merge_train_when_pipeline_succeeds_spec.rb | 101 ---------------- .../mutations/merge_requests/accept_spec.rb | 14 --- ...rge_train_when_checks_pass_service_spec.rb | 8 -- ...ain_when_pipeline_succeeds_service_spec.rb | 68 +++++------ .../add_merge_request_service_spec.rb | 22 ---- spec/support/rspec_order_todo.yml | 1 - 12 files changed, 30 insertions(+), 320 deletions(-) delete mode 100644 ee/config/feature_flags/beta/merge_when_checks_pass_merge_train.yml delete mode 100644 ee/spec/features/merge_trains/user_adds_to_merge_train_when_pipeline_succeeds_spec.rb diff --git a/doc/ci/pipelines/merge_trains.md b/doc/ci/pipelines/merge_trains.md index 1d20bbaa12deb5..bb8f412707fb49 100644 --- a/doc/ci/pipelines/merge_trains.md +++ b/doc/ci/pipelines/merge_trains.md @@ -155,6 +155,7 @@ You can also remove (**{close}**) a merge request from the merge train details v > - Auto-merge for merge trains [introduced](https://gitlab.com/groups/gitlab-org/-/epics/10874) in GitLab 17.2 [with a flag](../../administration/feature_flags.md) named `merge_when_checks_pass_merge_train`. Disabled by default. > - Auto-merge for merge trains [enabled](https://gitlab.com/gitlab-org/gitlab/-/issues/470667) on GitLab.com in GitLab 17.2. > - Auto-merge for merge trains [enabled](https://gitlab.com/gitlab-org/gitlab/-/issues/470667) by default in GitLab 17.4. +> - Auto-merge for merge trains [generally available](https://gitlab.com/gitlab-org/gitlab/-/issues/470667) in GitLab 17.7. Feature flag `merge_when_checks_pass_merge_train` removed. FLAG: The availability of this feature is controlled by a feature flag. diff --git a/ee/app/services/auto_merge/add_to_merge_train_when_checks_pass_service.rb b/ee/app/services/auto_merge/add_to_merge_train_when_checks_pass_service.rb index ff77d29526ba02..76f9c24a5c710c 100644 --- a/ee/app/services/auto_merge/add_to_merge_train_when_checks_pass_service.rb +++ b/ee/app/services/auto_merge/add_to_merge_train_when_checks_pass_service.rb @@ -54,7 +54,6 @@ def abort(merge_request, reason) def available_for?(merge_request) super do - next false unless ::Feature.enabled?(:merge_when_checks_pass_merge_train, merge_request.project) next false unless merge_request.has_ci_enabled? next false if merge_request.mergeable? && !merge_request.diff_head_pipeline_considered_in_progress? diff --git a/ee/app/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service.rb b/ee/app/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service.rb index 42944a309fd9dd..6a960c04359c4d 100644 --- a/ee/app/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service.rb +++ b/ee/app/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service.rb @@ -39,13 +39,8 @@ def abort(merge_request, reason) end end - def available_for?(merge_request) - super do - next false if ::Feature.enabled?(:merge_when_checks_pass_merge_train, merge_request.project) - - merge_request.project.merge_trains_enabled? && - merge_request.diff_head_pipeline_considered_in_progress? - end + def available_for?(_merge_request) + false end end end diff --git a/ee/app/services/merge_trains/add_merge_request_service.rb b/ee/app/services/merge_trains/add_merge_request_service.rb index 39829049f05647..b014f487aac168 100644 --- a/ee/app/services/merge_trains/add_merge_request_service.rb +++ b/ee/app/services/merge_trains/add_merge_request_service.rb @@ -16,13 +16,7 @@ def execute strategy = AutoMergeService::STRATEGY_MERGE_TRAIN - if params[:when_pipeline_succeeds] - strategy = if ::Feature.enabled?(:merge_when_checks_pass_merge_train, @merge_request.project) - AutoMergeService::STRATEGY_ADD_TO_MERGE_TRAIN_WHEN_CHECKS_PASS - else - AutoMergeService::STRATEGY_ADD_TO_MERGE_TRAIN_WHEN_PIPELINE_SUCCEEDS - end - end + strategy = AutoMergeService::STRATEGY_ADD_TO_MERGE_TRAIN_WHEN_CHECKS_PASS if params[:when_pipeline_succeeds] response = AutoMergeService.new(@merge_request.target_project, current_user, params.slice(:sha)) .execute(@merge_request, strategy) diff --git a/ee/config/feature_flags/beta/merge_when_checks_pass_merge_train.yml b/ee/config/feature_flags/beta/merge_when_checks_pass_merge_train.yml deleted file mode 100644 index f571a545d87e32..00000000000000 --- a/ee/config/feature_flags/beta/merge_when_checks_pass_merge_train.yml +++ /dev/null @@ -1,9 +0,0 @@ ---- -name: merge_when_checks_pass_merge_train -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/460609 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/155034 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/470667 -milestone: '17.2' -group: group::code review -type: beta -default_enabled: true diff --git a/ee/spec/features/merge_trains/user_adds_merge_request_to_merge_train_spec.rb b/ee/spec/features/merge_trains/user_adds_merge_request_to_merge_train_spec.rb index ee4a436fc67f2f..ffba39d1517179 100644 --- a/ee/spec/features/merge_trains/user_adds_merge_request_to_merge_train_spec.rb +++ b/ee/spec/features/merge_trains/user_adds_merge_request_to_merge_train_spec.rb @@ -88,24 +88,6 @@ expect(page).to have_content("Merged") end - - context 'when merge_when_checks_pass_merge_train is off' do - before do - stub_feature_flags(merge_when_checks_pass_merge_train: false) - end - - it 'displays the expected content', :js, quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/471186' do - expect(page).to have_selector('[data-testid="pipeline-mini-graph-dropdown"]') - - find_by_testid('pipeline-mini-graph-dropdown-toggle').click - page.within '.ci-job-component' do - expect(page).to have_selector('[data-testid="ci-icon"]') - expect(page).not_to have_selector('.retry') - end - - expect(page).to have_content("Merged") - end - end end context "when user clicks 'Remove from merge train' button" do @@ -228,95 +210,5 @@ end end end - - context 'when merge_when_checks_pass_merge_train is off' do - before do - stub_feature_flags(merge_when_checks_pass_merge_train: false) - end - - it "shows 'Merge' button with 'Add to merge train when pipeline succeeds' helper text" do - visit project_merge_request_path(project, merge_request) - - expect(page).to have_button('Merge') - expect(page).to have_content('Add to merge train when pipeline succeeds') - end - - context 'when merge_trains EEP license is not available' do - before do - stub_licensed_features(merge_trains: false) - end - - it "does not show 'Add to merge train when pipeline succeeds' helper text" do - visit project_merge_request_path(project, merge_request) - - expect(page).not_to have_content('Add to merge train when pipeline succeeds') - end - end - - context "when user clicks 'Add to merge train when pipeline succeeds' button" do - before do - visit project_merge_request_path(project, merge_request) - click_button 'Set to auto-merge' - wait_for_requests - end - - it 'shows merge request will be added to merge train when pipeline succeeds' do - page.within('.mr-state-widget') do - expect(page).to have_content("Set by #{user.name} to start a merge train when the pipeline succeeds") - expect(page).to have_content('Source branch will not be deleted.') - expect(page).to have_button('Cancel auto-merge') - end - end - - context 'when pipeline succeeds' do - before do - merge_request.head_pipeline.succeed! - visit project_merge_request_path(project, merge_request) - end - - it 'adds the MR to the merge train but not yet merged' do - expect(page).to have_content("Added to the merge train by #{user.name}") - expect(page).to have_content('Source branch will not be deleted.') - expect(page).to have_button('Remove from merge train') - - expect(page).not_to have_content("Merged") - end - - context 'when the merge train pipeline passes' do - let(:project) { create(:project, :repository) } - let(:user) { project.owner } - - it 'merges the MR' do - merge_request.merge_train_car.pipeline.builds.map(&:success!) - - expect(page).to have_selector('[data-testid="pipeline-mini-graph-dropdown"]') - - find_by_testid('pipeline-mini-graph-dropdown-toggle').click - page.within '.ci-job-component' do - expect(page).to have_selector('[data-testid="ci-icon"]') - expect(page).not_to have_selector('.retry') - end - - expect(page).to have_content("Merged") - end - end - end - - context "when user clicks 'Cancel auto-merge' button" do - before do - click_button 'Cancel auto-merge' - end - - it 'cancels automatic merge' do - wait_for_requests - - page.within('.mr-state-widget') do - expect(page).not_to have_content("Set by #{user.name} to start a merge train when the pipeline succeeds") - expect(page).to have_button('Set to auto-merge') - end - end - end - end - end end end diff --git a/ee/spec/features/merge_trains/user_adds_to_merge_train_when_pipeline_succeeds_spec.rb b/ee/spec/features/merge_trains/user_adds_to_merge_train_when_pipeline_succeeds_spec.rb deleted file mode 100644 index 293bbde08deeb8..00000000000000 --- a/ee/spec/features/merge_trains/user_adds_to_merge_train_when_pipeline_succeeds_spec.rb +++ /dev/null @@ -1,101 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe 'User adds to merge train when pipeline succeeds', :js, feature_category: :merge_trains do - let(:project) { create(:project, :repository) } - let(:user) { create(:user) } - - let!(:merge_request) do - create(:merge_request, :with_merge_request_pipeline, - source_project: project, source_branch: 'feature', - target_project: project, target_branch: 'master') - end - - let(:pipeline) { merge_request.all_pipelines.first } - - before do - stub_feature_flags(merge_when_checks_pass_merge_train: false) - stub_licensed_features(merge_pipelines: true, merge_trains: true) - project.add_maintainer(user) - project.update!(merge_pipelines_enabled: true, merge_trains_enabled: true) - - merge_request.update_head_pipeline - - sign_in(user) - end - - it 'shows Start merge train when pipeline succeeds button and helper icon' do - visit project_merge_request_path(project, merge_request) - - expect(page).to have_button('Set to auto-merge') - expect(page).to have_selector('[data-testid="auto-merge-helper-text"]') - - within_testid('ready_to_merge_state') do - find_by_testid('auto-merge-helper-text-icon').hover - end - - expect(page).to have_link('merge train') - end - - context 'when merge_trains EEP license is not available' do - before do - stub_licensed_features(merge_trains: false) - end - - it 'does not show Start merge train when pipeline succeeds button' do - visit project_merge_request_path(project, merge_request) - - expect(page).to have_button('Set to auto-merge') - expect(page).to have_content('Merge when all merge checks pass') - end - end - - context "when user clicks 'Start merge train when pipeline succeeds' button" do - before do - visit project_merge_request_path(project, merge_request) - click_button 'Set to auto-merge' - - wait_for_requests - end - - it 'informs merge request that auto merge is enabled' do - page.within('.mr-state-widget') do - expect(page).to have_content("Set by #{user.name} to start a merge train when the pipeline succeeds") - expect(page).to have_content('Source branch will not be deleted.') - expect(page).to have_button('Cancel auto-merge') - end - end - - context "when user clicks 'Cancel' button" do - before do - click_button 'Cancel auto-merge' - - wait_for_requests - end - - it 'cancels automatic merge' do - page.within('.mr-state-widget') do - expect(page).not_to have_content("Set by #{user.name} to start a merge train when the pipeline succeeds") - expect(page).to have_button('Set to auto-merge') - expect(page).to have_content('Add to merge train when pipeline succeeds') - end - end - end - end - - context 'when the merge request is not the first queue on the train' do - before do - create(:merge_request, :on_train, - source_project: project, source_branch: 'signed-commits', - target_project: project, target_branch: 'master') - end - - it 'shows Add to merge train when pipeline succeeds button' do - visit project_merge_request_path(project, merge_request) - - expect(page).to have_button('Set to auto-merge') - expect(page).to have_content('Add to merge train when pipeline succeeds') - end - end -end diff --git a/ee/spec/graphql/mutations/merge_requests/accept_spec.rb b/ee/spec/graphql/mutations/merge_requests/accept_spec.rb index b29f45fc49e24b..9ea8b9a7a0f1e9 100644 --- a/ee/spec/graphql/mutations/merge_requests/accept_spec.rb +++ b/ee/spec/graphql/mutations/merge_requests/accept_spec.rb @@ -51,19 +51,5 @@ def mutation_arguments(merge_request) expect(result).not_to include(merge_request: be_merged) expect(result).to include(merge_request: be_auto_merge_enabled) end - - it "can use the ADD_TO_MERGE_TRAIN_WHEN_PIPELINE_SUCCEEDS strategy" do - stub_feature_flags(merge_when_checks_pass_merge_train: false) - enum = ::Types::MergeStrategyEnum.values['ADD_TO_MERGE_TRAIN_WHEN_PIPELINE_SUCCEEDS'] - merge_request = create(:merge_request, :with_head_pipeline, source_project: project) - - args = mutation_arguments(merge_request).merge( - auto_merge_strategy: enum.value - ) - result = mutation.resolve(**args) - - expect(result).not_to include(merge_request: be_merged) - expect(result).to include(merge_request: be_auto_merge_enabled) - end end end diff --git a/ee/spec/services/auto_merge/add_to_merge_train_when_checks_pass_service_spec.rb b/ee/spec/services/auto_merge/add_to_merge_train_when_checks_pass_service_spec.rb index a98315ab6e51ac..81b859fe490a05 100644 --- a/ee/spec/services/auto_merge/add_to_merge_train_when_checks_pass_service_spec.rb +++ b/ee/spec/services/auto_merge/add_to_merge_train_when_checks_pass_service_spec.rb @@ -182,14 +182,6 @@ it { is_expected.to eq(false) } end - context 'when merge_when_checks_pass_merge_train is false' do - before do - stub_feature_flags(merge_when_checks_pass_merge_train: false) - end - - it { is_expected.to eq(false) } - end - context 'when the MR does not have ci enabled' do before do allow(merge_request).to receive(:has_ci_enabled?).and_return(false) diff --git a/ee/spec/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service_spec.rb b/ee/spec/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service_spec.rb index 29c8e72f99dc5d..6004e36e7283c8 100644 --- a/ee/spec/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service_spec.rb +++ b/ee/spec/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service_spec.rb @@ -155,63 +155,47 @@ describe '#available_for?' do subject { service.available_for?(merge_request) } - context 'when merge_when_checks_pass_merge_train is false' do - before do - stub_feature_flags(merge_when_checks_pass_merge_train: false) - end - - it { is_expected.to eq(true) } - - it 'memoizes the result' do - expect(merge_request).to receive(:can_be_merged_by?).once.and_call_original - - 2.times { is_expected.to be_truthy } - end - - context 'when merge trains option is disabled' do - before do - expect(merge_request.project).to receive(:merge_trains_enabled?) { false } - end + it { is_expected.to eq(false) } - it { is_expected.to eq(false) } + context 'when merge trains option is disabled' do + before do + allow(merge_request.project).to receive(:merge_trains_enabled?) { false } end - context 'when the latest pipeline in the merge request is completed' do - before do - pipeline.succeed! - end + it { is_expected.to eq(false) } + end - it { is_expected.to eq(false) } + context 'when the latest pipeline in the merge request is completed' do + before do + pipeline.succeed! end - context 'when merge request is not mergeable' do - before do - merge_request.update!(title: merge_request.draft_title) - end + it { is_expected.to eq(false) } + end - it { is_expected.to eq(false) } + context 'when merge request is not mergeable' do + before do + merge_request.update!(title: merge_request.draft_title) end - context 'when there is an open MR dependency' do - before do - stub_licensed_features(blocking_merge_requests: true) - create(:merge_request_block, blocked_merge_request: merge_request) - end + it { is_expected.to eq(false) } + end - it { is_expected.to be_falsy } + context 'when there is an open MR dependency' do + before do + stub_licensed_features(blocking_merge_requests: true) + create(:merge_request_block, blocked_merge_request: merge_request) end - context 'when the user does not have permission to merge' do - before do - allow(merge_request).to receive(:can_be_merged_by?) { false } - end + it { is_expected.to be_falsy } + end - it { is_expected.to be_falsy } + context 'when the user does not have permission to merge' do + before do + allow(merge_request).to receive(:can_be_merged_by?) { false } end - end - context 'when merge_when_checks_pass_merge_train is true' do - it { is_expected.to eq(false) } + it { is_expected.to be_falsy } end end end diff --git a/ee/spec/services/merge_trains/add_merge_request_service_spec.rb b/ee/spec/services/merge_trains/add_merge_request_service_spec.rb index 81033f8b27b90f..2166e69faa40b1 100644 --- a/ee/spec/services/merge_trains/add_merge_request_service_spec.rb +++ b/ee/spec/services/merge_trains/add_merge_request_service_spec.rb @@ -106,28 +106,6 @@ expect(merge_request.merge_train_car).not_to be_present end - - context 'when merge_when_checks_pass_merge_train is off' do - before do - stub_feature_flags(merge_when_checks_pass_merge_train: false) - end - - context 'when pipeline is not completed' do - let(:pipeline_status) { :running } - - it 'returns success' do - is_expected.to be_success - end - - it 'waits to add to merge train' do - subject - - merge_request.reload - - expect(merge_request.merge_train_car).not_to be_present - end - end - end end end diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index 2befbe35aa0a92..ac5bcec253b648 100644 --- a/spec/support/rspec_order_todo.yml +++ b/spec/support/rspec_order_todo.yml @@ -164,7 +164,6 @@ - './ee/spec/features/merge_request/user_views_blocked_merge_request_spec.rb' - './ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb' - './ee/spec/features/merge_trains/user_adds_merge_request_to_merge_train_spec.rb' -- './ee/spec/features/merge_trains/user_adds_to_merge_train_when_pipeline_succeeds_spec.rb' - './ee/spec/features/milestones/user_views_milestone_spec.rb' - './ee/spec/features/namespace_user_cap_reached_alert_spec.rb' - './ee/spec/features/oncall_schedules/user_creates_schedule_spec.rb' -- GitLab From 88a15296e4aa3c8f66c74870f54e138d65481dce Mon Sep 17 00:00:00 2001 From: Marc Shaw Date: Fri, 6 Dec 2024 10:07:51 +0100 Subject: [PATCH 2/2] Remove additional docs around feature flag --- doc/user/project/merge_requests/auto_merge.md | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/doc/user/project/merge_requests/auto_merge.md b/doc/user/project/merge_requests/auto_merge.md index 202969b7372f8a..7eddfbe7acce6f 100644 --- a/doc/user/project/merge_requests/auto_merge.md +++ b/doc/user/project/merge_requests/auto_merge.md @@ -81,19 +81,6 @@ resolve all existing threads. > - Auto-merge for merge trains [introduced](https://gitlab.com/groups/gitlab-org/-/epics/10874) in GitLab 17.2 [with a flag](../../../administration/feature_flags.md) named `merge_when_checks_pass_merge_train`. Disabled by default. > - Auto-merge for merge trains [enabled](https://gitlab.com/gitlab-org/gitlab/-/issues/470667) on GitLab.com in GitLab 17.2. -FLAG: -The availability of this feature is controlled by a feature flag. -For more information, see the history. - -If your project uses [merge trains](../../../ci/pipelines/merge_trains.md), you can -use the auto-merge feature after you enable these two feature flags: - -- `merge_when_checks_pass` -- `merge_when_checks_pass_merge_train` - -After you enable both feature flags, selecting **Set to auto-merge** on a merge request -adds it to the merge train after all checks pass. - ## Pipeline success for auto-merge If the pipeline succeeds, the merge request merges. If the pipeline fails, the author -- GitLab