From 9da3ae70923fdf35e4850676cc1d238e5d63ea16 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Sun, 20 Jan 2019 11:41:11 +0900 Subject: Don't create merge request pipeline without commits --- app/models/ci/pipeline.rb | 7 +++++++ .../unreleased/not-run-pipeline-on-empty-merge-request.yml | 5 +++++ spec/models/ci/pipeline_spec.rb | 13 ++++++++++--- spec/models/merge_request_spec.rb | 2 +- spec/services/ci/create_pipeline_service_spec.rb | 9 +++++++++ spec/services/merge_requests/refresh_service_spec.rb | 8 ++++++-- 6 files changed, 38 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/not-run-pipeline-on-empty-merge-request.yml diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 30a957b4117..09fcdc03577 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -54,6 +54,7 @@ module Ci validates :ref, presence: { unless: :importing? } validates :merge_request, presence: { if: :merge_request? } validates :merge_request, absence: { unless: :merge_request? } + validate :presence_of_commits_in_merge_request, if: :merge_request? validates :tag, inclusion: { in: [false], if: :merge_request? } validates :status, presence: { unless: :importing? } validate :valid_commit_sha, unless: :importing? @@ -345,6 +346,12 @@ module Ci end end + def presence_of_commits_in_merge_request + if merge_request&.has_no_commits? + errors.add(:merge_request, "must have commits") + end + end + def git_author_name strong_memoize(:git_author_name) do commit.try(:author_name) diff --git a/changelogs/unreleased/not-run-pipeline-on-empty-merge-request.yml b/changelogs/unreleased/not-run-pipeline-on-empty-merge-request.yml new file mode 100644 index 00000000000..732e4baf4e9 --- /dev/null +++ b/changelogs/unreleased/not-run-pipeline-on-empty-merge-request.yml @@ -0,0 +1,5 @@ +--- +title: Don't create new merge request pipeline without commits +merge_request: 24503 +author: Hiroyuki Sato +type: added diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 17f33785fda..2869022f2f6 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Ci::Pipeline, :mailer do let(:user) { create(:user) } - set(:project) { create(:project) } + set(:project) { create(:project, :repository) } let(:pipeline) do create(:ci_empty_pipeline, status: :created, project: project) @@ -131,12 +131,18 @@ describe Ci::Pipeline, :mailer do context 'when source is merge request' do let(:source) { :merge_request } - context 'when merge request is specified' do + context 'when merge request with commits is specified' do let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'feature', target_project: project, target_branch: 'master') } it { expect(pipeline).to be_valid } end + context 'when merge request without commits is specified' do + let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'not-merged-branch', target_project: project, target_branch: 'master') } + + it { expect(pipeline).not_to be_valid } + end + context 'when merge request is empty' do let(:merge_request) { nil } @@ -1036,6 +1042,8 @@ describe Ci::Pipeline, :mailer do end context 'when repository does not exist' do + let(:project) { create(:project) } + let(:pipeline) do create(:ci_empty_pipeline, project: project, ref: 'master') end @@ -2071,7 +2079,6 @@ describe Ci::Pipeline, :mailer do end describe "#all_merge_requests" do - let(:project) { create(:project) } let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master') } it "returns all merge requests having the same source branch" do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index bfc9035cb56..c9643b34fa0 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1378,7 +1378,7 @@ describe MergeRequest do source_project: project, source_branch: source_ref, target_project: project, - target_branch: 'stable') + target_branch: 'merge-test') end before do diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 8497e90bd8b..be6bdc21b81 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -736,6 +736,15 @@ describe Ci::CreatePipelineService do end end + context 'when there are no commits between source branch and target branch' do + let(:ref_name) { 'refs/heads/not-merged-branch' } + + it 'does not create a merge request pipeline' do + expect(pipeline).not_to be_persisted + expect(pipeline.errors[:merge_request]).to eq(["must have commits"]) + end + end + context 'when merge request is created from a forked project' do let(:merge_request) do create(:merge_request, diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 1169ed5f9f2..57b8e71c2d5 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -150,11 +150,15 @@ describe MergeRequests::RefreshService do } end - it 'create merge request pipeline' do + it 'create merge request pipeline with commits' do expect { subject } .to change { @merge_request.merge_request_pipelines.count }.by(1) .and change { @fork_merge_request.merge_request_pipelines.count }.by(1) - .and change { @another_merge_request.merge_request_pipelines.count }.by(1) + .and change { @another_merge_request.merge_request_pipelines.count }.by(0) + + expect(@merge_request.has_commits?).to be true + expect(@fork_merge_request.has_commits?).to be true + expect(@another_merge_request.has_commits?).to be false end context "when branch pipeline was created before a merge request pipline has been created" do -- cgit v1.2.1