Skip to content
Snippets Groups Projects
Commit 64f67036 authored by Furkan Ayhan's avatar Furkan Ayhan :two:
Browse files

Refactor metadata usage in CI Rules entry

Entry::Rules is shared between the Workflow and Processable entries.
Normally, we support only the if, changes, exists, when, and variables
keys in workflow:rules. Yet, we accidentally allow start_in,
allow_failure, and needs.

In this change, we don't aim to fix this, this will be done in
#436473.

In this change, we aim to simplify and highlight this behavior by
adding allowed_keys in the Rules usage.

Also, in validations, we start using only allowed_when and allowed_keys,
we don't use the class constants anymore.
parent 8bc78e71
No related branches found
No related tags found
No related merge requests found
......@@ -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'
......
......@@ -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,
......
......@@ -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
......
......@@ -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.'
......
......@@ -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' }
......
......@@ -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! }
......
......@@ -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
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment