Refactored Entry::Rules::Rule validation for when:

Separated class-level validation from a class constant, which lists all
allowable values, from instance-level validation via factory metadata
providing the allowed values (subset of class constant) for that
specific instance.

Added validation specs for both created and composed entries.

lib/gitlab/ci/config/entry/job.rb

@@ -114,7 +114,10 @@ module Gitlab
 
           entry :rules, Entry::Rules,
             description: 'List of evaluable Rules to determine job inclusion.',
-            inherit: false
+            inherit: false,
+            metadata: {
+              allowed_when: %w[on_success on_failure always never manual delayed].freeze
+            }
 
           entry :needs, Entry::Needs,
             description: 'Needs configuration for this job.',

lib/gitlab/ci/config/entry/rules/rule.rb

@@ -8,9 +8,9 @@ module Gitlab
           include ::Gitlab::Config::Entry::Validatable
           include ::Gitlab::Config::Entry::Attributable
 
-          CLAUSES      = %i[if changes exists].freeze
-          ALLOWED_KEYS = %i[if changes exists when start_in].freeze
-          ALLOWED_WHEN = %w[on_success on_failure always never manual delayed].freeze
+          CLAUSES        = %i[if changes exists].freeze
+          ALLOWED_KEYS   = %i[if changes exists when start_in].freeze
+          ALLOWABLE_WHEN = %w[on_success on_failure always never manual delayed].freeze
 
           attributes :if, :changes, :exists, :when, :start_in
 
@@ -25,7 +25,14 @@ module Gitlab
             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: ALLOWED_WHEN }
+              validates :when, allowed_values: { in: ALLOWABLE_WHEN }
+            end
+
+            validate do
+              validates_with Gitlab::Config::Entry::Validators::AllowedValuesValidator,
+                attributes: %i[when],
+                allow_nil: true,
+                in: opt(:allowed_when)
             end
           end
 

spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb

@@ -6,7 +6,17 @@ require 'support/helpers/stub_feature_flags'
 require_dependency 'active_model'
 
 describe Gitlab::Ci::Config::Entry::Rules::Rule do
-  let(:entry) { described_class.new(config) }
+  let(:factory) do
+    Gitlab::Config::Entry::Factory.new(described_class)
+      .metadata(metadata)
+      .value(config)
+  end
+
+  let(:metadata) do
+    { allowed_when: %w[on_success on_failure always never manual delayed] }
+  end
+
+  let(:entry) { factory.create! }
 
   describe '.new' do
     subject { entry }
@@ -212,6 +222,112 @@ describe Gitlab::Ci::Config::Entry::Rules::Rule do
           .to include(/should be a hash/)
       end
     end
+
+    context 'when: validation' do
+      context 'with an invalid boolean when:' do
+        let(:config) do
+          { if: '$THIS == "that"', when: false }
+        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: false/)
+        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: false/)
+          end
+        end
+      end
+
+      context 'with an invalid string when:' do
+        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 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] } }
+
+        let(:config) do
+          { if: '$THIS == "that"', when: 'on_success' }
+        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: on_success/)
+        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: on_success/)
+          end
+        end
+      end
+    end
   end
 
   describe '#value' do

spec/lib/gitlab/ci/config/entry/rules_spec.rb

@@ -5,7 +5,14 @@ require 'support/helpers/stub_feature_flags'
 require_dependency 'active_model'
 
 describe Gitlab::Ci::Config::Entry::Rules do
-  let(:entry) { described_class.new(config) }
+  let(:factory) do
+    Gitlab::Config::Entry::Factory.new(described_class)
+      .metadata(metadata)
+      .value(config)
+  end
+
+  let(:metadata) { { allowed_when: %w[always never] } }
+  let(:entry)    { factory.create! }
 
   describe '.new' do
     subject { entry }
@@ -18,7 +25,7 @@ describe Gitlab::Ci::Config::Entry::Rules do
       it { is_expected.to be_a(described_class) }
       it { is_expected.to be_valid }
 
-      context 'after #compose!' do
+      context 'when composed' do
         before do
           subject.compose!
         end
@@ -38,7 +45,7 @@ describe Gitlab::Ci::Config::Entry::Rules do
       it { is_expected.to be_a(described_class) }
       it { is_expected.to be_valid }
 
-      context 'after #compose!' do
+      context 'when composed' do
         before do
           subject.compose!
         end
@@ -54,48 +61,6 @@ describe Gitlab::Ci::Config::Entry::Rules do
 
       it { is_expected.not_to be_valid }
     end
-
-    context 'with an invalid boolean when:' do
-      let(:config) do
-        [{ if: '$THIS == "that"', when: false }]
-      end
-
-      it { is_expected.to be_a(described_class) }
-      it { is_expected.to be_valid }
-
-      context 'after #compose!' 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: false/)
-        end
-      end
-    end
-
-    context 'with an invalid string when:' do
-      let(:config) do
-        [{ if: '$THIS == "that"', when: 'explode' }]
-      end
-
-      it { is_expected.to be_a(described_class) }
-      it { is_expected.to be_valid }
-
-      context 'after #compose!' 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
   end
 
   describe '#value' do