From e260da59c6de11dee21a619fb097418c9ea6138e Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Tue, 7 Jun 2022 22:48:46 +0300 Subject: [PATCH 1/2] Move CI rules-changes to an entry class This is a first step of improving the "rules:changes" CI config. This will just move it to its own entry class. --- lib/gitlab/ci/config/entry/rules/rule.rb | 21 +++-- .../ci/config/entry/rules/rule/changes.rb | 21 +++++ .../config/entry/rules/rule/changes_spec.rb | 81 +++++++++++++++++++ .../gitlab/ci/config/entry/rules/rule_spec.rb | 10 ++- 4 files changed, 125 insertions(+), 8 deletions(-) create mode 100644 lib/gitlab/ci/config/entry/rules/rule/changes.rb create mode 100644 spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb diff --git a/lib/gitlab/ci/config/entry/rules/rule.rb b/lib/gitlab/ci/config/entry/rules/rule.rb index 4722f2e9a61ca4..63bf1b38ac68c0 100644 --- a/lib/gitlab/ci/config/entry/rules/rule.rb +++ b/lib/gitlab/ci/config/entry/rules/rule.rb @@ -9,11 +9,13 @@ class Rules::Rule < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Configurable include ::Gitlab::Config::Entry::Attributable - CLAUSES = %i[if changes exists].freeze - ALLOWED_KEYS = %i[if changes exists when start_in allow_failure variables].freeze - ALLOWABLE_WHEN = %w[on_success on_failure always never manual delayed].freeze + ALLOWED_KEYS = %i[if changes exists when start_in allow_failure variables].freeze + ALLOWED_WHEN = %w[on_success on_failure always never manual delayed].freeze - attributes :if, :changes, :exists, :when, :start_in, :allow_failure + attributes :if, :exists, :when, :start_in, :allow_failure + + entry :changes, Entry::Rules::Rule::Changes, + description: 'File change condition rule.' entry :variables, Entry::Variables, description: 'Environment variables to define for rule conditions.' @@ -28,8 +30,8 @@ class Rules::Rule < ::Gitlab::Config::Entry::Node with_options allow_nil: true do validates :if, expression: true - validates :changes, :exists, array_of_strings: true, length: { maximum: 50 } - validates :when, allowed_values: { in: ALLOWABLE_WHEN } + validates :exists, array_of_strings: true, length: { maximum: 50 } + validates :when, allowed_values: { in: ALLOWED_WHEN } validates :allow_failure, boolean: true end @@ -41,6 +43,13 @@ class Rules::Rule < ::Gitlab::Config::Entry::Node end end + def value + config.merge( + changes: (changes_value if changes_defined?), + variables: (variables_value if variables_defined?) + ).compact + end + def specifies_delay? self.when == 'delayed' end diff --git a/lib/gitlab/ci/config/entry/rules/rule/changes.rb b/lib/gitlab/ci/config/entry/rules/rule/changes.rb new file mode 100644 index 00000000000000..04b182a3007813 --- /dev/null +++ b/lib/gitlab/ci/config/entry/rules/rule/changes.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + class Config + module Entry + class Rules + class Rule + class Changes < ::Gitlab::Config::Entry::Node + include ::Gitlab::Config::Entry::Validatable + + validations do + validates :config, array_of_strings: true, length: { maximum: 50 } + end + end + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb b/spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb new file mode 100644 index 00000000000000..348655f9e13a86 --- /dev/null +++ b/spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::Ci::Config::Entry::Rules::Rule::Changes do + let(:factory) do + Gitlab::Config::Entry::Factory.new(described_class) + .value(config) + end + + subject(:entry) { factory.create! } + + before do + entry.compose! + end + + describe '.new' do + context 'when using a string array' do + let(:config) { %w[app/ lib/ spec/ other/* paths/**/*.rb] } + + it { is_expected.to be_valid } + end + + context 'when using an integer array' do + let(:config) { [1, 2] } + + it { is_expected.not_to be_valid } + + it 'returns errors' do + expect(entry.errors).to include(/changes config should be an array of strings/) + end + end + + context 'when using a string' do + let(:config) { 'a regular string' } + + it { is_expected.not_to be_valid } + + it 'reports an error about invalid policy' do + expect(entry.errors).to include(/should be an array of strings/) + end + end + + context 'when using a long array' do + let(:config) { ['app/'] * 51 } + + it { is_expected.not_to be_valid } + + it 'returns errors' do + expect(entry.errors).to include(/too long \(maximum is 50 characters\)/) + end + end + + context 'when clause is empty' do + let(:config) {} + + it { is_expected.to be_valid } + end + + context 'when policy strategy does not match' do + let(:config) { 'string strategy' } + + it { is_expected.not_to be_valid } + + it 'returns information about errors' do + expect(entry.errors) + .to include(/should be an array of strings/) + end + end + end + + describe '#value' do + subject(:value) { entry.value } + + context 'when using a string array' do + let(:config) { %w[app/ lib/ spec/ other/* paths/**/*.rb] } + + it { is_expected.to eq(config) } + end + end +end 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 8627078843154a..6974afee114626 100644 --- a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb @@ -18,6 +18,10 @@ let(:entry) { factory.create! } + before do + entry.compose! + end + describe '.new' do subject { entry } @@ -121,7 +125,7 @@ it { is_expected.not_to be_valid } it 'returns errors' do - expect(subject.errors).to include(/changes should be an array of strings/) + expect(subject.errors).to include(/changes config should be an array of strings/) end end @@ -131,7 +135,7 @@ it { is_expected.not_to be_valid } it 'returns errors' do - expect(subject.errors).to include(/changes is too long \(maximum is 50 characters\)/) + expect(subject.errors).to include(/changes config is too long \(maximum is 50 characters\)/) end end @@ -434,6 +438,8 @@ end describe '.default' do + let(:config) {} + it 'does not have default value' do expect(described_class.default).to be_nil end -- GitLab From b14f5547b8ae4a10c3e5dbd0fca34a2b445dc401 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Fri, 10 Jun 2022 13:03:11 +0300 Subject: [PATCH 2/2] Change the error message --- lib/gitlab/ci/config/entry/rules/rule/changes.rb | 4 +++- spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb | 2 +- spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/ci/config/entry/rules/rule/changes.rb b/lib/gitlab/ci/config/entry/rules/rule/changes.rb index 04b182a3007813..be57e089f34c9d 100644 --- a/lib/gitlab/ci/config/entry/rules/rule/changes.rb +++ b/lib/gitlab/ci/config/entry/rules/rule/changes.rb @@ -10,7 +10,9 @@ class Changes < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Validatable validations do - validates :config, array_of_strings: true, length: { maximum: 50 } + validates :config, + array_of_strings: true, + length: { maximum: 50, too_long: "has too many entries (maximum %{count})" } end end end diff --git a/spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb b/spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb index 348655f9e13a86..3ed4a9f263f1d8 100644 --- a/spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules/rule/changes_spec.rb @@ -47,7 +47,7 @@ it { is_expected.not_to be_valid } it 'returns errors' do - expect(entry.errors).to include(/too long \(maximum is 50 characters\)/) + expect(entry.errors).to include(/has too many entries \(maximum 50\)/) end end 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 6974afee114626..89d349efe8f5cd 100644 --- a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb @@ -135,7 +135,7 @@ it { is_expected.not_to be_valid } it 'returns errors' do - expect(subject.errors).to include(/changes config is too long \(maximum is 50 characters\)/) + expect(subject.errors).to include(/changes config has too many entries \(maximum 50\)/) end end -- GitLab