diff --git a/.rubocop_todo/rspec/feature_category.yml b/.rubocop_todo/rspec/feature_category.yml index 163eeb207ad2895ee7ace785ed7da094b0be1381..59bd65a04bb04dac771b585ae2a449d20a3e0913 100644 --- a/.rubocop_todo/rspec/feature_category.yml +++ b/.rubocop_todo/rspec/feature_category.yml @@ -2933,8 +2933,6 @@ RSpec/FeatureCategory: - 'spec/lib/gitlab/ci/config/entry/retry_spec.rb' - 'spec/lib/gitlab/ci/config/entry/root_spec.rb' - 'spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb' - - 'spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb' - - 'spec/lib/gitlab/ci/config/entry/rules_spec.rb' - 'spec/lib/gitlab/ci/config/entry/service_spec.rb' - 'spec/lib/gitlab/ci/config/entry/services_spec.rb' - 'spec/lib/gitlab/ci/config/entry/stage_spec.rb' diff --git a/lib/gitlab/ci/config/entry/processable.rb b/lib/gitlab/ci/config/entry/processable.rb index 0b322fd433cae15bec375a21383ff040c5d58583..d19140851f57182481d395abe0f4cae66b81037e 100644 --- a/lib/gitlab/ci/config/entry/processable.rb +++ b/lib/gitlab/ci/config/entry/processable.rb @@ -58,7 +58,8 @@ module Processable description: 'List of evaluable Rules to determine job inclusion.', inherit: false, metadata: { - allowed_when: %w[on_success on_failure always never manual delayed].freeze + allowed_when: %w[on_success on_failure always never manual delayed].freeze, + allowed_keys: %i[if changes exists when start_in allow_failure variables needs].freeze } entry :variables, ::Gitlab::Ci::Config::Entry::Variables, diff --git a/lib/gitlab/ci/config/entry/rules/rule.rb b/lib/gitlab/ci/config/entry/rules/rule.rb index 1e7f6056a651e90b4d1dcb85b2f28ad090cdcc85..f5b5323c532bee29f26bbe551f0ea8f19043c811 100644 --- a/lib/gitlab/ci/config/entry/rules/rule.rb +++ b/lib/gitlab/ci/config/entry/rules/rule.rb @@ -4,14 +4,16 @@ module Gitlab module Ci class Config module Entry + # A rule is a condition that is evaluated before a job is executed. + # Until we find a better solution in https://gitlab.com/gitlab-org/gitlab/-/issues/436473, + # these two metadata parameters need to be passed to `Entry::Rules`: + # - `allowed_when`: a list of allowed values for the `when` keyword. + # - `allowed_keys`: a list of allowed keys for each rule. class Rules::Rule < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Validatable include ::Gitlab::Config::Entry::Configurable include ::Gitlab::Config::Entry::Attributable - ALLOWED_KEYS = %i[if changes exists when start_in allow_failure variables needs].freeze - ALLOWED_WHEN = %w[on_success on_failure always never manual delayed].freeze - attributes :if, :exists, :when, :start_in, :allow_failure entry :changes, Entry::Rules::Rule::Changes, @@ -28,7 +30,6 @@ class Rules::Rule < ::Gitlab::Config::Entry::Node validations do validates :config, presence: true validates :config, type: { with: Hash } - validates :config, allowed_keys: ALLOWED_KEYS validates :config, disallowed_keys: %i[start_in], unless: :specifies_delay? validates :start_in, presence: true, if: :specifies_delay? validates :start_in, duration: { limit: '1 week' }, if: :specifies_delay? @@ -36,15 +37,22 @@ class Rules::Rule < ::Gitlab::Config::Entry::Node with_options allow_nil: true do validates :if, expression: true validates :exists, array_of_strings: true, length: { maximum: 50 } - validates :when, allowed_values: { in: ALLOWED_WHEN } validates :allow_failure, boolean: true end validate do + # This validation replaces the old `validates :when, allowed_values: { in: ALLOWED_WHEN }` validation. + # In https://gitlab.com/gitlab-org/gitlab/-/issues/436473, we'll remove this custom validation. validates_with Gitlab::Config::Entry::Validators::AllowedValuesValidator, attributes: %i[when], allow_nil: true, in: opt(:allowed_when) + + # This validation replaces the old `validates :config, allowed_keys: ALLOWED_KEYS` validation. + # In https://gitlab.com/gitlab-org/gitlab/-/issues/436473, we'll remove this custom validation. + validates_with Gitlab::Config::Entry::Validators::AllowedKeysValidator, + attributes: %i[config], + in: opt(:allowed_keys) end end diff --git a/lib/gitlab/ci/config/entry/workflow.rb b/lib/gitlab/ci/config/entry/workflow.rb index 5b81c74fe4dd68352314f9c5ac8ab04248926b15..7aafd9ba78e4618470823f3eb1794ab01ef850c2 100644 --- a/lib/gitlab/ci/config/entry/workflow.rb +++ b/lib/gitlab/ci/config/entry/workflow.rb @@ -19,9 +19,14 @@ class Workflow < ::Gitlab::Config::Entry::Node validates :name, allow_nil: true, length: { minimum: 1, maximum: 255 } end + # `start_in`, `allow_failure`, and `needs` should not be allowed but we can't break this behavior now. + # More information: https://gitlab.com/gitlab-org/gitlab/-/issues/436473 entry :rules, Entry::Rules, description: 'List of evaluable Rules to determine Pipeline status.', - metadata: { allowed_when: %w[always never] } + metadata: { + allowed_when: %w[always never].freeze, + allowed_keys: %i[if changes exists when start_in allow_failure variables needs].freeze + } entry :auto_cancel, Entry::AutoCancel, description: 'Auto-cancel configuration for this pipeline.' diff --git a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb index 3531d6e9f1af9101942503404db43b98163c5243..23b1046deb74ddc4e295d5c554ab39d119857dba 100644 --- a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' require 'gitlab_chronic_duration' -RSpec.describe Gitlab::Ci::Config::Entry::Rules::Rule do +RSpec.describe Gitlab::Ci::Config::Entry::Rules::Rule, feature_category: :pipeline_composition do let(:factory) do Gitlab::Config::Entry::Factory.new(described_class) .metadata(metadata) @@ -11,7 +11,10 @@ end let(:metadata) do - { allowed_when: %w[on_success on_failure always never manual delayed] } + { + allowed_when: %w[on_success on_failure always never manual delayed], + allowed_keys: %i[if changes exists when start_in allow_failure variables needs] + } end let(:entry) { factory.create! } @@ -296,35 +299,8 @@ end end - context 'with a string passed in metadata but not allowed in the class' do - let(:metadata) { { allowed_when: %w[explode] } } - - let(:config) do - { if: '$THIS == "that"', when: 'explode' } - end - - it { is_expected.to be_a(described_class) } - it { is_expected.not_to be_valid } - - it 'returns an error about invalid when:' do - expect(subject.errors).to include(/when unknown value: explode/) - end - - context 'when composed' do - before do - subject.compose! - end - - it { is_expected.not_to be_valid } - - it 'returns an error about invalid when:' do - expect(subject.errors).to include(/when unknown value: explode/) - end - end - end - - context 'with a string allowed in the class but not passed in metadata' do - let(:metadata) { { allowed_when: %w[always never] } } + context 'with an invalid when' do + let(:metadata) { { allowed_when: %w[always never], allowed_keys: %i[if when] } } let(:config) do { if: '$THIS == "that"', when: 'on_success' } diff --git a/spec/lib/gitlab/ci/config/entry/rules_spec.rb b/spec/lib/gitlab/ci/config/entry/rules_spec.rb index b0871f2345e0d305695c6b9d33ff6972bd835e8e..37219c743ccac82f322b7fb848795ea507122eb9 100644 --- a/spec/lib/gitlab/ci/config/entry/rules_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules_spec.rb @@ -3,14 +3,16 @@ require 'fast_spec_helper' require_dependency 'active_model' -RSpec.describe Gitlab::Ci::Config::Entry::Rules do +RSpec.describe Gitlab::Ci::Config::Entry::Rules, feature_category: :pipeline_composition do let(:factory) do Gitlab::Config::Entry::Factory.new(described_class) .metadata(metadata) .value(config) end - let(:metadata) { { allowed_when: %w[always never] } } + let(:metadata) do + { allowed_when: %w[always never], allowed_keys: %i[if when] } + end subject(:entry) { factory.create! } diff --git a/spec/lib/gitlab/ci/config/entry/workflow_spec.rb b/spec/lib/gitlab/ci/config/entry/workflow_spec.rb index d3ce3ffe641da87158f5d1df0a0f6b254f972895..01ba72ff042702279fb20285e4bcb9150b75ebae 100644 --- a/spec/lib/gitlab/ci/config/entry/workflow_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/workflow_spec.rb @@ -6,6 +6,10 @@ subject(:config) { described_class.new(workflow_hash) } describe 'validations' do + before do + config.compose! + end + context 'when work config value is a string' do let(:workflow_hash) { 'build' } @@ -27,6 +31,28 @@ end context 'when work config value is a hash' do + context 'with an invalid key' do + let(:workflow_hash) { { trash: [{ if: '$VAR' }] } } + + describe '#valid?' do + it 'is invalid' do + expect(config).not_to be_valid + end + + it 'attaches an error specifying the unknown key' do + expect(config.errors).to include('workflow config contains unknown keys: trash') + end + end + + describe '#value' do + it 'returns the invalid configuration' do + expect(config.value).to eq(workflow_hash) + end + end + end + end + + context 'when config has rules' do let(:workflow_hash) { { rules: [{ if: '$VAR' }] } } describe '#valid?' do @@ -45,8 +71,8 @@ end end - context 'with an invalid key' do - let(:workflow_hash) { { trash: [{ if: '$VAR' }] } } + context 'when rules has an invalid key' do + let(:workflow_hash) { { rules: [{ if: '$VAR', trash: 'something' }] } } describe '#valid?' do it 'is invalid' do @@ -54,7 +80,7 @@ end it 'attaches an error specifying the unknown key' do - expect(config.errors).to include('workflow config contains unknown keys: trash') + expect(config.errors).to include('rules:rule config contains unknown keys: trash') end end