From 4f472d49b52452b0377fa0701dfe970f3e83bb85 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 27 Feb 2018 12:24:16 +0100 Subject: [PATCH] Make pipeline priority variables concept explicit --- app/models/ci/build.rb | 3 ++- app/models/ci/pipeline.rb | 16 +++++++----- app/models/project.rb | 3 ++- spec/models/ci/build_spec.rb | 26 ++++++++++++++++++++ spec/models/ci/pipeline_spec.rb | 43 ++++++++++++++++++++++++++++----- 5 files changed, 77 insertions(+), 14 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8e2b9dd570b..137ccfbed89 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -262,8 +262,9 @@ module Ci variables += secret_variables(environment: environment) variables += trigger_request.user_variables if trigger_request variables += persisted_environment_variables if environment + variables += pipeline.priority_variables - variables + variables.reverse.uniq { |variable| variable.fetch(:key) }.reverse end def features diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 2eb0fa1897f..0435f6b2f9d 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -473,10 +473,9 @@ module Ci end def predefined_variables - predefined = [ + pipeline_predefined = [ { key: 'CI', value: 'true', public: true }, { key: 'GITLAB_CI', value: 'true', public: true }, - { key: 'GITLAB_FEATURES', value: project.namespace.features.join(','), public: true }, { key: 'CI_SERVER_NAME', value: 'GitLab', public: true }, { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true }, { key: 'CI_SERVER_REVISION', value: Gitlab::REVISION, public: true }, @@ -485,11 +484,16 @@ module Ci { key: 'CI_PIPELINE_SOURCE', value: source.to_s, public: true } ] - predefined += project.predefined_variables - predefined += pipeline_schedule.job_variables if pipeline_schedule - predefined += self.variables.map(&:to_runner_variable) + Array(project.predefined_variables) + pipeline_predefined + end + + def priority_variables + Array(pipeline_schedule&.job_variables) + + self.variables.map(&:to_runner_variable) + end - predefined + def runtime_variables + predefined_variables + priority_variables end def queued_duration diff --git a/app/models/project.rb b/app/models/project.rb index fdaa41a79d6..62a7e10a5cf 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1589,7 +1589,8 @@ class Project < ActiveRecord::Base { key: 'CI_PROJECT_PATH_SLUG', value: full_path_slug, public: true }, { key: 'CI_PROJECT_NAMESPACE', value: namespace.full_path, public: true }, { key: 'CI_PROJECT_URL', value: web_url, public: true }, - { key: 'CI_PROJECT_VISIBILITY', value: Gitlab::VisibilityLevel.string_level(visibility_level), public: true } + { key: 'CI_PROJECT_VISIBILITY', value: Gitlab::VisibilityLevel.string_level(visibility_level), public: true }, + { key: 'GITLAB_FEATURES', value: namespace.features.join(','), public: true } ] variables += container_registry_variables diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 5a4080f9c6c..d1db61084c3 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1891,6 +1891,32 @@ describe Ci::Build do end end end + + context 'when there are duplicated variables present ' do + context 'when there are duplicated YAML variables' do + before do + build.yaml_variables = [{ key: 'MYVAR', value: 'first', public: true }, + { key: 'MYVAR', value: 'second', public: true}] + end + + it 'keeps the last occurence of a variable by given key' do + expect(subject).not_to include(key: 'MYVAR', value: 'first', public: true) + expect(subject).to include(key: 'MYVAR', value: 'second', public: true) + end + end + + context 'when pipeline trigger variable overrides YAML variables' do + before do + build.yaml_variables = [{ key: 'MYVAR', value: 'myvar', public: true }] + pipeline.variables.build(key: 'MYVAR', value: 'pipeline value') + end + + it 'overrides YAML variable with a pipeline trigger variable' do + expect(subject).not_to include(key: 'MYVAR', value: 'myvar', public: true) + expect(subject).to include(key: 'MYVAR', value: 'pipeline value', public: false) + end + end + end end describe 'state transition: any => [:pending]' do diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 14d234f6aab..4d5fc1dfcd8 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -167,15 +167,46 @@ describe Ci::Pipeline, :mailer do end end - describe '#predefined_variables' do - subject { pipeline.predefined_variables } + describe 'pipeline variables' do + describe '#predefined_variables' do + subject { pipeline.predefined_variables } - it { is_expected.to be_an(Array) } + it { is_expected.to be_an(Array) } + + it 'includes the defined keys' do + keys = subject.map { |v| v.fetch(:key) } + + expect(keys).to include('CI_PIPELINE_ID', 'CI_CONFIG_PATH', 'CI_PIPELINE_SOURCE') + end + + it 'includes project-level predefined variables' do + keys = subject.map { |v| v.fetch(:key) } + + expect(keys).to include('CI_PROJECT_NAME') + end + end + + describe '#priority_variables' do + before do + pipeline.variables.build(key: 'MY_VAR', value: 'my var') + end - it 'includes the defined keys' do - keys = subject.map { |v| v[:key] } + it 'returns trigger variables' do + expect(pipeline.priority_variables) + .to include(key: 'MY_VAR', value: 'my var', public: false) + end + end - expect(keys).to include('CI_PIPELINE_ID', 'CI_CONFIG_PATH', 'CI_PIPELINE_SOURCE') + describe '#runtime_variables' do + before do + pipeline.variables.build(key: 'MY_VAR', value: 'my var') + end + + it 'includes predefined and priority variables' do + variables = pipeline.runtime_variables.map { |v| v.fetch(:key) } + + expect(variables).to include('MY_VAR', 'CI_PIPELINE_ID', 'CI_PROJECT_ID') + end end end -- GitLab