Merge branch 'backstage/gb/refactor-only-except-policies-config' into 'master'

Refactor only/except configuration policies

See merge request gitlab-org/gitlab-ce!24359

13 files changed, 64 insertions, 39 deletions

diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb
index 3239743bfff..1d8904f7b29 100644
--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/lib/gitlab/ci/config/entry/job.rb
@@ -16,13 +16,6 @@ module Gitlab
                             dependencies before_script after_script variables
                             environment coverage retry parallel extends].freeze
 
-          DEFAULT_ONLY_POLICY = {
-            refs: %w(branches tags)
-          }.freeze
-
-          DEFAULT_EXCEPT_POLICY = {
-          }.freeze
-
           validations do
             validates :config, allowed_keys: ALLOWED_KEYS
             validates :config, presence: true
@@ -73,7 +66,8 @@ module Gitlab
             description: 'Services that will be used to execute this job.'
 
           entry :only, Entry::Policy,
-            description: 'Refs policy this job will be executed for.'
+            description: 'Refs policy this job will be executed for.',
+            default: { refs: %w[branches tags] }
 
           entry :except, Entry::Policy,
             description: 'Refs policy this job will be executed for.'
@@ -156,8 +150,8 @@ module Gitlab
               services: services_value,
               stage: stage_value,
               cache: cache_value,
-              only: DEFAULT_ONLY_POLICY.deep_merge(only_value.to_h),
-              except: DEFAULT_EXCEPT_POLICY.deep_merge(except_value.to_h),
+              only: only_value,
+              except: except_value,
               variables: variables_defined? ? variables_value : nil,
               environment: environment_defined? ? environment_value : nil,
               environment_name: environment_defined? ? environment_value[:name] : nil,
diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb
index 998da1f6837..9c677bf6617 100644
--- a/lib/gitlab/ci/config/entry/policy.rb
+++ b/lib/gitlab/ci/config/entry/policy.rb
@@ -64,7 +64,8 @@ module Gitlab
             end
           end
 
-          def self.default
+          def value
+            default.to_h.deep_merge(subject.value.to_h)
           end
         end
       end
diff --git a/lib/gitlab/ci/config/entry/retry.rb b/lib/gitlab/ci/config/entry/retry.rb
index eaf8b38aa3c..e9cbcb31e21 100644
--- a/lib/gitlab/ci/config/entry/retry.rb
+++ b/lib/gitlab/ci/config/entry/retry.rb
@@ -82,9 +82,6 @@ module Gitlab
               'retry config'
             end
           end
-
-          def self.default
-          end
         end
       end
     end
diff --git a/lib/gitlab/ci/config/entry/variables.rb b/lib/gitlab/ci/config/entry/variables.rb
index 89d790ebfa6..c9d0c7cb568 100644
--- a/lib/gitlab/ci/config/entry/variables.rb
+++ b/lib/gitlab/ci/config/entry/variables.rb
@@ -14,7 +14,7 @@ module Gitlab
             validates :config, variables: true
           end
 
-          def self.default
+          def self.default(**)
             {}
           end
 
diff --git a/lib/gitlab/config/entry/configurable.rb b/lib/gitlab/config/entry/configurable.rb
index afdb60b2cd5..37ba16dba25 100644
--- a/lib/gitlab/config/entry/configurable.rb
+++ b/lib/gitlab/config/entry/configurable.rb
@@ -56,6 +56,7 @@ module Gitlab
           def entry(key, entry, metadata)
             factory = ::Gitlab::Config::Entry::Factory.new(entry)
               .with(description: metadata[:description])
+              .with(default: metadata[:default])
 
             (@nodes ||= {}).merge!(key.to_sym => factory)
           end
diff --git a/lib/gitlab/config/entry/factory.rb b/lib/gitlab/config/entry/factory.rb
index 30d43c9f9a1..79f9ff32514 100644
--- a/lib/gitlab/config/entry/factory.rb
+++ b/lib/gitlab/config/entry/factory.rb
@@ -12,7 +12,7 @@ module Gitlab
         def initialize(entry)
           @entry = entry
           @metadata = {}
-          @attributes = {}
+          @attributes = { default: entry.default }
         end
 
         def value(value)
@@ -21,12 +21,12 @@ module Gitlab
         end
 
         def metadata(metadata)
-          @metadata.merge!(metadata)
+          @metadata.merge!(metadata.compact)
           self
         end
 
         def with(attributes)
-          @attributes.merge!(attributes)
+          @attributes.merge!(attributes.compact)
           self
         end
 
@@ -38,9 +38,7 @@ module Gitlab
           # See issue #18775.
           #
           if @value.nil?
-            Entry::Unspecified.new(
-              fabricate_unspecified
-            )
+            Entry::Unspecified.new(fabricate_unspecified)
           else
             fabricate(@entry, @value)
           end
@@ -53,10 +51,12 @@ module Gitlab
           # If entry has a default value we fabricate concrete node
           # with default value.
           #
-          if @entry.default.nil?
+          default = @attributes.fetch(:default)
+
+          if default.nil?
             fabricate(Entry::Undefined)
           else
-            fabricate(@entry, @entry.default)
+            fabricate(@entry, default)
           end
         end
 
@@ -64,6 +64,7 @@ module Gitlab
           entry.new(value, @metadata).tap do |node|
             node.key = @attributes[:key]
             node.parent = @attributes[:parent]
+            node.default = @attributes[:default]
             node.description = @attributes[:description]
           end
         end
diff --git a/lib/gitlab/config/entry/node.rb b/lib/gitlab/config/entry/node.rb
index 30357b2c95b..9999ab4ff95 100644
--- a/lib/gitlab/config/entry/node.rb
+++ b/lib/gitlab/config/entry/node.rb
@@ -10,7 +10,7 @@ module Gitlab
         InvalidError = Class.new(StandardError)
 
         attr_reader :config, :metadata
-        attr_accessor :key, :parent, :description
+        attr_accessor :key, :parent, :default, :description
 
         def initialize(config, **metadata)
           @config = config
@@ -85,7 +85,7 @@ module Gitlab
           "#<#{self.class.name} #{unspecified}{#{key}: #{val.inspect}}>"
         end
 
-        def self.default
+        def self.default(**)
         end
 
         def self.aspects
diff --git a/lib/gitlab/config/entry/simplifiable.rb b/lib/gitlab/config/entry/simplifiable.rb
index 3e148fe2e91..5fbf7565e2a 100644
--- a/lib/gitlab/config/entry/simplifiable.rb
+++ b/lib/gitlab/config/entry/simplifiable.rb
@@ -6,6 +6,8 @@ module Gitlab
       class Simplifiable < SimpleDelegator
         EntryStrategy = Struct.new(:name, :condition)
 
+        attr_reader :subject
+
         def initialize(config, **metadata)
           unless self.class.const_defined?(:UnknownStrategy)
             raise ArgumentError, 'UndefinedStrategy not available!'
@@ -17,7 +19,7 @@ module Gitlab
 
           entry = self.class.entry_class(strategy)
 
-          super(entry.new(config, metadata))
+          super(@subject = entry.new(config, metadata))
         end
 
         def self.strategy(name, **opts)
@@ -37,6 +39,9 @@ module Gitlab
             self::UnknownStrategy
           end
         end
+
+        def self.default
+        end
       end
     end
   end
diff --git a/spec/lib/gitlab/ci/config/entry/global_spec.rb b/spec/lib/gitlab/ci/config/entry/global_spec.rb
index 941ef33c8a4..7651f594a4c 100644
--- a/spec/lib/gitlab/ci/config/entry/global_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/global_spec.rb
@@ -160,8 +160,7 @@ describe Gitlab::Ci::Config::Entry::Global do
                        variables: { 'VAR' => 'value' },
                        ignore: false,
                        after_script: ['make clean'],
-                       only: { refs: %w[branches tags] },
-                       except: {} },
+                       only: { refs: %w[branches tags] } },
               spinach: { name: :spinach,
                          before_script: [],
                          script: %w[spinach],
@@ -172,8 +171,7 @@ describe Gitlab::Ci::Config::Entry::Global do
                          variables: {},
                          ignore: false,
                          after_script: ['make clean'],
-                         only: { refs: %w[branches tags] },
-                         except: {} }
+                         only: { refs: %w[branches tags] } }
             )
           end
         end
diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb
index 3d0b98eb238..0560eb42e4d 100644
--- a/spec/lib/gitlab/ci/config/entry/job_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb
@@ -258,8 +258,7 @@ describe Gitlab::Ci::Config::Entry::Job do
                    stage: 'test',
                    ignore: false,
                    after_script: %w[cleanup],
-                   only: { refs: %w[branches tags] },
-                   except: {})
+                   only: { refs: %w[branches tags] })
         end
       end
     end
diff --git a/spec/lib/gitlab/ci/config/entry/jobs_spec.rb b/spec/lib/gitlab/ci/config/entry/jobs_spec.rb
index d97be76f0e0..271ee30df3c 100644
--- a/spec/lib/gitlab/ci/config/entry/jobs_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/jobs_spec.rb
@@ -67,14 +67,12 @@ describe Gitlab::Ci::Config::Entry::Jobs do
                    script: %w[rspec],
                    ignore: false,
                    stage: 'test',
-                   only: { refs: %w[branches tags] },
-                   except: {} },
+                   only: { refs: %w[branches tags] } },
           spinach: { name: :spinach,
                      script: %w[spinach],
                      ignore: false,
                      stage: 'test',
-                     only: { refs: %w[branches tags] },
-                     except: {} })
+                     only: { refs: %w[branches tags] } })
       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 83001b7fdd8..1c987e13a9a 100644
--- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb
@@ -168,8 +168,33 @@ describe Gitlab::Ci::Config::Entry::Policy do
     end
   end
 
+  describe '#value' do
+    context 'when default value has been provided' do
+      before do
+        entry.default = { refs: %w[branches tags] }
+      end
+
+      context 'when user overrides default values' do
+        let(:config) { { refs: %w[feature], variables: %w[$VARIABLE] } }
+
+        it 'does not include default values' do
+          expect(entry.value).to eq config
+        end
+      end
+
+      context 'when default value has not been defined' do
+        let(:config) { { variables: %w[$VARIABLE] } }
+
+        it 'includes default values' do
+          expect(entry.value).to eq(refs: %w[branches tags],
+                                    variables: %w[$VARIABLE])
+        end
+      end
+    end
+  end
+
   describe '.default' do
-    it 'does not have a default value' do
+    it 'does not have default policy' do
       expect(described_class.default).to be_nil
     end
   end
diff --git a/spec/lib/gitlab/config/entry/configurable_spec.rb b/spec/lib/gitlab/config/entry/configurable_spec.rb
index 85a7cf1d241..37e38e49c0d 100644
--- a/spec/lib/gitlab/config/entry/configurable_spec.rb
+++ b/spec/lib/gitlab/config/entry/configurable_spec.rb
@@ -7,6 +7,10 @@ describe Gitlab::Config::Entry::Configurable do
     end
   end
 
+  before do
+    allow(entry).to receive(:default)
+  end
+
   describe 'validations' do
     context 'when entry is a hash' do
       let(:instance) { entry.new(key: 'value') }
@@ -26,9 +30,11 @@ describe Gitlab::Config::Entry::Configurable do
   end
 
   describe 'configured entries' do
+    let(:entry_class) { double('entry_class', default: nil) }
+
     before do
-      entry.class_eval do
-        entry :object, Object, description: 'test object'
+      entry.class_exec(entry_class) do |entry_class|
+        entry :object, entry_class, description: 'test object'
       end
     end