Merge branch 'fix-inconsistent-behavior-of-has-ref' into 'master'

Fix inconsistent `branch?` behavior between Ci::Pipeline and Ci::Build

See merge request gitlab-org/gitlab-ce!25629

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