From ac77bb9376ad50899619ff8026e6c6b420ff9c4b Mon Sep 17 00:00:00 2001
From: drew <dscimino@gmail.com>
Date: Tue, 20 Aug 2019 20:03:43 +0000
Subject: Introducing new Syntax for Ci::Build inclusion rules

- Added Gitlab::Ci::Config::Entry::Rules and Gitlab::Ci::Config::Entry::Rules:Rule
  to handle lists of Rule objects to be evalauted for job inclusion
- Added `if:` and `changes:` as available Rules::Rule::Clause classes
- Added Rules handling logic to Seed::Build#included? with extra specs
- Use DisallowedKeysValidator to mutually exclude rules: from only:/except: on job config
---
 spec/lib/gitlab/ci/config/entry/job_spec.rb        | 111 ++++++++++-
 spec/lib/gitlab/ci/config/entry/policy_spec.rb     |  12 +-
 spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb | 208 +++++++++++++++++++++
 spec/lib/gitlab/ci/config/entry/rules_spec.rb      | 135 +++++++++++++
 4 files changed, 461 insertions(+), 5 deletions(-)
 create mode 100644 spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb
 create mode 100644 spec/lib/gitlab/ci/config/entry/rules_spec.rb

(limited to 'spec/lib/gitlab/ci/config')

diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb
index 415ade7a096..1853efde350 100644
--- a/spec/lib/gitlab/ci/config/entry/job_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb
@@ -11,7 +11,7 @@ describe Gitlab::Ci::Config::Entry::Job do
 
       let(:result) do
         %i[before_script script stage type after_script cache
-           image services only except variables artifacts
+           image services only except rules variables artifacts
            environment coverage retry]
       end
 
@@ -201,6 +201,21 @@ describe Gitlab::Ci::Config::Entry::Job do
             expect(entry.errors).to include 'job parallel must be an integer'
           end
         end
+
+        context 'when it uses both "when:" and "rules:"' do
+          let(:config) do
+            {
+              script: 'echo',
+              when: 'on_failure',
+              rules: [{ if: '$VARIABLE', when: 'on_success' }]
+            }
+          end
+
+          it 'returns an error about when: being combined with rules' do
+            expect(entry).not_to be_valid
+            expect(entry.errors).to include 'job config key may not be used with `rules`: when'
+          end
+        end
       end
 
       context 'when delayed job' do
@@ -240,6 +255,100 @@ describe Gitlab::Ci::Config::Entry::Job do
         end
       end
 
+      context 'when only: is used with rules:' do
+        let(:config) { { only: ['merge_requests'], rules: [{ if: '$THIS' }] } }
+
+        it 'returns error about mixing only: with rules:' do
+          expect(entry).not_to be_valid
+          expect(entry.errors).to include /may not be used with `rules`/
+        end
+
+        context 'and only: is blank' do
+          let(:config) { { only: nil, rules: [{ if: '$THIS' }] } }
+
+          it 'returns error about mixing only: with rules:' do
+            expect(entry).not_to be_valid
+            expect(entry.errors).to include /may not be used with `rules`/
+          end
+        end
+
+        context 'and rules: is blank' do
+          let(:config) { { only: ['merge_requests'], rules: nil } }
+
+          it 'returns error about mixing only: with rules:' do
+            expect(entry).not_to be_valid
+            expect(entry.errors).to include /may not be used with `rules`/
+          end
+        end
+      end
+
+      context 'when except: is used with rules:' do
+        let(:config) { { except: { refs: %w[master] }, rules: [{ if: '$THIS' }] } }
+
+        it 'returns error about mixing except: with rules:' do
+          expect(entry).not_to be_valid
+          expect(entry.errors).to include /may not be used with `rules`/
+        end
+
+        context 'and except: is blank' do
+          let(:config) { { except: nil, rules: [{ if: '$THIS' }] } }
+
+          it 'returns error about mixing except: with rules:' do
+            expect(entry).not_to be_valid
+            expect(entry.errors).to include /may not be used with `rules`/
+          end
+        end
+
+        context 'and rules: is blank' do
+          let(:config) { { except: { refs: %w[master] }, rules: nil } }
+
+          it 'returns error about mixing except: with rules:' do
+            expect(entry).not_to be_valid
+            expect(entry.errors).to include /may not be used with `rules`/
+          end
+        end
+      end
+
+      context 'when only: and except: are both used with rules:' do
+        let(:config) do
+          {
+            only: %w[merge_requests],
+            except: { refs: %w[master] },
+            rules: [{ if: '$THIS' }]
+          }
+        end
+
+        it 'returns errors about mixing both only: and except: with rules:' do
+          expect(entry).not_to be_valid
+          expect(entry.errors).to include /may not be used with `rules`/
+          expect(entry.errors).to include /may not be used with `rules`/
+        end
+
+        context 'when only: and except: as both blank' do
+          let(:config) do
+            { only: nil, except: nil, rules: [{ if: '$THIS' }] }
+          end
+
+          it 'returns errors about mixing both only: and except: with rules:' do
+            expect(entry).not_to be_valid
+            expect(entry.errors).to include /may not be used with `rules`/
+            expect(entry.errors).to include /may not be used with `rules`/
+          end
+        end
+
+        context 'when rules: is blank' do
+          let(:config) do
+            { only: %w[merge_requests], except: { refs: %w[master] }, rules: nil }
+          end
+
+          it 'returns errors about mixing both only: and except: with rules:' do
+            expect(entry).not_to be_valid
+            expect(entry.errors).to include /may not be used with `rules`/
+            expect(entry.errors).to include /may not be used with `rules`/
+          end
+        end
+      end
+
       context 'when start_in specified without delayed specification' do
         let(:config) { { start_in: '1 day' } }
 
diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
index 266a27c1e47..a606eb303e7 100644
--- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
@@ -51,8 +51,6 @@ describe Gitlab::Ci::Config::Entry::Policy do
 
           let(:config) { ['/^(?!master).+/'] }
 
-          subject { described_class.new([regexp]) }
-
           context 'when allow_unsafe_ruby_regexp is disabled' do
             before do
               stub_feature_flags(allow_unsafe_ruby_regexp: false)
@@ -113,8 +111,6 @@ describe Gitlab::Ci::Config::Entry::Policy do
 
       let(:config) { { refs: ['/^(?!master).+/'] } }
 
-      subject { described_class.new([regexp]) }
-
       context 'when allow_unsafe_ruby_regexp is disabled' do
         before do
           stub_feature_flags(allow_unsafe_ruby_regexp: false)
@@ -203,6 +199,14 @@ describe Gitlab::Ci::Config::Entry::Policy do
       end
     end
 
+    context 'when changes policy is invalid' do
+      let(:config) { { changes: 'some/*' } }
+
+      it 'returns errors' do
+        expect(entry.errors).to include /changes should be an array of strings/
+      end
+    end
+
     context 'when changes policy is invalid' do
       let(:config) { { changes: [1, 2] } }
 
diff --git a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb
new file mode 100644
index 00000000000..c25344ec1a4
--- /dev/null
+++ b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb
@@ -0,0 +1,208 @@
+require 'fast_spec_helper'
+require 'chronic_duration'
+require 'support/helpers/stub_feature_flags'
+require_dependency 'active_model'
+
+describe Gitlab::Ci::Config::Entry::Rules::Rule do
+  let(:entry) { described_class.new(config) }
+
+  describe '.new' do
+    subject { entry }
+
+    context 'with a when: value but no clauses' do
+      let(:config) { { when: 'manual' } }
+
+      it { is_expected.to be_valid }
+    end
+
+    context 'when specifying an if: clause' do
+      let(:config) { { if: '$THIS || $THAT', when: 'manual' } }
+
+      it { is_expected.to be_valid }
+
+      describe '#when' do
+        subject { entry.when }
+
+        it { is_expected.to eq('manual') }
+      end
+    end
+
+    context 'using a list of multiple expressions' do
+      let(:config) { { if: ['$MY_VAR == "this"', '$YOUR_VAR == "that"'] } }
+
+      it { is_expected.not_to be_valid }
+
+      it 'reports an error about invalid format' do
+        expect(subject.errors).to include(/invalid expression syntax/)
+      end
+    end
+
+    context 'when specifying an invalid if: clause expression' do
+      let(:config) { { if: ['$MY_VAR =='] } }
+
+      it { is_expected.not_to be_valid }
+
+      it 'reports an error about invalid statement' do
+        expect(subject.errors).to include(/invalid expression syntax/)
+      end
+    end
+
+    context 'when specifying an if: clause expression with an invalid token' do
+      let(:config) { { if: ['$MY_VAR == 123'] } }
+
+      it { is_expected.not_to be_valid }
+
+      it 'reports an error about invalid statement' do
+        expect(subject.errors).to include(/invalid expression syntax/)
+      end
+    end
+
+    context 'when using invalid regex in an if: clause' do
+      let(:config) { { if: ['$MY_VAR =~ /some ( thing/'] } }
+
+      it 'reports an error about invalid expression' do
+        expect(subject.errors).to include(/invalid expression syntax/)
+      end
+    end
+
+    context 'when using an if: clause with lookahead regex character "?"' do
+      let(:config) { { if: '$CI_COMMIT_REF =~ /^(?!master).+/' } }
+
+      context 'when allow_unsafe_ruby_regexp is disabled' do
+        it { is_expected.not_to be_valid }
+
+        it 'reports an error about invalid expression syntax' do
+          expect(subject.errors).to include(/invalid expression syntax/)
+        end
+      end
+    end
+
+    context 'when using a changes: clause' do
+      let(:config) { { changes: %w[app/ lib/ spec/ other/* paths/**/*.rb] } }
+
+      it { is_expected.to be_valid }
+    end
+
+    context 'when using a string as an invalid changes: clause' do
+      let(:config) { { changes: 'a regular string' } }
+
+      it { is_expected.not_to be_valid }
+
+      it 'reports an error about invalid policy' do
+        expect(subject.errors).to include(/should be an array of strings/)
+      end
+    end
+
+    context 'when using a list as an invalid changes: clause' do
+      let(:config) { { changes: [1, 2] } }
+
+      it { is_expected.not_to be_valid }
+
+      it 'returns errors' do
+        expect(subject.errors).to include(/changes should be an array of strings/)
+      end
+    end
+
+    context 'specifying a delayed job' do
+      let(:config) { { if: '$THIS || $THAT', when: 'delayed', start_in: '15 minutes' } }
+
+      it { is_expected.to be_valid }
+
+      it 'sets attributes for the job delay' do
+        expect(entry.when).to eq('delayed')
+        expect(entry.start_in).to eq('15 minutes')
+      end
+
+      context 'without a when: key' do
+        let(:config) { { if: '$THIS || $THAT', start_in: '15 minutes' } }
+
+        it { is_expected.not_to be_valid }
+
+        it 'returns an error about the disallowed key' do
+          expect(entry.errors).to include(/disallowed keys: start_in/)
+        end
+      end
+
+      context 'without a start_in: key' do
+        let(:config) { { if: '$THIS || $THAT', when: 'delayed' } }
+
+        it { is_expected.not_to be_valid }
+
+        it 'returns an error about tstart_in being blank' do
+          expect(entry.errors).to include(/start in can't be blank/)
+        end
+      end
+    end
+
+    context 'when specifying unknown policy' do
+      let(:config) { { invalid: :something } }
+
+      it { is_expected.not_to be_valid }
+
+      it 'returns error about invalid key' do
+        expect(entry.errors).to include(/unknown keys: invalid/)
+      end
+    end
+
+    context 'when clause is empty' do
+      let(:config) { {} }
+
+      it { is_expected.not_to be_valid }
+
+      it 'is not a valid configuration' do
+        expect(entry.errors).to include(/can't be blank/)
+      end
+    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 a hash/)
+      end
+    end
+  end
+
+  describe '#value' do
+    subject { entry.value }
+
+    context 'when specifying an if: clause' do
+      let(:config) { { if: '$THIS || $THAT', when: 'manual' } }
+
+      it 'stores the expression as "if"' do
+        expect(subject).to eq(if: '$THIS || $THAT', when: 'manual')
+      end
+    end
+
+    context 'when using a changes: clause' do
+      let(:config) { { changes: %w[app/ lib/ spec/ other/* paths/**/*.rb] } }
+
+      it { is_expected.to eq(config) }
+    end
+
+    context 'when default value has been provided' do
+      let(:config) { { changes: %w[app/**/*.rb] } }
+
+      before do
+        entry.default = { changes: %w[**/*] }
+      end
+
+      it 'does not set a default value' do
+        expect(entry.default).to eq(nil)
+      end
+
+      it 'does not add to provided configuration' do
+        expect(entry.value).to eq(config)
+      end
+    end
+  end
+
+  describe '.default' do
+    it 'does not have default value' do
+      expect(described_class.default).to be_nil
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/config/entry/rules_spec.rb b/spec/lib/gitlab/ci/config/entry/rules_spec.rb
new file mode 100644
index 00000000000..291e7373daf
--- /dev/null
+++ b/spec/lib/gitlab/ci/config/entry/rules_spec.rb
@@ -0,0 +1,135 @@
+require 'fast_spec_helper'
+require 'support/helpers/stub_feature_flags'
+require_dependency 'active_model'
+
+describe Gitlab::Ci::Config::Entry::Rules do
+  let(:entry) { described_class.new(config) }
+
+  describe '.new' do
+    subject { entry }
+
+    context 'with a list of rule rule' do
+      let(:config) do
+        [{ if: '$THIS == "that"', when: 'never' }]
+      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.to be_valid }
+      end
+    end
+
+    context 'with a list of two rules' do
+      let(:config) do
+        [
+          { if: '$THIS == "that"', when: 'always' },
+          { if: '$SKIP',           when: 'never' }
+        ]
+      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.to be_valid }
+      end
+    end
+
+    context 'with a single rule object' do
+      let(:config) do
+        { if: '$SKIP', when: 'never' }
+      end
+
+      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
+    subject { entry.value }
+
+    context 'with a list of rule rule' do
+      let(:config) do
+        [{ if: '$THIS == "that"', when: 'never' }]
+      end
+
+      it { is_expected.to eq(config) }
+    end
+
+    context 'with a list of two rules' do
+      let(:config) do
+        [
+          { if: '$THIS == "that"', when: 'always' },
+          { if: '$SKIP',           when: 'never' }
+        ]
+      end
+
+      it { is_expected.to eq(config) }
+    end
+
+    context 'with a single rule object' do
+      let(:config) do
+        { if: '$SKIP', when: 'never' }
+      end
+
+      it { is_expected.to eq(config) }
+    end
+  end
+
+  describe '.default' do
+    it 'does not have default policy' do
+      expect(described_class.default).to be_nil
+    end
+  end
+end
-- 
cgit v1.2.1