Commit 67a94b2f authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch '37354-pipelines-update' into 'master'

Make sure head pippeline always corresponds with an MR

Closes #37354

See merge request gitlab-org/gitlab-ce!14358
parents f9229281 b328cff3
......@@ -283,15 +283,15 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
@merge_request.update(merge_error: nil)
if params[:merge_when_pipeline_succeeds].present?
return :failed unless @merge_request.head_pipeline
return :failed unless @merge_request.actual_head_pipeline
if @merge_request.head_pipeline.active?
if @merge_request.actual_head_pipeline.active?
::MergeRequests::MergeWhenPipelineSucceedsService
.new(@project, current_user, merge_params)
.execute(@merge_request)
:merge_when_pipeline_succeeds
elsif @merge_request.head_pipeline.success?
elsif @merge_request.actual_head_pipeline.success?
# This can be triggered when a user clicks the auto merge button while
# the tests finish at about the same time
@merge_request.merge_async(current_user.id, params)
......
......@@ -145,6 +145,13 @@ class MergeRequest < ActiveRecord::Base
'!'
end
# Use this method whenever you need to make sure the head_pipeline is synced with the
# branch head commit, for example checking if a merge request can be merged.
# For more information check: https://gitlab.com/gitlab-org/gitlab-ce/issues/40004
def actual_head_pipeline
head_pipeline&.sha == diff_head_sha ? head_pipeline : nil
end
# Pattern used to extract `!123` merge request references from text
#
# This pattern supports cross-project references.
......@@ -822,8 +829,9 @@ class MergeRequest < ActiveRecord::Base
def mergeable_ci_state?
return true unless project.only_allow_merge_if_pipeline_succeeds?
return true unless head_pipeline
!head_pipeline || head_pipeline.success? || head_pipeline.skipped?
actual_head_pipeline&.success? || actual_head_pipeline&.skipped?
end
def environments_for(current_user)
......@@ -997,7 +1005,7 @@ class MergeRequest < ActiveRecord::Base
return true if autocomplete_precheck
return false unless mergeable?(skip_ci_check: true)
return false if head_pipeline && !(head_pipeline.success? || head_pipeline.active?)
return false if actual_head_pipeline && !(actual_head_pipeline.success? || actual_head_pipeline.active?)
return false if last_diff_sha != diff_head_sha
true
......
......@@ -163,7 +163,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
end
def pipeline
@pipeline ||= head_pipeline
@pipeline ||= actual_head_pipeline
end
def issues_sentence(project, issues)
......
......@@ -33,7 +33,7 @@ class MergeRequestEntity < IssuableEntity
end
expose :merge_commit_message
expose :head_pipeline, with: PipelineDetailsEntity, as: :pipeline
expose :actual_head_pipeline, with: PipelineDetailsEntity, as: :pipeline
# Booleans
expose :merge_ongoing?, as: :merge_ongoing
......
......@@ -29,7 +29,7 @@ module Ci
.new(pipeline, command, SEQUENCE)
sequence.build! do |pipeline, sequence|
update_merge_requests_head_pipeline if pipeline.persisted?
schedule_head_pipeline_update
if sequence.complete?
cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
......@@ -38,15 +38,18 @@ module Ci
pipeline.process!
end
end
pipeline
end
private
def update_merge_requests_head_pipeline
return unless pipeline.latest?
def commit
@commit ||= project.commit(origin_sha || origin_ref)
end
MergeRequest.where(source_project: @pipeline.project, source_branch: @pipeline.ref)
.update_all(head_pipeline_id: @pipeline.id)
def sha
commit.try(:id)
end
def cancel_pending_pipelines
......@@ -69,5 +72,15 @@ module Ci
@pipeline_created_counter ||= Gitlab::Metrics
.counter(:pipelines_created_total, "Counter of pipelines created")
end
def schedule_head_pipeline_update
related_merge_requests.each do |merge_request|
UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id)
end
end
def related_merge_requests
MergeRequest.where(source_project: pipeline.project, source_branch: pipeline.ref)
end
end
end
......@@ -76,6 +76,7 @@ module MergeRequests
end
merge_request.mark_as_unchecked
UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id)
end
end
......
class UpdateHeadPipelineForMergeRequestWorker
include ApplicationWorker
sidekiq_options queue: 'pipeline_default'
def perform(merge_request_id)
merge_request = MergeRequest.find(merge_request_id)
pipeline = Ci::Pipeline.where(project: merge_request.source_project, ref: merge_request.source_branch).last
return unless pipeline && pipeline.latest?
raise ArgumentError, 'merge request sha does not equal pipeline sha' if merge_request.diff_head_sha != pipeline.sha
merge_request.update_attribute(:head_pipeline_id, pipeline.id)
end
end
---
title: Make sure head pippeline always corresponds with the head sha of an MR
merge_request:
author:
type: fixed
......@@ -324,12 +324,12 @@ describe Projects::MergeRequestsController do
end
context 'when the pipeline succeeds is passed' do
def merge_when_pipeline_succeeds
post :merge, base_params.merge(sha: merge_request.diff_head_sha, merge_when_pipeline_succeeds: '1')
let!(:head_pipeline) do
create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request)
end
before do
create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request)
def merge_when_pipeline_succeeds
post :merge, base_params.merge(sha: merge_request.diff_head_sha, merge_when_pipeline_succeeds: '1')
end
it 'returns :merge_when_pipeline_succeeds' do
......@@ -354,6 +354,18 @@ describe Projects::MergeRequestsController do
project.update_column(:only_allow_merge_if_pipeline_succeeds, true)
end
context 'and head pipeline is not the current one' do
before do
head_pipeline.update(sha: 'not_current_sha')
end
it 'returns :failed' do
merge_when_pipeline_succeeds
expect(json_response).to eq('status' => 'failed')
end
end
it 'returns :merge_when_pipeline_succeeds' do
merge_when_pipeline_succeeds
......
......@@ -20,10 +20,14 @@ feature 'Pipelines for Merge Requests', :js do
end
before do
visit project_merge_request_path(project, merge_request)
merge_request.update_attribute(:head_pipeline_id, pipeline.id)
end
scenario 'user visits merge request pipelines tab' do
visit project_merge_request_path(project, merge_request)
expect(page.find('.ci-widget')).to have_content('pending')
page.within('.merge-request-tabs') do
click_link('Pipelines')
end
......@@ -31,6 +35,15 @@ feature 'Pipelines for Merge Requests', :js do
expect(page).to have_selector('.stage-cell')
end
scenario 'pipeline sha does not equal last commit sha' do
pipeline.update_attribute(:sha, '19e2e9b4ef76b422ce1154af39a91323ccc57434')
visit project_merge_request_path(project, merge_request)
wait_for_requests
expect(page.find('.ci-widget')).to have_content(
'Could not connect to the CI server. Please check your settings and try again')
end
end
context 'without pipelines' do
......
......@@ -827,16 +827,19 @@ describe MergeRequest do
end
end
context 'head pipeline' do
before do
allow(subject).to receive(:diff_head_sha).and_return('lastsha')
end
describe '#head_pipeline' do
describe 'when the source project exists' do
it 'returns the latest pipeline' do
pipeline = create(:ci_empty_pipeline, project: subject.source_project, ref: 'master', status: 'running', sha: "123abc", head_pipeline_of: subject)
it 'returns nil for MR without head_pipeline_id' do
subject.update_attribute(:head_pipeline_id, nil)
expect(subject.head_pipeline).to eq(pipeline)
end
expect(subject.head_pipeline).to be_nil
end
describe 'when the source project does not exist' do
context 'when the source project does not exist' do
it 'returns nil' do
allow(subject).to receive(:source_project).and_return(nil)
......@@ -845,6 +848,30 @@ describe MergeRequest do
end
end
describe '#actual_head_pipeline' do
it 'returns nil for MR with old pipeline' do
pipeline = create(:ci_empty_pipeline, sha: 'notlatestsha')
subject.update_attribute(:head_pipeline_id, pipeline.id)
expect(subject.actual_head_pipeline).to be_nil
end
it 'returns the pipeline for MR with recent pipeline' do
pipeline = create(:ci_empty_pipeline, sha: 'lastsha')
subject.update_attribute(:head_pipeline_id, pipeline.id)
expect(subject.actual_head_pipeline).to eq(subject.head_pipeline)
expect(subject.actual_head_pipeline).to eq(pipeline)
end
it 'returns nil when source project does not exist' do
allow(subject).to receive(:source_project).and_return(nil)
expect(subject.actual_head_pipeline).to be_nil
end
end
end
describe '#has_ci?' do
let(:merge_request) { build_stubbed(:merge_request) }
......@@ -1179,7 +1206,7 @@ describe MergeRequest do
context 'when it is only allowed to merge when build is green' do
context 'and a failed pipeline is associated' do
before do
pipeline.update(status: 'failed')
pipeline.update(status: 'failed', sha: subject.diff_head_sha)
allow(subject).to receive(:head_pipeline) { pipeline }
end
......@@ -1188,7 +1215,7 @@ describe MergeRequest do
context 'and a successful pipeline is associated' do
before do
pipeline.update(status: 'success')
pipeline.update(status: 'success', sha: subject.diff_head_sha)
allow(subject).to receive(:head_pipeline) { pipeline }
end
......@@ -1197,7 +1224,7 @@ describe MergeRequest do
context 'and a skipped pipeline is associated' do
before do
pipeline.update(status: 'skipped')
pipeline.update(status: 'skipped', sha: subject.diff_head_sha)
allow(subject).to receive(:head_pipeline) { pipeline }
end
......
......@@ -31,7 +31,7 @@ describe MergeRequestPresenter do
let(:pipeline) { build_stubbed(:ci_pipeline) }
before do
allow(resource).to receive(:head_pipeline).and_return(pipeline)
allow(resource).to receive(:actual_head_pipeline).and_return(pipeline)
end
context 'success with warnings' do
......
......@@ -5,23 +5,35 @@ describe MergeRequestEntity do
let(:resource) { create(:merge_request, source_project: project, target_project: project) }
let(:user) { create(:user) }
let(:request) { double('request', current_user: user) }
let(:request) { double('request', current_user: user, project: project) }
subject do
described_class.new(resource, request: request).as_json
end
it 'includes pipeline' do
req = double('request', current_user: user)
pipeline = build_stubbed(:ci_pipeline)
allow(resource).to receive(:head_pipeline).and_return(pipeline)
describe 'pipeline' do
let(:pipeline) { create(:ci_empty_pipeline, project: project, ref: resource.source_branch, sha: resource.source_branch_sha, head_pipeline_of: resource) }
context 'when is up to date' do
let(:req) { double('request', current_user: user, project: project) }
it 'returns pipeline' do
pipeline_payload = PipelineDetailsEntity
.represent(pipeline, request: req)
.as_json
expect(subject[:pipeline]).to eq(pipeline_payload)
end
end
context 'when is not up to date' do
it 'returns nil' do
pipeline.update(sha: "not up to date")
expect(subject[:pipeline]).to be_nil
end
end
end
it 'includes issues_links' do
issues_links = subject[:issues_links]
......
......@@ -57,20 +57,40 @@ describe Ci::CreatePipelineService do
end
context 'when merge requests already exist for this source branch' do
it 'updates head pipeline of each merge request' do
merge_request_1 = create(:merge_request, source_branch: 'master',
target_branch: "branch_1",
source_project: project)
let(:merge_request_1) do
create(:merge_request, source_branch: 'master', target_branch: "branch_1", source_project: project)
end
let(:merge_request_2) do
create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project)
end
merge_request_2 = create(:merge_request, source_branch: 'master',
target_branch: "branch_2",
source_project: project)
context 'when the head pipeline sha equals merge request sha' do
it 'updates head pipeline of each merge request' do
merge_request_1
merge_request_2
head_pipeline = execute_service
expect(merge_request_1.reload.head_pipeline).to eq(head_pipeline)
expect(merge_request_2.reload.head_pipeline).to eq(head_pipeline)
end
end
context 'when the head pipeline sha does not equal merge request sha' do
it 'raises the ArgumentError error from worker and does not update the head piepeline of MRs' do
merge_request_1
merge_request_2
allow_any_instance_of(Ci::Pipeline).to receive(:latest?).and_return(true)
expect { execute_service(after: 'ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }.to raise_error(ArgumentError)
last_pipeline = Ci::Pipeline.last
expect(merge_request_1.reload.head_pipeline).not_to eq(last_pipeline)
expect(merge_request_2.reload.head_pipeline).not_to eq(last_pipeline)
end
end
context 'when there is no pipeline for source branch' do
it "does not update merge request head pipeline" do
......@@ -106,8 +126,7 @@ describe Ci::CreatePipelineService do
target_branch: "branch_1",
source_project: project)
allow_any_instance_of(Ci::Pipeline)
.to receive(:latest?).and_return(false)
allow_any_instance_of(Ci::Pipeline).to receive(:latest?).and_return(false)
execute_service
......
......@@ -74,6 +74,20 @@ describe MergeRequests::RefreshService do
end
end
context 'when pipeline exists for the source branch' do
let!(:pipeline) { create(:ci_empty_pipeline, ref: @merge_request.source_branch, project: @project, sha: @commits.first.sha)}
subject { service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') }
it 'updates the head_pipeline_id for @merge_request' do
expect { subject }.to change { @merge_request.reload.head_pipeline_id }.from(nil).to(pipeline.id)
end
it 'does not update the head_pipeline_id for @fork_merge_request' do
expect { subject }.not_to change { @fork_merge_request.reload.head_pipeline_id }
end
end
context 'push to origin repo source branch when an MR was reopened' do
let(:refresh_service) { service.new(@project, @user) }
......
require 'spec_helper'
describe UpdateHeadPipelineForMergeRequestWorker do
describe '#perform' do
let(:user) { create(:user) }
let(:project) { create(:project, :repository) }
let(:merge_request) { create(:merge_request, source_project: project) }
let(:latest_sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' }
context 'when pipeline exists for the source project and branch' do
before do
create(:ci_empty_pipeline, project: project, ref: merge_request.source_branch, sha: latest_sha)
end
it 'updates the head_pipeline_id of the merge_request' do
expect { subject.perform(merge_request.id) }.to change { merge_request.reload.head_pipeline_id }
end
context 'when merge request sha does not equal pipeline sha' do
before do
merge_request.merge_request_diff.update(head_commit_sha: 'different_sha')
end
it 'does not update head_pipeline_id' do
expect { subject.perform(merge_request.id) }.to raise_error(ArgumentError)
expect(merge_request.reload.head_pipeline_id).to eq(nil)
end
end
end
context 'when pipeline does not exist for the source project and branch' do
it 'does not update the head_pipeline_id of the merge_request' do
expect { subject.perform(merge_request.id) }.not_to change { merge_request.reload.head_pipeline_id }
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment