diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 3e838f2ca3d7576889f9fc0d907bcfafb1951715..9d792297ae433f34cd110871f643aa6d1baad637 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -45,11 +45,6 @@ def refresh_merge_requests! notify_about_push(mr) mark_mr_as_draft_from_commits(mr) - if Feature.disabled?(:split_refresh_worker_pipeline, @current_user) && !@push.branch_removed? - # Run at the end of the loop to avoid any potential contention on the MR object - refresh_pipelines_on_merge_requests(mr) - end - merge_request_activity_counter.track_mr_including_ci_config(user: mr.author, merge_request: mr) end @@ -364,16 +359,14 @@ def execute_async_workers @push.ref ) - if Feature.enabled?(:split_refresh_worker_pipeline, @current_user) - MergeRequests::Refresh::PipelineWorker.perform_async( - @project.id, - @current_user.id, - @push.oldrev, - @push.newrev, - @push.ref, - params.slice(:push_options, :gitaly_context).as_json # ensure sidekiq-compatible hash argument - ) - end + MergeRequests::Refresh::PipelineWorker.perform_async( + @project.id, + @current_user.id, + @push.oldrev, + @push.newrev, + @push.ref, + params.slice(:push_options, :gitaly_context).as_json # ensure sidekiq-compatible hash argument + ) end end end diff --git a/config/feature_flags/beta/split_refresh_worker_pipeline.yml b/config/feature_flags/beta/split_refresh_worker_pipeline.yml deleted file mode 100644 index 6cf2f83374543c995bfbde8b3bcc6eb10ce4be8b..0000000000000000000000000000000000000000 --- a/config/feature_flags/beta/split_refresh_worker_pipeline.yml +++ /dev/null @@ -1,10 +0,0 @@ ---- -name: split_refresh_worker_pipeline -description: Split out the refreshing of pipelines from the refresh MR service -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/570495 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/203431 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/570746 -milestone: '18.5' -group: group::code review -type: beta -default_enabled: true diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 4d06bd53648486566c96611a1e716781fdf706cc..c20151db072bdbd958ab98c4696721e92b71c702 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -10,14 +10,8 @@ let(:user) { create(:user) } let(:service) { described_class } - let(:pipeline_ff) { false } - describe '#execute' do before do - stub_feature_flags( - split_refresh_worker_pipeline: pipeline_ff - ) - @user = create(:user) group = create(:group) group.add_owner(@user) @@ -143,8 +137,8 @@ end it 'triggers mergeRequestMergeStatusUpdated GraphQL subscription conditionally' do - expect(GraphqlTriggers).to receive(:merge_request_merge_status_updated).twice.with(@merge_request) - expect(GraphqlTriggers).to receive(:merge_request_merge_status_updated).twice.with(@another_merge_request) + expect(GraphqlTriggers).to receive(:merge_request_merge_status_updated).with(@merge_request) + expect(GraphqlTriggers).to receive(:merge_request_merge_status_updated).with(@another_merge_request) expect(GraphqlTriggers).not_to receive(:merge_request_merge_status_updated).with(@fork_merge_request) refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') @@ -275,224 +269,26 @@ subject { service.new(project: project, current_user: @user, params: { push_options: {}, gitaly_context: { nested: "value" } }).execute(@oldrev, @newrev, ref) } - context 'when split_refresh_worker_pipeline ff is true' do - let(:pipeline_ff) { true } - - it 'calls the pipeline worker async, forwarding properly formatted push_options and gitaly_context' do - expect(MergeRequests::Refresh::PipelineWorker).to receive(:perform_async) - .with( - @project.id, - @user.id, - @oldrev, - @newrev, - 'refs/heads/master', - { - "push_options" => {}, - "gitaly_context" => { "nested" => "value" } - } - ) - - subject - end + it 'calls the pipeline worker async, forwarding properly formatted push_options and gitaly_context' do + expect(MergeRequests::Refresh::PipelineWorker).to receive(:perform_async) + .with( + @project.id, + @user.id, + @oldrev, + @newrev, + 'refs/heads/master', + { + "push_options" => {}, + "gitaly_context" => { "nested" => "value" } + } + ) - it 'does not execute create the pipelines' do - expect { subject } - .not_to change { @merge_request.pipelines_for_merge_request.count + @another_merge_request.pipelines_for_merge_request.count } - end + subject end - context 'when split_refresh_worker_pipeline ff is false', :sidekiq_inline do - context "when .gitlab-ci.yml has merge_requests keywords" do - it 'create detached merge request pipeline with commits' do - expect { subject } - .to change { @merge_request.pipelines_for_merge_request.count }.by(1) - .and not_change { @another_merge_request.pipelines_for_merge_request.count } - - expect(@merge_request.has_commits?).to be_truthy - expect(@another_merge_request.has_commits?).to be_falsy - end - - context 'when push is a branch removal' do - before do - # If @newrev is a blank SHA, it means the ref has been removed - @newrev = Gitlab::Git::SHA1_BLANK_SHA - end - - it 'does not create detached merge request pipeline' do - expect { subject } - .not_to change { @merge_request.pipelines_for_merge_request.count } - end - end - - context 'when "push_options: nil" is passed' do - let(:service_instance) { service.new(project: project, current_user: @user, params: { push_options: nil }) } - - subject { service_instance.execute(@oldrev, @newrev, ref) } - - it 'creates a detached merge request pipeline with commits' do - expect { subject } - .to change { @merge_request.pipelines_for_merge_request.count }.by(1) - .and not_change { @another_merge_request.pipelines_for_merge_request.count } - - expect(@merge_request.has_commits?).to be_truthy - expect(@another_merge_request.has_commits?).to be_falsy - end - end - - context 'when ci.skip push_options are passed' do - let(:params) { { push_options: { ci: { skip: true } } } } - let(:service_instance) { service.new(project: project, current_user: @user, params: params) } - - subject { service_instance.execute(@oldrev, @newrev, ref) } - - it 'creates a skipped detached merge request pipeline with commits' do - expect { subject } - .to change { @merge_request.pipelines_for_merge_request.count }.by(1) - .and not_change { @another_merge_request.pipelines_for_merge_request.count } - - expect(@merge_request.has_commits?).to be_truthy - expect(@another_merge_request.has_commits?).to be_falsy - - pipeline = @merge_request.pipelines_for_merge_request.last - expect(pipeline).to be_skipped - end - end - - it 'does not create detached merge request pipeline for forked project' do - expect { subject } - .not_to change { @fork_merge_request.pipelines_for_merge_request.count } - end - - it 'create detached merge request pipeline for non-fork merge request' do - subject - - expect(@merge_request.pipelines_for_merge_request.first) - .to be_detached_merge_request_pipeline - end - - context 'when service is hooked by target branch' do - let(:ref) { 'refs/heads/feature' } - - it 'does not create detached merge request pipeline' do - expect { subject } - .not_to change { @merge_request.pipelines_for_merge_request.count } - end - end - - context 'when service runs on forked project' do - let(:project) { @fork_project } - - it 'creates detached merge request pipeline for fork merge request' do - expect { subject } - .to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1) - - merge_request_pipeline = @fork_merge_request.pipelines_for_merge_request.first - expect(merge_request_pipeline).to be_detached_merge_request_pipeline - expect(merge_request_pipeline.project).to eq(@project) - end - end - - context "when branch pipeline was created before a detaced merge request pipeline has been created" do - before do - create( - :ci_pipeline, - project: @merge_request.source_project, - sha: @merge_request.diff_head_sha, - ref: @merge_request.source_branch, - tag: false - ) - - subject - end - - it 'sets the latest detached merge request pipeline as a head pipeline' do - @merge_request.reload - expect(@merge_request.diff_head_pipeline).to be_merge_request_event - end - - it 'returns pipelines in correct order' do - @merge_request.reload - expect(@merge_request.all_pipelines.first).to be_merge_request_event - expect(@merge_request.all_pipelines.second).to be_push - end - end - - context "when MergeRequestUpdateWorker is retried by an exception" do - it 'does not re-create a duplicate detached merge request pipeline' do - expect do - service.new(project: @project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/master') - end.to change { @merge_request.pipelines_for_merge_request.count }.by(1) - - expect do - service.new(project: @project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/master') - end.not_to change { @merge_request.pipelines_for_merge_request.count } - end - end - - context 'when the pipeline should be skipped' do - it 'saves a skipped detached merge request pipeline' do - project.repository.create_file( - @user, 'new-file.txt', 'A new file', - message: '[skip ci] This is a test', - branch_name: 'master' - ) - - expect { subject } - .to change { @merge_request.pipelines_for_merge_request.count }.by(1) - expect(@merge_request.pipelines_for_merge_request.last).to be_skipped - end - end - end - - context "when .gitlab-ci.yml does not have merge_requests keywords" do - let(:config) do - YAML.dump({ - test: { - stage: 'test', - script: 'echo' - } - }) - end - - it 'does not create a detached merge request pipeline' do - expect { subject } - .not_to change { @merge_request.pipelines_for_merge_request.count } - end - end - - context 'when .gitlab-ci.yml is invalid' do - let(:config) { 'invalid yaml file' } - - it 'persists a pipeline with config error' do - expect { subject } - .to change { @merge_request.pipelines_for_merge_request.count }.by(1) - expect(@merge_request.pipelines_for_merge_request.last).to be_failed - expect(@merge_request.pipelines_for_merge_request.last).to be_config_error - end - end - - context 'when .gitlab-ci.yml file is valid but has a logical error' do - let(:config) do - YAML.dump({ - build: { - script: 'echo "Valid yaml syntax, but..."', - only: ['master'] - }, - test: { - script: 'echo "... I depend on build, which does not run."', - only: ['merge_request'], - needs: ['build'] - } - }) - end - - it 'persists a pipeline with config error' do - expect { subject } - .to change { @merge_request.pipelines_for_merge_request.count }.by(1) - expect(@merge_request.pipelines_for_merge_request.last).to be_failed - expect(@merge_request.pipelines_for_merge_request.last).to be_config_error - end - end + it 'does not create the pipelines inline' do + expect { subject } + .not_to change { @merge_request.pipelines_for_merge_request.count + @another_merge_request.pipelines_for_merge_request.count } end end