Merge branch 'fix-ci-commit-ref-name-and-slug' into 'master'

Make `CI_COMMIT_REF_NAME` and `SLUG` variable idempotent

Closes #60822

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

app/models/ci/bridge.rb

@@ -4,6 +4,7 @@ module Ci
   class Bridge < CommitStatus
     include Ci::Processable
     include Ci::Contextable
+    include Ci::PipelineDelegator
     include Importable
     include AfterCommitQueue
     include HasRef
@@ -13,8 +14,6 @@ module Ci
     belongs_to :trigger_request
     validates :ref, presence: true
 
-    delegate :merge_request_event?, to: :pipeline
-
     def self.retry(bridge, current_user)
       raise NotImplementedError
     end

app/models/ci/build.rb

@@ -6,6 +6,7 @@ module Ci
     include Ci::Processable
     include Ci::Metadatable
     include Ci::Contextable
+    include Ci::PipelineDelegator
     include TokenAuthenticatable
     include AfterCommitQueue
     include ObjectStorage::BackgroundMove
@@ -49,8 +50,6 @@ module Ci
     delegate :terminal_specification, to: :runner_session, allow_nil: true
     delegate :gitlab_deploy_token, to: :project
     delegate :trigger_short_token, to: :trigger_request, allow_nil: true
-    delegate :merge_request_event?, :merge_request_ref?,
-             :legacy_detached_merge_request_pipeline?, to: :pipeline
 
     ##
     # Since Gitlab 11.5, deployments records started being created right after

app/models/ci/pipeline.rb

@@ -759,6 +759,18 @@ module Ci
       user == current_user
     end
 
+    def source_ref
+      if triggered_by_merge_request?
+        merge_request.source_branch
+      else
+        ref
+      end
+    end
+
+    def source_ref_slug
+      Gitlab::Utils.slugify(source_ref.to_s)
+    end
+
     private
 
     def ci_yaml_from_repo

app/models/concerns/ci/contextable.rb

@@ -70,8 +70,8 @@ module Ci
         variables.append(key: 'CI_COMMIT_SHA', value: sha)
         variables.append(key: 'CI_COMMIT_SHORT_SHA', value: short_sha)
         variables.append(key: 'CI_COMMIT_BEFORE_SHA', value: before_sha)
-        variables.append(key: 'CI_COMMIT_REF_NAME', value: ref)
-        variables.append(key: 'CI_COMMIT_REF_SLUG', value: ref_slug)
+        variables.append(key: 'CI_COMMIT_REF_NAME', value: source_ref)
+        variables.append(key: 'CI_COMMIT_REF_SLUG', value: source_ref_slug)
         variables.append(key: "CI_COMMIT_TAG", value: ref) if tag?
         variables.append(key: "CI_PIPELINE_TRIGGERED", value: 'true') if trigger_request
         variables.append(key: "CI_JOB_MANUAL", value: 'true') if action?
@@ -85,8 +85,8 @@ module Ci
       Gitlab::Ci::Variables::Collection.new.tap do |variables|
         variables.append(key: 'CI_BUILD_REF', value: sha)
         variables.append(key: 'CI_BUILD_BEFORE_SHA', value: before_sha)
-        variables.append(key: 'CI_BUILD_REF_NAME', value: ref)
-        variables.append(key: 'CI_BUILD_REF_SLUG', value: ref_slug)
+        variables.append(key: 'CI_BUILD_REF_NAME', value: source_ref)
+        variables.append(key: 'CI_BUILD_REF_SLUG', value: source_ref_slug)
         variables.append(key: 'CI_BUILD_NAME', value: name)
         variables.append(key: 'CI_BUILD_STAGE', value: stage)
         variables.append(key: "CI_BUILD_TAG", value: ref) if tag?

app/models/concerns/ci/pipeline_delegator.rb

+# frozen_string_literal: true
+
+##
+# This module is mainly used by child associations of `Ci::Pipeline` that needs to look up
+# single source of truth. For example, `Ci::Build` has `git_ref` method, which behaves
+# slightly different from `Ci::Pipeline`'s `git_ref`. This is very confusing as
+# the system could behave differently time to time.
+# We should have a single interface in `Ci::Pipeline` and access the method always.
+module Ci
+  module PipelineDelegator
+    extend ActiveSupport::Concern
+
+    included do
+      delegate :merge_request_event?,
+               :merge_request_ref?,
+               :source_ref,
+               :source_ref_slug,
+               :legacy_detached_merge_request_pipeline?, to: :pipeline
+    end
+  end
+end

app/models/concerns/has_ref.rb

 # frozen_string_literal: true
 
+##
+# We will disable `ref` and `sha` attributes in `Ci::Build` in the future
+# and remove this module in favor of Ci::PipelineDelegator.
 module HasRef
   extend ActiveSupport::Concern
 

changelogs/unreleased/fix-ci-commit-ref-name-and-slug.yml

+---
+title: Make `CI_COMMIT_REF_NAME` and `SLUG` variable idempotent
+merge_request: 27663
+author:
+type: fixed

spec/models/ci/bridge_spec.rb

@@ -10,6 +10,8 @@ describe Ci::Bridge do
     create(:ci_bridge, pipeline: pipeline)
   end
 
+  it { is_expected.to include_module(Ci::PipelineDelegator) }
+
   describe '#tags' do
     it 'only has a bridge tag' do
       expect(bridge.tags).to eq [:bridge]

spec/models/ci/build_spec.rb

@@ -28,6 +28,7 @@ describe Ci::Build do
   it { is_expected.to delegate_method(:merge_request_event?).to(:pipeline) }
   it { is_expected.to delegate_method(:merge_request_ref?).to(:pipeline) }
   it { is_expected.to delegate_method(:legacy_detached_merge_request_pipeline?).to(:pipeline) }
+  it { is_expected.to include_module(Ci::PipelineDelegator) }
 
   it { is_expected.to be_a(ArtifactMigratable) }
 
@@ -2273,6 +2274,19 @@ describe Ci::Build do
       it { user_variables.each { |v| is_expected.to include(v) } }
     end
 
+    context 'when build belongs to a pipeline for merge request' do
+      let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline, source_branch: 'improve/awesome') }
+      let(:pipeline) { merge_request.all_pipelines.first }
+      let(:build) { create(:ci_build, ref: pipeline.ref, pipeline: pipeline) }
+
+      it 'returns values based on source ref' do
+        is_expected.to include(
+          { key: 'CI_COMMIT_REF_NAME', value: 'improve/awesome', public: true, masked: false },
+          { key: 'CI_COMMIT_REF_SLUG', value: 'improve-awesome', public: true, masked: false }
+        )
+      end
+    end
+
     context 'when build has an environment' do
       let(:environment_variables) do
         [
@@ -2664,6 +2678,8 @@ describe Ci::Build do
         )
       end
 
+      let(:pipeline) { create(:ci_pipeline, project: project, ref: 'feature') }
+
       it 'returns static predefined variables' do
         expect(build.variables.size).to be >= 28
         expect(build.variables)
@@ -2713,6 +2729,8 @@ describe Ci::Build do
         )
       end
 
+      let(:pipeline) { create(:ci_pipeline, project: project, ref: 'feature') }
+
       it 'does not persist the build' do
         expect(build).to be_valid
         expect(build).not_to be_persisted

spec/models/ci/pipeline_spec.rb

@@ -382,6 +382,54 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
+  describe '#source_ref' do
+    subject { pipeline.source_ref }
+
+    let(:pipeline) { create(:ci_pipeline, ref: 'feature') }
+
+    it 'returns source ref' do
+      is_expected.to eq('feature')
+    end
+
+    context 'when the pipeline is a detached merge request pipeline' do
+      let(:merge_request) { create(:merge_request) }
+
+      let(:pipeline) do
+        create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, ref: merge_request.ref_path)
+      end
+
+      it 'returns source ref' do
+        is_expected.to eq(merge_request.source_branch)
+      end
+    end
+  end
+
+  describe '#source_ref_slug' do
+    subject { pipeline.source_ref_slug }
+
+    let(:pipeline) { create(:ci_pipeline, ref: 'feature') }
+
+    it 'slugifies with the source ref' do
+      expect(Gitlab::Utils).to receive(:slugify).with('feature')
+
+      subject
+    end
+
+    context 'when the pipeline is a detached merge request pipeline' do
+      let(:merge_request) { create(:merge_request) }
+
+      let(:pipeline) do
+        create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, ref: merge_request.ref_path)
+      end
+
+      it 'slugifies with the source ref of the merge request' do
+        expect(Gitlab::Utils).to receive(:slugify).with(merge_request.source_branch)
+
+        subject
+      end
+    end
+  end
+
   describe '.triggered_for_branch' do
     subject { described_class.triggered_for_branch(ref) }