Merge branch 'mc/bug/rules-flattening' into 'master'

Allow combining hashes and nested hash arrays

See merge request gitlab-org/gitlab!68398

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

@@ -16,6 +16,7 @@ module Gitlab
 
           PROCESSABLE_ALLOWED_KEYS = %i[extends stage only except rules variables
                                         inherit allow_failure when needs resource_group].freeze
+          MAX_NESTING_LEVEL = 10
 
           included do
             validations do
@@ -31,7 +32,7 @@ module Gitlab
 
               with_options allow_nil: true do
                 validates :extends, array_of_strings_or_string: true
-                validates :rules, nested_array_of_hashes: true
+                validates :rules, nested_array_of_hashes_or_arrays: { max_level: MAX_NESTING_LEVEL }
                 validates :resource_group, type: String
               end
             end

lib/gitlab/config/entry/validators.rb

@@ -90,14 +90,22 @@ module Gitlab
           end
         end
 
-        class NestedArrayOfHashesValidator < ArrayOfHashesValidator
+        class NestedArrayOfHashesOrArraysValidator < ArrayOfHashesValidator
           include NestedArrayHelpers
 
           def validate_each(record, attribute, value)
-            unless validate_nested_array(value, 1, &method(:validate_array_of_hashes))
+            max_level = options.fetch(:max_level, 1)
+
+            unless validate_nested_array(value, max_level, &method(:validate_hash))
               record.errors.add(attribute, 'should be an array containing hashes and arrays of hashes')
             end
           end
+
+          private
+
+          def validate_hash(value)
+            value.is_a?(Hash)
+          end
         end
 
         class ArrayOrStringValidator < ActiveModel::EachValidator

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

@@ -169,6 +169,22 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
           it { expect(entry).to be_valid }
         end
       end
+
+      context 'when rules are used' do
+        let(:config) { { script: 'ls', cache: { key: 'test' }, rules: rules } }
+
+        let(:rules) do
+          [
+            { if: '$CI_PIPELINE_SOURCE == "schedule"', when: 'never' },
+            [
+              { if: '$CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH' },
+              { if: '$CI_PIPELINE_SOURCE == "merge_request_event"' }
+            ]
+          ]
+        end
+
+        it { expect(entry).to be_valid }
+      end
     end
 
     context 'when entry value is not correct' do
@@ -485,6 +501,70 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
           end
         end
       end
+
+      context 'when invalid rules are used' do
+        let(:config) { { script: 'ls', cache: { key: 'test' }, rules: rules } }
+
+        context 'with rules nested more than max allowed levels' do
+          let(:sample_rule) { { if: '$THIS == "other"', when: 'always' } }
+
+          let(:rules) do
+            [
+              { if: '$THIS == "that"', when: 'always' },
+              [
+                { if: '$SKIP', when: 'never' },
+                [
+                  sample_rule,
+                  [
+                    sample_rule,
+                    [
+                      sample_rule,
+                      [
+                        sample_rule,
+                        [
+                          sample_rule,
+                          [
+                            sample_rule,
+                            [
+                              sample_rule,
+                              [
+                                sample_rule,
+                                [
+                                  sample_rule,
+                                  [
+                                    sample_rule,
+                                    [sample_rule]
+                                  ]
+                                ]
+                              ]
+                            ]
+                          ]
+                        ]
+                      ]
+                    ]
+                  ]
+                ]
+              ]
+            ]
+          end
+
+          it { expect(entry).not_to be_valid }
+        end
+
+        context 'with rules with invalid keys' do
+          let(:rules) do
+            [
+              { invalid_key: 'invalid' },
+              [
+                { if: '$CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH' },
+                { if: '$CI_PIPELINE_SOURCE == "merge_request_event"' }
+              ]
+            ]
+          end
+
+          it { expect(entry).not_to be_valid }
+        end
+      end
     end
   end
 

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

@@ -53,7 +53,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Rules do
       let(:config) do
         [
           { if: '$THIS == "that"', when: 'always' },
-          [{ if: '$SKIP', when: 'never' }]
+          [{ if: '$SKIP', when: 'never' }, { if: '$THIS == "other"', when: 'always' }]
         ]
       end
 
@@ -64,11 +64,11 @@ RSpec.describe Gitlab::Ci::Config::Entry::Rules do
       let(:config) do
         [
           { if: '$THIS == "that"', when: 'always' },
-          [{ if: '$SKIP', when: 'never' }, [{ if: '$THIS == "other"', when: 'aways' }]]
+          [{ if: '$SKIP', when: 'never' }, [{ if: '$THIS == "other"', when: 'always' }]]
         ]
       end
 
-      it { is_expected.not_to be_valid }
+      it { is_expected.to be_valid }
     end
   end
 
@@ -119,7 +119,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Rules do
     context 'with rules nested more than one level' do
       let(:first_rule) { { if: '$THIS == "that"', when: 'always' } }
       let(:second_rule) { { if: '$SKIP', when: 'never' } }
-      let(:third_rule) { { if: '$THIS == "other"', when: 'aways' } }
+      let(:third_rule) { { if: '$THIS == "other"', when: 'always' } }
 
       let(:config) do
         [