From 6a4c87d6db68e2536b90b38c1e11f9a158bd7480 Mon Sep 17 00:00:00 2001
From: Hordur Freyr Yngvason <hfyngvason@gitlab.com>
Date: Tue, 26 Mar 2024 21:22:07 +0000
Subject: [PATCH] Fix commit status API incorrectly overwriting MR head
pipeline
The commit status API would sometimes incorrectly overwrite the merge
request head pipeline with a branch pipeline when a merge request
pipeline also existed for the same commit.
This was due to a duplicate and incorrect implementation of the required
logic. The solution is to instead fire a `Ci::PipelineCreatedEvent` _if_
an `external` pipeline is created, as this is the canonical way to
trigger the `head_pipeline_id` update. There is no need to update the
`head_pipeline_id` for pre-existing pipelines, as these would already
have fired the required events.
See https://gitlab.com/gitlab-org/gitlab/-/issues/450176
Changelog: fixed
---
.../ci/create_commit_status_service.rb | 13 +++-----
spec/requests/api/commit_statuses_spec.rb | 6 ++--
.../ci/create_commit_status_service_spec.rb | 32 +++++++++++++++++--
3 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/app/services/ci/create_commit_status_service.rb b/app/services/ci/create_commit_status_service.rb
index de3e7b3f7ffae..24a9130e7acf0 100644
--- a/app/services/ci/create_commit_status_service.rb
+++ b/app/services/ci/create_commit_status_service.rb
@@ -36,7 +36,6 @@ def unsafe_execute
return bad_request(response.message) if response.error?
- update_merge_request_head_pipeline
response
end
@@ -73,6 +72,10 @@ def create_pipeline
).tap do |new_pipeline|
new_pipeline.ensure_project_iid!
new_pipeline.save!
+
+ Gitlab::EventStore.publish(
+ Ci::PipelineCreatedEvent.new(data: { pipeline_id: new_pipeline.id })
+ )
end
end
@@ -106,14 +109,6 @@ def add_or_update_external_job
end
end
- def update_merge_request_head_pipeline
- return unless pipeline.latest?
-
- ::MergeRequest
- .from_project(project).from_source_branches(ref)
- .update_all(head_pipeline_id: pipeline.id)
- end
-
def apply_job_state!(job)
case params[:state]
when 'pending'
diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb
index 97e4f8f16b450..24dc414710dc3 100644
--- a/spec/requests/api/commit_statuses_spec.rb
+++ b/spec/requests/api/commit_statuses_spec.rb
@@ -293,9 +293,11 @@ def create_status(commit, opts = {})
end
context 'when merge request exists for given branch' do
- let!(:merge_request) { create(:merge_request, source_project: project, source_branch: 'master', target_branch: 'develop') }
+ let!(:merge_request) do
+ create(:merge_request, source_project: project, head_pipeline_id: nil)
+ end
- it 'sets head pipeline' do
+ it 'sets head pipeline', :sidekiq_inline do
subject
expect(response).to have_gitlab_http_status(:created)
diff --git a/spec/services/ci/create_commit_status_service_spec.rb b/spec/services/ci/create_commit_status_service_spec.rb
index cbdb681afe31b..67989347d4e0d 100644
--- a/spec/services/ci/create_commit_status_service_spec.rb
+++ b/spec/services/ci/create_commit_status_service_spec.rb
@@ -112,10 +112,10 @@
context 'when merge request exists for given branch' do
let!(:merge_request) do
- create(:merge_request, source_project: project, source_branch: 'master', target_branch: 'develop')
+ create(:merge_request, source_project: project, head_pipeline: nil)
end
- it 'sets head pipeline' do
+ it 'sets head pipeline', :sidekiq_inline do
expect { response }
.to change { ::Ci::Pipeline.count }.by(1)
.and change { ::Ci::Stage.count }.by(1)
@@ -124,6 +124,34 @@
expect(response).to be_success
expect(merge_request.reload.head_pipeline).not_to be_nil
end
+
+ context 'when the MR has a branch head pipeline' do
+ let!(:merge_request) do
+ create(:merge_request, :with_head_pipeline, source_project: project)
+ end
+
+ it 'adds the status to the existing pipeline' do
+ expect { response }.not_to change { ::Ci::Pipeline.count }
+ expect(response.payload[:job].pipeline_id).to eq(merge_request.head_pipeline_id)
+ end
+ end
+
+ context 'when the MR has a merged result head pipeline' do
+ let!(:merge_request) do
+ create(:merge_request, source_project: project, head_pipeline: head_pipeline)
+ end
+
+ let(:head_pipeline) { create(:ci_pipeline, :merged_result_pipeline) }
+
+ it 'creates a new branch pipeline but does not change the head pipeline' do
+ expect { response }
+ .to change { ::Ci::Pipeline.count }.by(1)
+ .and change { ::Ci::Stage.count }.by(1)
+ .and change { ::CommitStatus.count }.by(1)
+
+ expect(merge_request.reload.head_pipeline_id).to eq(head_pipeline.id)
+ end
+ end
end
end
--
GitLab