Unverified Commit 8a52d1ad authored by Sean McGivern's avatar Sean McGivern Committed by John T Skarbek

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
parent 2ee9b141
......@@ -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
......
......@@ -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
......@@ -48,8 +49,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
......
......@@ -754,6 +754,22 @@ module Ci
self.sha == sha || self.source_sha == sha
end
def triggered_by?(current_user)
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
......
......@@ -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?
......
# 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
# 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
......
---
title: Make `CI_COMMIT_REF_NAME` and `SLUG` variable idempotent
merge_request: 27663
author:
type: fixed
......@@ -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]
......
......@@ -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) }
......@@ -2312,6 +2313,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
[
......@@ -2703,6 +2717,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)
......@@ -2752,6 +2768,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
......
......@@ -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) }
......
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