From 5ced2d8d7d9107f031894c5b16908db8bf6b913f Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 23 Aug 2017 15:09:20 +0200
Subject: [PATCH 01/15] Rename CI/CD job triggering policy class to Policy

---
 lib/gitlab/ci/config/entry/job.rb                             | 4 ++--
 lib/gitlab/ci/config/entry/{trigger.rb => policy.rb}          | 2 +-
 .../ci/config/entry/{trigger_spec.rb => policy_spec.rb}       | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)
 rename lib/gitlab/ci/config/entry/{trigger.rb => policy.rb} (91%)
 rename spec/lib/gitlab/ci/config/entry/{trigger_spec.rb => policy_spec.rb} (90%)

diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb
index 32f5c6ab142..91aac6df4b1 100644
--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/lib/gitlab/ci/config/entry/job.rb
@@ -59,10 +59,10 @@ module Gitlab
           entry :services, Entry::Services,
             description: 'Services that will be used to execute this job.'
 
-          entry :only, Entry::Trigger,
+          entry :only, Entry::Policy,
             description: 'Refs policy this job will be executed for.'
 
-          entry :except, Entry::Trigger,
+          entry :except, Entry::Policy,
             description: 'Refs policy this job will be executed for.'
 
           entry :variables, Entry::Variables,
diff --git a/lib/gitlab/ci/config/entry/trigger.rb b/lib/gitlab/ci/config/entry/policy.rb
similarity index 91%
rename from lib/gitlab/ci/config/entry/trigger.rb
rename to lib/gitlab/ci/config/entry/policy.rb
index 16b234e6c59..a08ab8a9d14 100644
--- a/lib/gitlab/ci/config/entry/trigger.rb
+++ b/lib/gitlab/ci/config/entry/policy.rb
@@ -5,7 +5,7 @@ module Gitlab
         ##
         # Entry that represents a trigger policy for the job.
         #
-        class Trigger < Node
+        class Policy < Node
           include Validatable
 
           validations do
diff --git a/spec/lib/gitlab/ci/config/entry/trigger_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
similarity index 90%
rename from spec/lib/gitlab/ci/config/entry/trigger_spec.rb
rename to spec/lib/gitlab/ci/config/entry/policy_spec.rb
index e4ee44f1274..ac57e3ef539 100644
--- a/spec/lib/gitlab/ci/config/entry/trigger_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::Ci::Config::Entry::Trigger do
+describe Gitlab::Ci::Config::Entry::Policy do
   let(:entry) { described_class.new(config) }
 
   describe 'validations' do
@@ -48,7 +48,7 @@ describe Gitlab::Ci::Config::Entry::Trigger do
       describe '#errors' do
         it 'saves errors' do
           expect(entry.errors)
-            .to include 'trigger config should be an array of strings or regexps'
+            .to include 'policy config should be an array of strings or regexps'
         end
       end
     end

From 0d7d7c1057c80e930d56363f2efd0519e1462586 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 24 Aug 2017 14:54:27 +0200
Subject: [PATCH 02/15] Use aspect-oriented design in CI/CD config entries

---
 lib/gitlab/ci/config/entry/configurable.rb    |  3 +-
 lib/gitlab/ci/config/entry/node.rb            | 11 ++---
 lib/gitlab/ci/config/entry/validatable.rb     | 11 +++++
 lib/gitlab/ci/config/entry/validator.rb       |  2 +-
 .../ci/config/entry/configurable_spec.rb      | 44 +++++++------------
 .../ci/config/entry/validatable_spec.rb       | 10 ++---
 6 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/configurable.rb b/lib/gitlab/ci/config/entry/configurable.rb
index e05aca9881b..68b6742385a 100644
--- a/lib/gitlab/ci/config/entry/configurable.rb
+++ b/lib/gitlab/ci/config/entry/configurable.rb
@@ -15,9 +15,10 @@ module Gitlab
         #
         module Configurable
           extend ActiveSupport::Concern
-          include Validatable
 
           included do
+            include Validatable
+
             validations do
               validates :config, type: Hash
             end
diff --git a/lib/gitlab/ci/config/entry/node.rb b/lib/gitlab/ci/config/entry/node.rb
index a6a914d79c1..2474684e07f 100644
--- a/lib/gitlab/ci/config/entry/node.rb
+++ b/lib/gitlab/ci/config/entry/node.rb
@@ -16,8 +16,9 @@ module Gitlab
             @metadata = metadata
             @entries = {}
 
-            @validator = self.class.validator.new(self)
-            @validator.validate(:new)
+            self.class.aspects.to_a.each do |aspect|
+              instance_exec(&aspect)
+            end
           end
 
           def [](key)
@@ -47,7 +48,7 @@ module Gitlab
           end
 
           def errors
-            @validator.messages + descendants.flat_map(&:errors)
+            []
           end
 
           def value
@@ -79,8 +80,8 @@ module Gitlab
           def self.default
           end
 
-          def self.validator
-            Validator
+          def self.aspects
+            @aspects ||= []
           end
         end
       end
diff --git a/lib/gitlab/ci/config/entry/validatable.rb b/lib/gitlab/ci/config/entry/validatable.rb
index f7f1b111571..5ced778d311 100644
--- a/lib/gitlab/ci/config/entry/validatable.rb
+++ b/lib/gitlab/ci/config/entry/validatable.rb
@@ -5,6 +5,17 @@ module Gitlab
         module Validatable
           extend ActiveSupport::Concern
 
+          def self.included(node)
+            node.aspects.append -> do
+              @validator = self.class.validator.new(self)
+              @validator.validate(:new)
+            end
+          end
+
+          def errors
+            @validator.messages + descendants.flat_map(&:errors)
+          end
+
           class_methods do
             def validator
               @validator ||= Class.new(Entry::Validator).tap do |validator|
diff --git a/lib/gitlab/ci/config/entry/validator.rb b/lib/gitlab/ci/config/entry/validator.rb
index 55343005fe3..5ab54d7e218 100644
--- a/lib/gitlab/ci/config/entry/validator.rb
+++ b/lib/gitlab/ci/config/entry/validator.rb
@@ -30,7 +30,7 @@ module Gitlab
 
           def key_name
             if key.blank?
-              @entry.class.name.demodulize.underscore.humanize
+              @entry.class.name.to_s.demodulize.underscore.humanize
             else
               key
             end
diff --git a/spec/lib/gitlab/ci/config/entry/configurable_spec.rb b/spec/lib/gitlab/ci/config/entry/configurable_spec.rb
index ae7e628b5b5..088d4b472da 100644
--- a/spec/lib/gitlab/ci/config/entry/configurable_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/configurable_spec.rb
@@ -1,40 +1,26 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::Entry::Configurable do
-  let(:entry) { Class.new }
-
-  before do
-    entry.include(described_class)
+  let(:entry) do
+    Class.new(Gitlab::Ci::Config::Entry::Node) do
+      include Gitlab::Ci::Config::Entry::Configurable
+    end
   end
 
   describe 'validations' do
-    let(:validator) { entry.validator.new(instance) }
-
-    before do
-      entry.class_eval do
-        attr_reader :config
-
-        def initialize(config)
-          @config = config
-        end
-      end
-
-      validator.validate
-    end
-
-    context 'when entry validator is invalid' do
-      let(:instance) { entry.new('ls') }
-
-      it 'returns invalid validator' do
-        expect(validator).to be_invalid
-      end
-    end
-
-    context 'when entry instance is valid' do
+    context 'when entry is a hash' do
       let(:instance) { entry.new(key: 'value') }
 
-      it 'returns valid validator' do
-        expect(validator).to be_valid
+      it 'correctly validates an instance' do
+        expect(instance).to be_valid
+      end
+    end
+
+    context 'when entry is not a hash' do
+      let(:instance) { entry.new('ls') }
+
+      it 'invalidates the instance' do
+        expect(instance).not_to be_valid
       end
     end
   end
diff --git a/spec/lib/gitlab/ci/config/entry/validatable_spec.rb b/spec/lib/gitlab/ci/config/entry/validatable_spec.rb
index d1856801827..ae2a7a51ba6 100644
--- a/spec/lib/gitlab/ci/config/entry/validatable_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/validatable_spec.rb
@@ -1,10 +1,10 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::Entry::Validatable do
-  let(:entry) { Class.new }
-
-  before do
-    entry.include(described_class)
+  let(:entry) do
+    Class.new(Gitlab::Ci::Config::Entry::Node) do
+      include Gitlab::Ci::Config::Entry::Validatable
+    end
   end
 
   describe '.validator' do
@@ -28,7 +28,7 @@ describe Gitlab::Ci::Config::Entry::Validatable do
     end
 
     context 'when validating entry instance' do
-      let(:entry_instance) { entry.new }
+      let(:entry_instance) { entry.new('something') }
 
       context 'when attribute is valid' do
         before do

From 8c409fc40ba4bf2e7fe0c8458fd2b59c09bd123a Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 25 Aug 2017 09:49:18 +0200
Subject: [PATCH 03/15] Make it possible to define CI/CD config strategies

---
 lib/gitlab/ci/config/entry/policy.rb          | 25 +++++-
 lib/gitlab/ci/config/entry/simplifiable.rb    | 31 ++++++++
 .../lib/gitlab/ci/config/entry/policy_spec.rb | 77 ++++++++++---------
 3 files changed, 93 insertions(+), 40 deletions(-)
 create mode 100644 lib/gitlab/ci/config/entry/simplifiable.rb

diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb
index a08ab8a9d14..ac564a6c7d0 100644
--- a/lib/gitlab/ci/config/entry/policy.rb
+++ b/lib/gitlab/ci/config/entry/policy.rb
@@ -5,11 +5,28 @@ module Gitlab
         ##
         # Entry that represents a trigger policy for the job.
         #
-        class Policy < Node
-          include Validatable
+        class Policy < Simplifiable
+          strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) }
+          strategy :ExpressionsPolicy, if: -> (config) { config.is_a?(Hash) }
 
-          validations do
-            validates :config, array_of_strings_or_regexps: true
+          class RefsPolicy < Entry::Node
+            include Entry::Validatable
+
+            validations do
+              validates :config, array_of_strings_or_regexps: true
+            end
+
+            def value
+              { refs: @config }
+            end
+          end
+
+          class ExpressionsPolicy < Entry::Node
+            include Entry::Validatable
+
+            validations do
+              validates :config, type: Hash
+            end
           end
         end
       end
diff --git a/lib/gitlab/ci/config/entry/simplifiable.rb b/lib/gitlab/ci/config/entry/simplifiable.rb
new file mode 100644
index 00000000000..5319065b846
--- /dev/null
+++ b/lib/gitlab/ci/config/entry/simplifiable.rb
@@ -0,0 +1,31 @@
+module Gitlab
+  module Ci
+    class Config
+      module Entry
+        class Simplifiable < SimpleDelegator
+          EntryStrategy = Struct.new(:name, :condition)
+
+          def initialize(config, **metadata)
+            strategy = self.class.strategies.find do |variant|
+              variant.condition.call(config)
+            end
+
+            entry = self.class.const_get(strategy.name)
+
+            super(entry.new(config, metadata))
+          end
+
+          def self.strategy(name, **opts)
+            EntryStrategy.new(name, opts.fetch(:if)).tap do |strategy|
+              (@strategies ||= []).append(strategy)
+            end
+          end
+
+          def self.strategies
+            @strategies || []
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
index ac57e3ef539..fe9a053c46c 100644
--- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
@@ -3,54 +3,59 @@ require 'spec_helper'
 describe Gitlab::Ci::Config::Entry::Policy do
   let(:entry) { described_class.new(config) }
 
-  describe 'validations' do
-    context 'when entry config value is valid' do
-      context 'when config is a branch or tag name' do
-        let(:config) { %w[master feature/branch] }
+  context 'when using simplified policy' do
+    describe 'validations' do
+      context 'when entry config value is valid' do
+        context 'when config is a branch or tag name' do
+          let(:config) { %w[master feature/branch] }
 
-        describe '#valid?' do
-          it 'is valid' do
-            expect(entry).to be_valid
+          describe '#valid?' do
+            it 'is valid' do
+              expect(entry).to be_valid
+            end
+          end
+
+          describe '#value' do
+            it 'returns key value' do
+              expect(entry.value).to eq(refs: config)
+            end
           end
         end
 
-        describe '#value' do
-          it 'returns key value' do
-            expect(entry.value).to eq config
+        context 'when config is a regexp' do
+          let(:config) { ['/^issue-.*$/'] }
+
+          describe '#valid?' do
+            it 'is valid' do
+              expect(entry).to be_valid
+            end
+          end
+        end
+
+        context 'when config is a special keyword' do
+          let(:config) { %w[tags triggers branches] }
+
+          describe '#valid?' do
+            it 'is valid' do
+              expect(entry).to be_valid
+            end
           end
         end
       end
 
-      context 'when config is a regexp' do
-        let(:config) { ['/^issue-.*$/'] }
+      context 'when entry value is not valid' do
+        let(:config) { [1] }
 
-        describe '#valid?' do
-          it 'is valid' do
-            expect(entry).to be_valid
+        describe '#errors' do
+          it 'saves errors' do
+            expect(entry.errors)
+              .to include /policy config should be an array of strings or regexps/
           end
         end
       end
-
-      context 'when config is a special keyword' do
-        let(:config) { %w[tags triggers branches] }
-
-        describe '#valid?' do
-          it 'is valid' do
-            expect(entry).to be_valid
-          end
-        end
-      end
-    end
-
-    context 'when entry value is not valid' do
-      let(:config) { [1] }
-
-      describe '#errors' do
-        it 'saves errors' do
-          expect(entry.errors)
-            .to include 'policy config should be an array of strings or regexps'
-        end
-      end
     end
   end
+
+  context 'when using complex policy' do
+  end
 end

From fcb4d1f80945cbcdfbdb73e102d046447f55e5b5 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 25 Aug 2017 10:27:00 +0200
Subject: [PATCH 04/15] Implement complex only/except policy CI/CD config

---
 lib/gitlab/ci/config/entry/policy.rb          | 20 ++++++++++++--
 lib/gitlab/ci/config/entry/simplifiable.rb    | 12 +++++++--
 .../lib/gitlab/ci/config/entry/policy_spec.rb | 26 +++++++++++++++++++
 3 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb
index ac564a6c7d0..decacebee55 100644
--- a/lib/gitlab/ci/config/entry/policy.rb
+++ b/lib/gitlab/ci/config/entry/policy.rb
@@ -3,7 +3,7 @@ module Gitlab
     class Config
       module Entry
         ##
-        # Entry that represents a trigger policy for the job.
+        # Entry that represents an only/except trigger policy for the job.
         #
         class Policy < Simplifiable
           strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) }
@@ -23,9 +23,25 @@ module Gitlab
 
           class ExpressionsPolicy < Entry::Node
             include Entry::Validatable
+            include Entry::Attributable
+
+            attributes :refs, :expressions
 
             validations do
-              validates :config, type: Hash
+              validates :config, presence: true
+              validates :config, allowed_keys: %i[refs expressions]
+
+              with_options allow_nil: true do
+                validates :refs, array_of_strings_or_regexps: true
+                validates :expressions, type: Array
+                validates :expressions, presence: true
+              end
+            end
+          end
+
+          class UnknownStrategy < Entry::Node
+            def errors
+              ['policy has to be either an array of conditions or a hash']
             end
           end
         end
diff --git a/lib/gitlab/ci/config/entry/simplifiable.rb b/lib/gitlab/ci/config/entry/simplifiable.rb
index 5319065b846..c26576fcbcd 100644
--- a/lib/gitlab/ci/config/entry/simplifiable.rb
+++ b/lib/gitlab/ci/config/entry/simplifiable.rb
@@ -10,7 +10,7 @@ module Gitlab
               variant.condition.call(config)
             end
 
-            entry = self.class.const_get(strategy.name)
+            entry = self.class.entry_class(strategy)
 
             super(entry.new(config, metadata))
           end
@@ -22,7 +22,15 @@ module Gitlab
           end
 
           def self.strategies
-            @strategies || []
+            @strategies.to_a
+          end
+
+          def self.entry_class(strategy)
+            if strategy.present?
+              self.const_get(strategy.name)
+            else
+              self::UnknownStrategy
+            end
           end
         end
       end
diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
index fe9a053c46c..69095144daa 100644
--- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
@@ -57,5 +57,31 @@ describe Gitlab::Ci::Config::Entry::Policy do
   end
 
   context 'when using complex policy' do
+    context 'when it is an empty hash' do
+      let(:config) { { } }
+
+      it 'reports an error about configuration not being present' do
+        expect(entry.errors).to include /can't be blank/
+      end
+    end
+
+    context 'when it contains unknown keys' do
+      let(:config) { { refs: ['something'], invalid: 'master' } }
+
+      it 'is not valid entry' do
+        expect(entry).not_to be_valid
+        expect(entry.errors)
+          .to include /policy config contains unknown keys: invalid/
+      end
+    end
+  end
+
+  context 'when policy strategy does not match' do
+    let(:config) { 'string strategy' }
+
+    it 'returns information about errors' do
+      expect(entry.errors)
+        .to include 'policy has to be either an array of conditions or a hash'
+    end
   end
 end

From 873460783a81ba7b89ca51c8fb6dfa5f6c2c51cc Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 25 Aug 2017 11:25:37 +0200
Subject: [PATCH 05/15] Add specs for a simplifiable CI/CD entry aspect

---
 .../ci/config/entry/simplifiable_spec.rb      | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 spec/lib/gitlab/ci/config/entry/simplifiable_spec.rb

diff --git a/spec/lib/gitlab/ci/config/entry/simplifiable_spec.rb b/spec/lib/gitlab/ci/config/entry/simplifiable_spec.rb
new file mode 100644
index 00000000000..5f9a0625b06
--- /dev/null
+++ b/spec/lib/gitlab/ci/config/entry/simplifiable_spec.rb
@@ -0,0 +1,75 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Entry::Simplifiable do
+  describe '.strategy' do
+    let(:entry) do
+      Class.new(described_class) do
+        strategy :Something, if: -> { 'condition' }
+        strategy :DifferentOne, if: -> { 'condition' }
+      end
+    end
+
+    it 'defines entry strategies' do
+      expect(entry.strategies.size).to eq 2
+      expect(entry.strategies.map(&:name))
+        .to eq %i[Something DifferentOne]
+    end
+  end
+
+  describe 'setting strategy by a condition' do
+    let(:first) { double('first strategy') }
+    let(:second) { double('second strategy') }
+    let(:unknown) { double('unknown strategy') }
+
+    before do
+      stub_const("#{described_class.name}::Something", first)
+      stub_const("#{described_class.name}::DifferentOne", second)
+      stub_const("#{described_class.name}::UnknownStrategy", unknown)
+    end
+
+    context 'when first strategy should be used' do
+      let(:entry) do
+        Class.new(described_class) do
+          strategy :Something, if: -> (arg) { arg == 'something' }
+          strategy :DifferentOne, if: -> (*) { false }
+        end
+      end
+
+      it 'attemps to load a first strategy' do
+        expect(first).to receive(:new).with('something', anything)
+
+        entry.new('something')
+      end
+    end
+
+    context 'when second strategy should be used' do
+      let(:entry) do
+        Class.new(described_class) do
+          strategy :Something, if: -> (arg) { arg == 'something' }
+          strategy :DifferentOne, if: -> (arg) { arg == 'test' }
+        end
+      end
+
+      it 'attemps to load a second strategy' do
+        expect(second).to receive(:new).with('test', anything)
+
+        entry.new('test')
+      end
+    end
+
+    context 'when neither one is a valid strategy' do
+      let(:entry) do
+        Class.new(described_class) do
+          strategy :Something, if: -> (*) { false }
+          strategy :DifferentOne, if: -> (*) { false }
+        end
+      end
+
+      it 'instantiates an unknown strategy' do
+        expect(unknown).to receive(:new).with('test', anything)
+
+        entry.new('test')
+      end
+    end
+  end
+end

From a061a2461a05200ab534d382b67e1b9099128478 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 25 Aug 2017 11:42:40 +0200
Subject: [PATCH 06/15] Fix CI/CD trigger policy default value

---
 lib/gitlab/ci/config/entry/policy.rb           | 3 +++
 spec/lib/gitlab/ci/config/entry/policy_spec.rb | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb
index decacebee55..41a3737b34a 100644
--- a/lib/gitlab/ci/config/entry/policy.rb
+++ b/lib/gitlab/ci/config/entry/policy.rb
@@ -44,6 +44,9 @@ module Gitlab
               ['policy has to be either an array of conditions or a hash']
             end
           end
+
+          def self.default
+          end
         end
       end
     end
diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
index 69095144daa..740b95427d3 100644
--- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
@@ -84,4 +84,10 @@ describe Gitlab::Ci::Config::Entry::Policy do
         .to include 'policy has to be either an array of conditions or a hash'
     end
   end
+
+  describe '.default' do
+    it 'does not have a default value' do
+      expect(described_class.default).to be_nil
+    end
+  end
 end

From 946e8d3a93eb8c9e30d1f3baa3b5b28e6c06fbc1 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 25 Aug 2017 12:01:59 +0200
Subject: [PATCH 07/15] Use only/except policy that returns an array

---
 lib/gitlab/ci/config/entry/policy.rb           | 4 ----
 spec/lib/gitlab/ci/config/entry/policy_spec.rb | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb
index 41a3737b34a..495822e9f5a 100644
--- a/lib/gitlab/ci/config/entry/policy.rb
+++ b/lib/gitlab/ci/config/entry/policy.rb
@@ -15,10 +15,6 @@ module Gitlab
             validations do
               validates :config, array_of_strings_or_regexps: true
             end
-
-            def value
-              { refs: @config }
-            end
           end
 
           class ExpressionsPolicy < Entry::Node
diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
index 740b95427d3..7f842c4b81b 100644
--- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
@@ -17,7 +17,7 @@ describe Gitlab::Ci::Config::Entry::Policy do
 
           describe '#value' do
             it 'returns key value' do
-              expect(entry.value).to eq(refs: config)
+              expect(entry.value).to eq config
             end
           end
         end

From 99dddac55850fed68ea6f66363c3f083bd4866fa Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 25 Aug 2017 13:00:45 +0200
Subject: [PATCH 08/15] Simplify ci config entry validator implementation

---
 lib/gitlab/ci/config/entry/validator.rb | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/validator.rb b/lib/gitlab/ci/config/entry/validator.rb
index 5ab54d7e218..83bca0d08bc 100644
--- a/lib/gitlab/ci/config/entry/validator.rb
+++ b/lib/gitlab/ci/config/entry/validator.rb
@@ -24,16 +24,11 @@ module Gitlab
           private
 
           def location
-            predecessors = ancestors.map(&:key).compact
-            predecessors.append(key_name).join(':')
+            ancestors.map(&:key).compact.append(key_name).join(':')
           end
 
           def key_name
-            if key.blank?
-              @entry.class.name.to_s.demodulize.underscore.humanize
-            else
-              key
-            end
+            key.presence || @entry.class.name.to_s.demodulize.underscore.humanize
           end
         end
       end

From 7e6bc4dde29af635c4ec281beea20ca87ccfbe34 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 25 Aug 2017 15:16:09 +0200
Subject: [PATCH 09/15] Improve reporting of a CI/CD entry config location

---
 lib/gitlab/ci/config/entry/attributable.rb    |  2 +
 lib/gitlab/ci/config/entry/node.rb            |  7 ++++
 lib/gitlab/ci/config/entry/policy.rb          |  2 +-
 lib/gitlab/ci/config/entry/validator.rb       | 11 -----
 spec/lib/ci/gitlab_ci_yaml_processor_spec.rb  | 41 +++++++++++--------
 .../lib/gitlab/ci/config/entry/policy_spec.rb |  2 +-
 .../gitlab/ci/config/entry/validator_spec.rb  |  2 +-
 7 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/attributable.rb b/lib/gitlab/ci/config/entry/attributable.rb
index 1c8b55ee4c4..24ff862a142 100644
--- a/lib/gitlab/ci/config/entry/attributable.rb
+++ b/lib/gitlab/ci/config/entry/attributable.rb
@@ -8,6 +8,8 @@ module Gitlab
           class_methods do
             def attributes(*attributes)
               attributes.flatten.each do |attribute|
+                raise ArgumentError if method_defined?(attribute)
+
                 define_method(attribute) do
                   return unless config.is_a?(Hash)
 
diff --git a/lib/gitlab/ci/config/entry/node.rb b/lib/gitlab/ci/config/entry/node.rb
index 2474684e07f..c868943c42e 100644
--- a/lib/gitlab/ci/config/entry/node.rb
+++ b/lib/gitlab/ci/config/entry/node.rb
@@ -71,6 +71,13 @@ module Gitlab
             true
           end
 
+          def location
+            name = @key.presence || self.class.name.to_s.demodulize
+                                        .underscore.humanize.downcase
+
+            ancestors.map(&:key).append(name).compact.join(':')
+          end
+
           def inspect
             val = leaf? ? config : descendants
             unspecified = specified? ? '' : '(unspecified) '
diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb
index 495822e9f5a..05602f1b3c6 100644
--- a/lib/gitlab/ci/config/entry/policy.rb
+++ b/lib/gitlab/ci/config/entry/policy.rb
@@ -37,7 +37,7 @@ module Gitlab
 
           class UnknownStrategy < Entry::Node
             def errors
-              ['policy has to be either an array of conditions or a hash']
+              ["#{location} has to be either an array of conditions or a hash"]
             end
           end
 
diff --git a/lib/gitlab/ci/config/entry/validator.rb b/lib/gitlab/ci/config/entry/validator.rb
index 83bca0d08bc..2df23a3edcd 100644
--- a/lib/gitlab/ci/config/entry/validator.rb
+++ b/lib/gitlab/ci/config/entry/validator.rb
@@ -8,7 +8,6 @@ module Gitlab
 
           def initialize(entry)
             super(entry)
-            @entry = entry
           end
 
           def messages
@@ -20,16 +19,6 @@ module Gitlab
           def self.name
             'Validator'
           end
-
-          private
-
-          def location
-            ancestors.map(&:key).compact.append(key_name).join(':')
-          end
-
-          def key_name
-            key.presence || @entry.class.name.to_s.demodulize.underscore.humanize
-          end
         end
       end
     end
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index ee28387cd48..fbfed263e15 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -344,28 +344,32 @@ module Ci
           let(:config) { { rspec: { script: "rspec", type: "test", only: only } } }
           let(:processor) { GitlabCiYamlProcessor.new(YAML.dump(config)) }
 
-          shared_examples 'raises an error' do
-            it do
-              expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, 'jobs:rspec:only config should be an array of strings or regexps')
-            end
-          end
 
           context 'when it is integer' do
             let(:only) { 1 }
 
-            it_behaves_like 'raises an error'
+            it do
+              expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError,
+                                                  'jobs:rspec:only has to be either an array of conditions or a hash')
+            end
           end
 
           context 'when it is an array of integers' do
             let(:only) { [1, 1] }
 
-            it_behaves_like 'raises an error'
+            it do
+              expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError,
+                                                  'jobs:rspec:only config should be an array of strings or regexps')
+            end
           end
 
           context 'when it is invalid regex' do
             let(:only) { ["/*invalid/"] }
 
-            it_behaves_like 'raises an error'
+            it do
+              expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError,
+                                                  'jobs:rspec:only config should be an array of strings or regexps')
+            end
           end
         end
       end
@@ -518,28 +522,31 @@ module Ci
           let(:config) { { rspec: { script: "rspec", except: except } } }
           let(:processor) { GitlabCiYamlProcessor.new(YAML.dump(config)) }
 
-          shared_examples 'raises an error' do
-            it do
-              expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, 'jobs:rspec:except config should be an array of strings or regexps')
-            end
-          end
-
           context 'when it is integer' do
             let(:except) { 1 }
 
-            it_behaves_like 'raises an error'
+            it do
+              expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError,
+                                                  'jobs:rspec:except has to be either an array of conditions or a hash')
+            end
           end
 
           context 'when it is an array of integers' do
             let(:except) { [1, 1] }
 
-            it_behaves_like 'raises an error'
+            it do
+              expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError,
+                                                  'jobs:rspec:except config should be an array of strings or regexps')
+            end
           end
 
           context 'when it is invalid regex' do
             let(:except) { ["/*invalid/"] }
 
-            it_behaves_like 'raises an error'
+            it do
+              expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError,
+                                                  'jobs:rspec:except config should be an array of strings or regexps')
+            end
           end
         end
       end
diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
index 7f842c4b81b..b3eba1382f2 100644
--- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
@@ -81,7 +81,7 @@ describe Gitlab::Ci::Config::Entry::Policy do
 
     it 'returns information about errors' do
       expect(entry.errors)
-        .to include 'policy has to be either an array of conditions or a hash'
+        .to include /has to be either an array of conditions or a hash/
     end
   end
 
diff --git a/spec/lib/gitlab/ci/config/entry/validator_spec.rb b/spec/lib/gitlab/ci/config/entry/validator_spec.rb
index ad7e6f07d3c..172b6b47a4f 100644
--- a/spec/lib/gitlab/ci/config/entry/validator_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/validator_spec.rb
@@ -48,7 +48,7 @@ describe Gitlab::Ci::Config::Entry::Validator do
         validator_instance.validate
 
         expect(validator_instance.messages)
-          .to include "node test attribute can't be blank"
+          .to include /test attribute can't be blank/
       end
     end
   end

From 4509594e206f93cdb54d80ae96fe7ad3d6a8fa4b Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 25 Aug 2017 17:54:14 +0200
Subject: [PATCH 10/15] Fix Rubocop offense in CI/CD only/except policy class

---
 spec/lib/gitlab/ci/config/entry/policy_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
index b3eba1382f2..ccf885969fb 100644
--- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
@@ -58,7 +58,7 @@ describe Gitlab::Ci::Config::Entry::Policy do
 
   context 'when using complex policy' do
     context 'when it is an empty hash' do
-      let(:config) { { } }
+      let(:config) { {} }
 
       it 'reports an error about configuration not being present' do
         expect(entry.errors).to include /can't be blank/

From 04108611716c0f1bb00092077ad5e31785675490 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 25 Aug 2017 18:26:55 +0200
Subject: [PATCH 11/15] Add specs for attributable aspect of ci config entry

---
 lib/gitlab/ci/config/entry/attributable.rb    |  4 ++-
 .../ci/config/entry/attributable_spec.rb      | 27 +++++++++++++++----
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/attributable.rb b/lib/gitlab/ci/config/entry/attributable.rb
index 24ff862a142..3e87a09704e 100644
--- a/lib/gitlab/ci/config/entry/attributable.rb
+++ b/lib/gitlab/ci/config/entry/attributable.rb
@@ -8,7 +8,9 @@ module Gitlab
           class_methods do
             def attributes(*attributes)
               attributes.flatten.each do |attribute|
-                raise ArgumentError if method_defined?(attribute)
+                if method_defined?(attribute)
+                  raise ArgumentError, 'Method already defined!'
+                end
 
                 define_method(attribute) do
                   return unless config.is_a?(Hash)
diff --git a/spec/lib/gitlab/ci/config/entry/attributable_spec.rb b/spec/lib/gitlab/ci/config/entry/attributable_spec.rb
index fde03c51e2c..b028b771375 100644
--- a/spec/lib/gitlab/ci/config/entry/attributable_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/attributable_spec.rb
@@ -1,18 +1,21 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::Entry::Attributable do
-  let(:node) { Class.new }
+  let(:node) do
+    Class.new do
+      include Gitlab::Ci::Config::Entry::Attributable
+    end
+  end
+
   let(:instance) { node.new }
 
   before do
-    node.include(described_class)
-
     node.class_eval do
       attributes :name, :test
     end
   end
 
-  context 'config is a hash' do
+  context 'when config is a hash' do
     before do
       allow(instance)
         .to receive(:config)
@@ -29,7 +32,7 @@ describe Gitlab::Ci::Config::Entry::Attributable do
     end
   end
 
-  context 'config is not a hash' do
+  context 'when config is not a hash' do
     before do
       allow(instance)
         .to receive(:config)
@@ -40,4 +43,18 @@ describe Gitlab::Ci::Config::Entry::Attributable do
       expect(instance.test).to be_nil
     end
   end
+
+  context 'when method is already defined in a superclass' do
+    it 'raises an error' do
+      expectation = expect do
+        Class.new(String) do
+          include Gitlab::Ci::Config::Entry::Attributable
+
+          attributes :length
+        end
+      end
+
+      expectation.to raise_error(ArgumentError, 'Method already defined!')
+    end
+  end
 end

From 6a5097fd247f5d6fe7fe4efac38d685e5ef190ae Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Sat, 26 Aug 2017 10:57:43 +0200
Subject: [PATCH 12/15] Fix rubocop offense in YAML processor specs

---
 spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index fbfed263e15..c70a4cb55fe 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -344,7 +344,6 @@ module Ci
           let(:config) { { rspec: { script: "rspec", type: "test", only: only } } }
           let(:processor) { GitlabCiYamlProcessor.new(YAML.dump(config)) }
 
-
           context 'when it is integer' do
             let(:only) { 1 }
 

From 808fb2549bf8b3dee58a4af81a4b6513a5016342 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 30 Aug 2017 13:20:55 +0200
Subject: [PATCH 13/15] Raise exception when simplifiable ci entry incomplete

---
 lib/gitlab/ci/config/entry/simplifiable.rb          |  4 ++++
 .../lib/gitlab/ci/config/entry/simplifiable_spec.rb | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/lib/gitlab/ci/config/entry/simplifiable.rb b/lib/gitlab/ci/config/entry/simplifiable.rb
index c26576fcbcd..0a03027736f 100644
--- a/lib/gitlab/ci/config/entry/simplifiable.rb
+++ b/lib/gitlab/ci/config/entry/simplifiable.rb
@@ -6,6 +6,10 @@ module Gitlab
           EntryStrategy = Struct.new(:name, :condition)
 
           def initialize(config, **metadata)
+            unless self.class.const_defined?(:UnknownStrategy)
+              raise ArgumentError, 'UndefinedStrategy not available!'
+            end
+
             strategy = self.class.strategies.find do |variant|
               variant.condition.call(config)
             end
diff --git a/spec/lib/gitlab/ci/config/entry/simplifiable_spec.rb b/spec/lib/gitlab/ci/config/entry/simplifiable_spec.rb
index 5f9a0625b06..395062207a3 100644
--- a/spec/lib/gitlab/ci/config/entry/simplifiable_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/simplifiable_spec.rb
@@ -72,4 +72,17 @@ describe Gitlab::Ci::Config::Entry::Simplifiable do
       end
     end
   end
+
+  context 'when a unknown strategy class is not defined' do
+    let(:entry) do
+      Class.new(described_class) do
+        strategy :String, if: -> (*) { true }
+      end
+    end
+
+    it 'raises an error when being initialized' do
+      expect { entry.new('something') }
+        .to raise_error ArgumentError, /UndefinedStrategy not available!/
+    end
+  end
 end

From 3474e109b73e0373a2663b5ae06db9aab61e1c02 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 30 Aug 2017 13:27:48 +0200
Subject: [PATCH 14/15] Simplify code for appending strategies in CI/CD config

---
 lib/gitlab/ci/config/entry/simplifiable.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/simplifiable.rb b/lib/gitlab/ci/config/entry/simplifiable.rb
index 0a03027736f..12764629686 100644
--- a/lib/gitlab/ci/config/entry/simplifiable.rb
+++ b/lib/gitlab/ci/config/entry/simplifiable.rb
@@ -21,12 +21,12 @@ module Gitlab
 
           def self.strategy(name, **opts)
             EntryStrategy.new(name, opts.fetch(:if)).tap do |strategy|
-              (@strategies ||= []).append(strategy)
+              strategies.append(strategy)
             end
           end
 
           def self.strategies
-            @strategies.to_a
+            @strategies ||= []
           end
 
           def self.entry_class(strategy)

From 1bf87d25241eb5e1a0f147d350814a3a1cfa1343 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 30 Aug 2017 14:47:54 +0200
Subject: [PATCH 15/15] Remove unused expressions policy from ci/cd config

---
 lib/gitlab/ci/config/entry/policy.rb          | 19 ------------------
 .../lib/gitlab/ci/config/entry/policy_spec.rb | 20 -------------------
 2 files changed, 39 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb
index 05602f1b3c6..3cdae1cee4f 100644
--- a/lib/gitlab/ci/config/entry/policy.rb
+++ b/lib/gitlab/ci/config/entry/policy.rb
@@ -7,7 +7,6 @@ module Gitlab
         #
         class Policy < Simplifiable
           strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) }
-          strategy :ExpressionsPolicy, if: -> (config) { config.is_a?(Hash) }
 
           class RefsPolicy < Entry::Node
             include Entry::Validatable
@@ -17,24 +16,6 @@ module Gitlab
             end
           end
 
-          class ExpressionsPolicy < Entry::Node
-            include Entry::Validatable
-            include Entry::Attributable
-
-            attributes :refs, :expressions
-
-            validations do
-              validates :config, presence: true
-              validates :config, allowed_keys: %i[refs expressions]
-
-              with_options allow_nil: true do
-                validates :refs, array_of_strings_or_regexps: true
-                validates :expressions, type: Array
-                validates :expressions, presence: true
-              end
-            end
-          end
-
           class UnknownStrategy < Entry::Node
             def errors
               ["#{location} has to be either an array of conditions or a hash"]
diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
index ccf885969fb..36a84da4a52 100644
--- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
@@ -56,26 +56,6 @@ describe Gitlab::Ci::Config::Entry::Policy do
     end
   end
 
-  context 'when using complex policy' do
-    context 'when it is an empty hash' do
-      let(:config) { {} }
-
-      it 'reports an error about configuration not being present' do
-        expect(entry.errors).to include /can't be blank/
-      end
-    end
-
-    context 'when it contains unknown keys' do
-      let(:config) { { refs: ['something'], invalid: 'master' } }
-
-      it 'is not valid entry' do
-        expect(entry).not_to be_valid
-        expect(entry.errors)
-          .to include /policy config contains unknown keys: invalid/
-      end
-    end
-  end
-
   context 'when policy strategy does not match' do
     let(:config) { 'string strategy' }