From adf003a9312fddfb5438775c3cf24b052c616b4f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 28 Feb 2019 18:15:32 +0900 Subject: [PATCH] Fix inconsistent `branch?` behavior between Ci::Pipeline and Ci::Build Add spec Add more tests ok --- app/models/ci/pipeline.rb | 4 - app/models/concerns/has_ref.rb | 2 +- spec/factories/merge_requests.rb | 9 +++ spec/models/ci/build_spec.rb | 116 +++++++++++++++++++++++++++ spec/models/concerns/has_ref_spec.rb | 20 +++++ 5 files changed, 146 insertions(+), 5 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index d4586219333..ca941ca4331 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -417,10 +417,6 @@ module Ci @commit ||= Commit.lazy(project, sha) end - def branch? - super && !merge_request? - end - def stuck? pending_builds.any?(&:stuck?) end diff --git a/app/models/concerns/has_ref.rb b/app/models/concerns/has_ref.rb index d7089294efc..44b06b4890c 100644 --- a/app/models/concerns/has_ref.rb +++ b/app/models/concerns/has_ref.rb @@ -4,7 +4,7 @@ module HasRef extend ActiveSupport::Concern def branch? - !tag? + !tag? && !merge_request? end def git_ref diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index 2392bfc4a53..0c0c5fc8a4c 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -101,6 +101,15 @@ FactoryBot.define do end end + trait :with_merge_request_pipeline do + after(:build) do |merge_request| + merge_request.merge_request_pipelines << build(:ci_pipeline, + source: :merge_request, + merge_request: merge_request, + project: merge_request.source_project) + end + end + trait :deployed_review_app do target_branch 'pages-deploy-target' diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 81ff727b458..e5281f3a09e 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2734,6 +2734,122 @@ describe Ci::Build do end end + describe '#secret_group_variables' do + subject { build.secret_group_variables } + + let!(:variable) { create(:ci_group_variable, protected: true, group: group) } + + context 'when ref is branch' do + let(:build) { create(:ci_build, ref: 'master', tag: false, project: project) } + + context 'when ref is protected' do + before do + create(:protected_branch, :developers_can_merge, name: 'master', project: project) + end + + it { is_expected.to include(variable) } + end + + context 'when ref is not protected' do + it { is_expected.not_to include(variable) } + end + end + + context 'when ref is tag' do + let(:build) { create(:ci_build, ref: 'v1.1.0', tag: true, project: project) } + + context 'when ref is protected' do + before do + create(:protected_tag, project: project, name: 'v*') + end + + it { is_expected.to include(variable) } + end + + context 'when ref is not protected' do + it { is_expected.not_to include(variable) } + end + end + + context 'when ref is merge request' do + let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) } + let(:pipeline) { merge_request.merge_request_pipelines.first } + let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) } + + context 'when ref is protected' do + before do + create(:protected_branch, :developers_can_merge, name: merge_request.source_branch, project: project) + end + + it 'does not return protected variables as it is not supported for merge request pipelines' do + is_expected.not_to include(variable) + end + end + + context 'when ref is not protected' do + it { is_expected.not_to include(variable) } + end + end + end + + describe '#secret_project_variables' do + subject { build.secret_project_variables } + + let!(:variable) { create(:ci_variable, protected: true, project: project) } + + context 'when ref is branch' do + let(:build) { create(:ci_build, ref: 'master', tag: false, project: project) } + + context 'when ref is protected' do + before do + create(:protected_branch, :developers_can_merge, name: 'master', project: project) + end + + it { is_expected.to include(variable) } + end + + context 'when ref is not protected' do + it { is_expected.not_to include(variable) } + end + end + + context 'when ref is tag' do + let(:build) { create(:ci_build, ref: 'v1.1.0', tag: true, project: project) } + + context 'when ref is protected' do + before do + create(:protected_tag, project: project, name: 'v*') + end + + it { is_expected.to include(variable) } + end + + context 'when ref is not protected' do + it { is_expected.not_to include(variable) } + end + end + + context 'when ref is merge request' do + let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) } + let(:pipeline) { merge_request.merge_request_pipelines.first } + let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) } + + context 'when ref is protected' do + before do + create(:protected_branch, :developers_can_merge, name: merge_request.source_branch, project: project) + end + + it 'does not return protected variables as it is not supported for merge request pipelines' do + is_expected.not_to include(variable) + end + end + + context 'when ref is not protected' do + it { is_expected.not_to include(variable) } + end + end + end + describe '#scoped_variables_hash' do context 'when overriding CI variables' do before do diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb index 8aed72d77a4..8aa2fecb18c 100644 --- a/spec/models/concerns/has_ref_spec.rb +++ b/spec/models/concerns/has_ref_spec.rb @@ -16,6 +16,16 @@ describe HasRef do it 'return true when tag is set to false' do is_expected.to be_truthy end + + context 'when it was triggered by merge request' do + let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) } + let(:pipeline) { merge_request.merge_request_pipelines.first } + let(:build) { create(:ci_build, pipeline: pipeline) } + + it 'returns false' do + is_expected.to be_falsy + end + end end context 'is not a tag' do @@ -55,5 +65,15 @@ describe HasRef do is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) end end + + context 'when it is triggered by a merge request' do + let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) } + let(:pipeline) { merge_request.merge_request_pipelines.first } + let(:build) { create(:ci_build, tag: false, pipeline: pipeline) } + + it 'returns nil' do + is_expected.to be_nil + end + end end end