diff options
-rw-r--r-- | changelogs/unreleased/define-default-value-for-only-except-keys.yml | 2 | ||||
-rw-r--r-- | lib/gitlab/ci/config/entry/except_policy.rb | 17 | ||||
-rw-r--r-- | lib/gitlab/ci/config/entry/job.rb | 15 | ||||
-rw-r--r-- | lib/gitlab/ci/config/entry/only_policy.rb | 18 | ||||
-rw-r--r-- | lib/gitlab/ci/config/entry/policy.rb | 15 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/config/entry/except_policy_spec.rb | 15 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/config/entry/global_spec.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/config/entry/job_spec.rb | 3 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/config/entry/jobs_spec.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/config/entry/only_policy_spec.rb | 15 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/config/entry/policy_spec.rb | 167 | ||||
-rw-r--r-- | spec/services/ci/create_pipeline_service_spec.rb | 31 | ||||
-rw-r--r-- | spec/support/shared_examples/only_except_policy_examples.rb | 167 |
13 files changed, 223 insertions, 254 deletions
diff --git a/changelogs/unreleased/define-default-value-for-only-except-keys.yml b/changelogs/unreleased/define-default-value-for-only-except-keys.yml index 3e5ecdcf51e..ed0e982f0fc 100644 --- a/changelogs/unreleased/define-default-value-for-only-except-keys.yml +++ b/changelogs/unreleased/define-default-value-for-only-except-keys.yml @@ -1,5 +1,5 @@ --- title: Define the default value for only/except policies -merge_request: 23531 +merge_request: 23765 author: type: changed diff --git a/lib/gitlab/ci/config/entry/except_policy.rb b/lib/gitlab/ci/config/entry/except_policy.rb deleted file mode 100644 index 46ded35325d..00000000000 --- a/lib/gitlab/ci/config/entry/except_policy.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Ci - class Config - module Entry - ## - # Entry that represents an only/except trigger policy for the job. - # - class ExceptPolicy < Policy - def self.default - end - end - end - end - end -end diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb index 085be5da08d..50942fbdb40 100644 --- a/lib/gitlab/ci/config/entry/job.rb +++ b/lib/gitlab/ci/config/entry/job.rb @@ -16,6 +16,13 @@ module Gitlab dependencies before_script after_script variables environment coverage retry parallel extends].freeze + DEFAULT_ONLY_POLICY = { + refs: %w(branches tags) + }.freeze + + DEFAULT_EXCEPT_POLICY = { + }.freeze + validations do validates :config, allowed_keys: ALLOWED_KEYS validates :config, presence: true @@ -65,10 +72,10 @@ module Gitlab entry :services, Entry::Services, description: 'Services that will be used to execute this job.' - entry :only, Entry::OnlyPolicy, + entry :only, Entry::Policy, description: 'Refs policy this job will be executed for.' - entry :except, Entry::ExceptPolicy, + entry :except, Entry::Policy, description: 'Refs policy this job will be executed for.' entry :variables, Entry::Variables, @@ -154,8 +161,8 @@ module Gitlab services: services_value, stage: stage_value, cache: cache_value, - only: only_value, - except: except_value, + only: DEFAULT_ONLY_POLICY.deep_merge(only_value.to_h), + except: DEFAULT_EXCEPT_POLICY.deep_merge(except_value.to_h), variables: variables_defined? ? variables_value : nil, environment: environment_defined? ? environment_value : nil, environment_name: environment_defined? ? environment_value[:name] : nil, diff --git a/lib/gitlab/ci/config/entry/only_policy.rb b/lib/gitlab/ci/config/entry/only_policy.rb deleted file mode 100644 index 9a581b8e97e..00000000000 --- a/lib/gitlab/ci/config/entry/only_policy.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Ci - class Config - module Entry - ## - # Entry that represents an only/except trigger policy for the job. - # - class OnlyPolicy < Policy - def self.default - { refs: %w[branches tags] } - end - end - end - end - end -end diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb index 81e74a639fc..998da1f6837 100644 --- a/lib/gitlab/ci/config/entry/policy.rb +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -5,9 +5,12 @@ module Gitlab class Config module Entry ## - # Base class for OnlyPolicy and ExceptPolicy + # Entry that represents an only/except trigger policy for the job. # class Policy < ::Gitlab::Config::Entry::Simplifiable + strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) } + strategy :ComplexPolicy, if: -> (config) { config.is_a?(Hash) } + class RefsPolicy < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Validatable @@ -63,16 +66,6 @@ module Gitlab def self.default end - - ## - # Class-level execution won't be inherited by subclasses by default. - # Therefore, we need to explicitly execute that for OnlyPolicy and ExceptPolicy - def self.inherited(klass) - super - - klass.strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) } - klass.strategy :ComplexPolicy, if: -> (config) { config.is_a?(Hash) } - end end end end diff --git a/spec/lib/gitlab/ci/config/entry/except_policy_spec.rb b/spec/lib/gitlab/ci/config/entry/except_policy_spec.rb deleted file mode 100644 index d036bf2f4d1..00000000000 --- a/spec/lib/gitlab/ci/config/entry/except_policy_spec.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::Ci::Config::Entry::ExceptPolicy do - let(:entry) { described_class.new(config) } - - it_behaves_like 'correct only except policy' - - describe '.default' do - it 'does not have a default value' do - expect(described_class.default).to be_nil - end - end -end diff --git a/spec/lib/gitlab/ci/config/entry/global_spec.rb b/spec/lib/gitlab/ci/config/entry/global_spec.rb index 12f4b9dc624..61d78f86b51 100644 --- a/spec/lib/gitlab/ci/config/entry/global_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/global_spec.rb @@ -161,7 +161,8 @@ describe Gitlab::Ci::Config::Entry::Global do variables: { 'VAR' => 'value' }, ignore: false, after_script: ['make clean'], - only: { refs: %w[branches tags] } }, + only: { refs: %w[branches tags] }, + except: {} }, spinach: { name: :spinach, before_script: [], script: %w[spinach], @@ -173,7 +174,8 @@ describe Gitlab::Ci::Config::Entry::Global do variables: {}, ignore: false, after_script: ['make clean'], - only: { refs: %w[branches tags] } } + only: { refs: %w[branches tags] }, + except: {} } ) end end diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb index c1f4a060063..8e32cede3b5 100644 --- a/spec/lib/gitlab/ci/config/entry/job_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb @@ -259,7 +259,8 @@ describe Gitlab::Ci::Config::Entry::Job do stage: 'test', ignore: false, after_script: %w[cleanup], - only: { refs: %w[branches tags] }) + only: { refs: %w[branches tags] }, + except: {}) end end end diff --git a/spec/lib/gitlab/ci/config/entry/jobs_spec.rb b/spec/lib/gitlab/ci/config/entry/jobs_spec.rb index 2a753408f54..1a2c30d3571 100644 --- a/spec/lib/gitlab/ci/config/entry/jobs_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/jobs_spec.rb @@ -68,13 +68,15 @@ describe Gitlab::Ci::Config::Entry::Jobs do commands: 'rspec', ignore: false, stage: 'test', - only: { refs: %w[branches tags] } }, + only: { refs: %w[branches tags] }, + except: {} }, spinach: { name: :spinach, script: %w[spinach], commands: 'spinach', ignore: false, stage: 'test', - only: { refs: %w[branches tags] } }) + only: { refs: %w[branches tags] }, + except: {} }) end end diff --git a/spec/lib/gitlab/ci/config/entry/only_policy_spec.rb b/spec/lib/gitlab/ci/config/entry/only_policy_spec.rb deleted file mode 100644 index 5518b68e51a..00000000000 --- a/spec/lib/gitlab/ci/config/entry/only_policy_spec.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::Ci::Config::Entry::OnlyPolicy do - let(:entry) { described_class.new(config) } - - it_behaves_like 'correct only except policy' - - describe '.default' do - it 'haa a default value' do - expect(described_class.default).to eq( { refs: %w[branches tags] } ) - end - end -end diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb index cf40a22af2e..83001b7fdd8 100644 --- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb @@ -1,8 +1,173 @@ -require 'spec_helper' +require 'fast_spec_helper' +require_dependency 'active_model' describe Gitlab::Ci::Config::Entry::Policy do let(:entry) { described_class.new(config) } + context 'when using simplified policy' do + describe 'validations' do + context 'when entry config value is valid' do + context 'when config is a branch or tag name' do + let(:config) { %w[master feature/branch] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + + describe '#value' do + it 'returns refs hash' do + expect(entry.value).to eq(refs: config) + end + end + end + + context 'when config is a regexp' do + let(:config) { ['/^issue-.*$/'] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + + context 'when config is a special keyword' do + let(:config) { %w[tags triggers branches] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + end + + context 'when entry value is not valid' do + let(:config) { [1] } + + describe '#errors' do + it 'saves errors' do + expect(entry.errors) + .to include /policy config should be an array of strings or regexps/ + end + end + end + end + end + + context 'when using complex policy' do + context 'when specifying refs policy' do + let(:config) { { refs: ['master'] } } + + it 'is a correct configuraton' do + expect(entry).to be_valid + expect(entry.value).to eq(refs: %w[master]) + end + end + + context 'when specifying kubernetes policy' do + let(:config) { { kubernetes: 'active' } } + + it 'is a correct configuraton' do + expect(entry).to be_valid + expect(entry.value).to eq(kubernetes: 'active') + end + end + + context 'when specifying invalid kubernetes policy' do + let(:config) { { kubernetes: 'something' } } + + it 'reports an error about invalid policy' do + expect(entry.errors).to include /unknown value: something/ + end + end + + context 'when specifying valid variables expressions policy' do + let(:config) { { variables: ['$VAR == null'] } } + + it 'is a correct configuraton' do + expect(entry).to be_valid + expect(entry.value).to eq(config) + end + end + + context 'when specifying variables expressions in invalid format' do + let(:config) { { variables: '$MY_VAR' } } + + it 'reports an error about invalid format' do + expect(entry.errors).to include /should be an array of strings/ + end + end + + context 'when specifying invalid variables expressions statement' do + let(:config) { { variables: ['$MY_VAR =='] } } + + it 'reports an error about invalid statement' do + expect(entry.errors).to include /invalid expression syntax/ + end + end + + context 'when specifying invalid variables expressions token' do + let(:config) { { variables: ['$MY_VAR == 123'] } } + + it 'reports an error about invalid expression' do + expect(entry.errors).to include /invalid expression syntax/ + end + end + + context 'when using invalid variables expressions regexp' do + let(:config) { { variables: ['$MY_VAR =~ /some ( thing/'] } } + + it 'reports an error about invalid expression' do + expect(entry.errors).to include /invalid expression syntax/ + end + end + + context 'when specifying a valid changes policy' do + let(:config) { { changes: %w[some/* paths/**/*.rb] } } + + it 'is a correct configuraton' do + expect(entry).to be_valid + expect(entry.value).to eq(config) + end + end + + context 'when changes policy is invalid' do + let(:config) { { changes: [1, 2] } } + + it 'returns errors' do + expect(entry.errors).to include /changes should be an array of strings/ + end + end + + context 'when specifying unknown policy' do + let(:config) { { refs: ['master'], invalid: :something } } + + it 'returns error about invalid key' do + expect(entry.errors).to include /unknown keys: invalid/ + end + end + + context 'when policy is empty' do + let(:config) { {} } + + it 'is not a valid configuration' do + expect(entry.errors).to include /can't be blank/ + end + end + end + + context 'when policy strategy does not match' do + let(:config) { 'string strategy' } + + it 'returns information about errors' do + expect(entry.errors) + .to include /has to be either an array of conditions or a hash/ + end + end + describe '.default' do it 'does not have a default value' do expect(described_class.default).to be_nil diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 8b8021ecbc8..ffa47d527f7 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -840,6 +840,37 @@ describe Ci::CreatePipelineService do end end + context "when config uses variables for only keyword" do + let(:config) do + { + build: { + stage: 'build', + script: 'echo', + only: { + variables: %w($CI) + } + } + } + end + + context 'when merge request is specified' do + let(:merge_request) do + create(:merge_request, + source_project: project, + source_branch: ref_name, + target_project: project, + target_branch: 'master') + end + + it 'does not create a merge request pipeline' do + expect(pipeline).not_to be_persisted + + expect(pipeline.errors[:base]) + .to eq(['No stages / jobs for this pipeline.']) + end + end + end + context "when config has 'except: [tags]'" do let(:config) do { diff --git a/spec/support/shared_examples/only_except_policy_examples.rb b/spec/support/shared_examples/only_except_policy_examples.rb deleted file mode 100644 index 35240af1d74..00000000000 --- a/spec/support/shared_examples/only_except_policy_examples.rb +++ /dev/null @@ -1,167 +0,0 @@ -# frozen_string_literal: true - -shared_examples 'correct only except policy' do - context 'when using simplified policy' do - describe 'validations' do - context 'when entry config value is valid' do - context 'when config is a branch or tag name' do - let(:config) { %w[master feature/branch] } - - describe '#valid?' do - it 'is valid' do - expect(entry).to be_valid - end - end - - describe '#value' do - it 'returns refs hash' do - expect(entry.value).to eq(refs: config) - end - end - end - - context 'when config is a regexp' do - let(:config) { ['/^issue-.*$/'] } - - describe '#valid?' do - it 'is valid' do - expect(entry).to be_valid - end - end - end - - context 'when config is a special keyword' do - let(:config) { %w[tags triggers branches] } - - describe '#valid?' do - it 'is valid' do - expect(entry).to be_valid - end - end - end - end - - context 'when entry value is not valid' do - let(:config) { [1] } - - describe '#errors' do - it 'saves errors' do - expect(entry.errors) - .to include /policy config should be an array of strings or regexps/ - end - end - end - end - end - - context 'when using complex policy' do - context 'when specifying refs policy' do - let(:config) { { refs: ['master'] } } - - it 'is a correct configuraton' do - expect(entry).to be_valid - expect(entry.value).to eq(refs: %w[master]) - end - end - - context 'when specifying kubernetes policy' do - let(:config) { { kubernetes: 'active' } } - - it 'is a correct configuraton' do - expect(entry).to be_valid - expect(entry.value).to eq(kubernetes: 'active') - end - end - - context 'when specifying invalid kubernetes policy' do - let(:config) { { kubernetes: 'something' } } - - it 'reports an error about invalid policy' do - expect(entry.errors).to include /unknown value: something/ - end - end - - context 'when specifying valid variables expressions policy' do - let(:config) { { variables: ['$VAR == null'] } } - - it 'is a correct configuraton' do - expect(entry).to be_valid - expect(entry.value).to eq(config) - end - end - - context 'when specifying variables expressions in invalid format' do - let(:config) { { variables: '$MY_VAR' } } - - it 'reports an error about invalid format' do - expect(entry.errors).to include /should be an array of strings/ - end - end - - context 'when specifying invalid variables expressions statement' do - let(:config) { { variables: ['$MY_VAR =='] } } - - it 'reports an error about invalid statement' do - expect(entry.errors).to include /invalid expression syntax/ - end - end - - context 'when specifying invalid variables expressions token' do - let(:config) { { variables: ['$MY_VAR == 123'] } } - - it 'reports an error about invalid expression' do - expect(entry.errors).to include /invalid expression syntax/ - end - end - - context 'when using invalid variables expressions regexp' do - let(:config) { { variables: ['$MY_VAR =~ /some ( thing/'] } } - - it 'reports an error about invalid expression' do - expect(entry.errors).to include /invalid expression syntax/ - end - end - - context 'when specifying a valid changes policy' do - let(:config) { { changes: %w[some/* paths/**/*.rb] } } - - it 'is a correct configuraton' do - expect(entry).to be_valid - expect(entry.value).to eq(config) - end - end - - context 'when changes policy is invalid' do - let(:config) { { changes: [1, 2] } } - - it 'returns errors' do - expect(entry.errors).to include /changes should be an array of strings/ - end - end - - context 'when specifying unknown policy' do - let(:config) { { refs: ['master'], invalid: :something } } - - it 'returns error about invalid key' do - expect(entry.errors).to include /unknown keys: invalid/ - end - end - - context 'when policy is empty' do - let(:config) { {} } - - it 'is not a valid configuration' do - expect(entry.errors).to include /can't be blank/ - end - end - end - - context 'when policy strategy does not match' do - let(:config) { 'string strategy' } - - it 'returns information about errors' do - expect(entry.errors) - .to include /has to be either an array of conditions or a hash/ - end - end -end |