From 1bf580688890a3b13e7ac0468f29108dafe08f9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 10 Nov 2018 14:34:53 +0100 Subject: [PATCH] Use full ref when possible to avoid ambiguity --- app/models/ci/build.rb | 5 ++-- app/models/ci/pipeline.rb | 3 ++- app/models/concerns/has_ref.rb | 21 +++++++++++++++++ lib/gitlab/ci/pipeline/chain/command.rb | 2 +- spec/models/ci/build_spec.rb | 12 ++++++---- spec/models/ci/pipeline_spec.rb | 4 ++++ spec/models/concerns/has_ref_spec.rb | 31 +++++++++++++++++++++++++ 7 files changed, 70 insertions(+), 8 deletions(-) create mode 100644 app/models/concerns/has_ref.rb create mode 100644 spec/models/concerns/has_ref_spec.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index d86a6eceb59..f5c21481f63 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -10,6 +10,7 @@ module Ci include Importable include Gitlab::Utils::StrongMemoize include Deployable + include HasRef belongs_to :project, inverse_of: :builds belongs_to :runner @@ -640,11 +641,11 @@ module Ci def secret_group_variables return [] unless project.group - project.group.ci_variables_for(ref, project) + project.group.ci_variables_for(git_ref, project) end def secret_project_variables(environment: persisted_environment) - project.ci_variables_for(ref: ref, environment: environment) + project.ci_variables_for(ref: git_ref, environment: environment) end def steps diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index d06022a0fb7..a1a11d2f7ab 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -11,6 +11,7 @@ module Ci include Gitlab::Utils::StrongMemoize include AtomicInternalId include EnumWithNil + include HasRef belongs_to :project, inverse_of: :all_pipelines belongs_to :user @@ -588,7 +589,7 @@ module Ci end def protected_ref? - strong_memoize(:protected_ref) { project.protected_for?(ref) } + strong_memoize(:protected_ref) { project.protected_for?(git_ref) } end def legacy_trigger diff --git a/app/models/concerns/has_ref.rb b/app/models/concerns/has_ref.rb new file mode 100644 index 00000000000..79816841f7f --- /dev/null +++ b/app/models/concerns/has_ref.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module HasRef + extend ActiveSupport::Concern + + def branch? + !tag? + end + + private + + def git_ref + if branch? + Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s + elsif tag? + Gitlab::Git::TAG_REF_PREFIX + ref.to_s + else + raise ArgumentError, 'Invalid pipeline type!' + end + end +end diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index 100b9521412..316c283d90b 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -54,7 +54,7 @@ module Gitlab def protected_ref? strong_memoize(:protected_ref) do - project.protected_for?(ref) + project.protected_for?(origin_ref) end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 89f78f629d4..d4056b4f7f1 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2385,6 +2385,8 @@ describe Ci::Build do end context 'when protected variable is defined' do + let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } + let(:protected_variable) do { key: 'PROTECTED_KEY', value: 'protected_value', public: false } end @@ -2397,7 +2399,7 @@ describe Ci::Build do context 'when the branch is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2405,7 +2407,7 @@ describe Ci::Build do context 'when the tag is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2430,6 +2432,8 @@ describe Ci::Build do end context 'when group protected variable is defined' do + let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } + let(:protected_variable) do { key: 'PROTECTED_KEY', value: 'protected_value', public: false } end @@ -2442,7 +2446,7 @@ describe Ci::Build do context 'when the branch is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2450,7 +2454,7 @@ describe Ci::Build do context 'when the tag is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index b67c6a4cffa..17f33785fda 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -397,6 +397,10 @@ describe Ci::Pipeline, :mailer do end describe '#protected_ref?' do + before do + pipeline.project = create(:project, :repository) + end + it 'delegates method to project' do expect(pipeline).not_to be_protected_ref end diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb new file mode 100644 index 00000000000..ec3cf9a8580 --- /dev/null +++ b/spec/models/concerns/has_ref_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe HasRef do + describe '#branch?' do + let(:pipeline) { create(:ci_pipeline) } + + subject { pipeline.branch? } + + context 'is not a tag' do + before do + pipeline.tag = false + end + + it 'return true when tag is set to false' do + is_expected.to be_truthy + end + end + + context 'is not a tag' do + before do + pipeline.tag = true + end + + it 'return false when tag is set to true' do + is_expected.to be_falsey + end + end + end +end