Merge branch 'add-inheritable-mixin' into 'master'

Make `Job`, `Bridge` and `Default` to be truly inheritable

Closes #34518

See merge request gitlab-org/gitlab!18867

changelogs/unreleased/add-inheritable-mixin.yml

+---
+title: Make `Job`, `Bridge` and `Default` inheritable
+merge_request: 18867
+author:
+type: added

ee/lib/ee/gitlab/ci/config/entry/bridge.rb

@@ -12,6 +12,7 @@ module EE
           class Bridge < ::Gitlab::Config::Entry::Node
             include ::Gitlab::Config::Entry::Configurable
             include ::Gitlab::Config::Entry::Attributable
+            include ::Gitlab::Config::Entry::Inheritable
 
             ALLOWED_KEYS = %i[trigger stage allow_failure only except
                               when extends variables needs].freeze
@@ -37,23 +38,29 @@ module EE
             end
 
             entry :trigger, ::EE::Gitlab::Ci::Config::Entry::Trigger,
-              description: 'CI/CD Bridge downstream trigger definition.'
+              description: 'CI/CD Bridge downstream trigger definition.',
+              inherit: false
 
             entry :needs, ::EE::Gitlab::Ci::Config::Entry::Needs,
-              description: 'CI/CD Bridge needs dependency definition.'
+              description: 'CI/CD Bridge needs dependency definition.',
+              inherit: false
 
             entry :stage, ::Gitlab::Ci::Config::Entry::Stage,
-              description: 'Pipeline stage this job will be executed into.'
+              description: 'Pipeline stage this job will be executed into.',
+              inherit: false
 
             entry :only, ::Gitlab::Ci::Config::Entry::Policy,
               description: 'Refs policy this job will be executed for.',
-              default: ::Gitlab::Ci::Config::Entry::Policy::DEFAULT_ONLY
+              default: ::Gitlab::Ci::Config::Entry::Policy::DEFAULT_ONLY,
+              inherit: false
 
             entry :except, ::Gitlab::Ci::Config::Entry::Policy,
-              description: 'Refs policy this job will be executed for.'
+              description: 'Refs policy this job will be executed for.',
+              inherit: false
 
             entry :variables, ::Gitlab::Ci::Config::Entry::Variables,
-              description: 'Environment variables available for this job.'
+              description: 'Environment variables available for this job.',
+              inherit: false
 
             helpers(*ALLOWED_KEYS)
             attributes(*ALLOWED_KEYS)
@@ -85,6 +92,12 @@ module EE
                 only: only_value,
                 except: except_value }.compact
             end
+
+            private
+
+            def overwrite_entry(deps, key, current_entry)
+              deps.default[key] unless current_entry.specified?
+            end
           end
         end
       end

ee/spec/lib/ee/gitlab/ci/config/entry/bridge_spec.rb

@@ -3,6 +3,20 @@
 require 'spec_helper'
 
 describe EE::Gitlab::Ci::Config::Entry::Bridge do
+  subject { described_class.new(config, name: :my_bridge) }
+
+  it_behaves_like 'with inheritable CI config' do
+    let(:inheritable_key) { 'default' }
+    let(:inheritable_class) { Gitlab::Ci::Config::Entry::Default }
+
+    # These are entries defined in Default
+    # that we know that we don't want to inherit
+    # as they do not have sense in context of Bridge
+    let(:ignored_inheritable_columns) do
+      %i[before_script after_script image services cache]
+    end
+  end
+
   describe '.matching?' do
     subject { described_class.matching?(name, config) }
 
@@ -42,8 +56,6 @@ describe EE::Gitlab::Ci::Config::Entry::Bridge do
   end
 
   describe '.new' do
-    subject { described_class.new(config, name: :my_bridge) }
-
     before do
       subject.compose!
     end

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

@@ -11,8 +11,7 @@ module Gitlab
         #
         class Default < ::Gitlab::Config::Entry::Node
           include ::Gitlab::Config::Entry::Configurable
-
-          DuplicateError = Class.new(Gitlab::Config::Loader::FormatError)
+          include ::Gitlab::Config::Entry::Inheritable
 
           ALLOWED_KEYS = %i[before_script image services
                             after_script cache].freeze
@@ -43,29 +42,16 @@ module Gitlab
 
           helpers :before_script, :image, :services, :after_script, :cache
 
-          def compose!(deps = nil)
-            super(self)
-
-            inherit!(deps)
-          end
-
           private
 
-          def inherit!(deps)
-            return unless deps
+          def overwrite_entry(deps, key, current_entry)
+            inherited_entry = deps[key]
 
-            self.class.nodes.each do |key, factory|
-              next unless factory.inheritable?
-
-              root_entry = deps[key]
-              next unless root_entry.specified?
-
-              if self[key].specified?
-                raise DuplicateError, "#{key} is defined in top-level and `default:` entry"
-              end
-
-              @entries[key] = root_entry
+            if inherited_entry.specified? && current_entry.specified?
+              raise InheritError, "#{key} is defined in top-level and `default:` entry"
             end
+
+            inherited_entry unless current_entry.specified?
           end
         end
       end

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

@@ -10,6 +10,7 @@ module Gitlab
         class Job < ::Gitlab::Config::Entry::Node
           include ::Gitlab::Config::Entry::Configurable
           include ::Gitlab::Config::Entry::Attributable
+          include ::Gitlab::Config::Entry::Inheritable
 
           ALLOWED_WHEN = %w[on_success on_failure always manual delayed].freeze
           ALLOWED_KEYS = %i[tags script only except rules type image services
@@ -73,13 +74,16 @@ module Gitlab
             inherit: true
 
           entry :script, Entry::Commands,
-            description: 'Commands that will be executed in this job.'
+            description: 'Commands that will be executed in this job.',
+            inherit: false
 
           entry :stage, Entry::Stage,
-            description: 'Pipeline stage this job will be executed into.'
+            description: 'Pipeline stage this job will be executed into.',
+            inherit: false
 
           entry :type, Entry::Stage,
-            description: 'Deprecated: stage this job will be executed into.'
+            description: 'Deprecated: stage this job will be executed into.',
+            inherit: false
 
           entry :after_script, Entry::Script,
             description: 'Commands that will be executed when finishing job.',
@@ -99,28 +103,36 @@ module Gitlab
 
           entry :only, Entry::Policy,
             description: 'Refs policy this job will be executed for.',
-            default: Entry::Policy::DEFAULT_ONLY
+            default: Entry::Policy::DEFAULT_ONLY,
+            inherit: false
 
           entry :except, Entry::Policy,
-            description: 'Refs policy this job will be executed for.'
+            description: 'Refs policy this job will be executed for.',
+            inherit: false
 
           entry :rules, Entry::Rules,
-            description: 'List of evaluable Rules to determine job inclusion.'
+            description: 'List of evaluable Rules to determine job inclusion.',
+            inherit: false
 
           entry :variables, Entry::Variables,
-            description: 'Environment variables available for this job.'
+            description: 'Environment variables available for this job.',
+            inherit: false
 
           entry :artifacts, Entry::Artifacts,
-            description: 'Artifacts configuration for this job.'
+            description: 'Artifacts configuration for this job.',
+            inherit: false
 
           entry :environment, Entry::Environment,
-            description: 'Environment configuration for this job.'
+            description: 'Environment configuration for this job.',
+            inherit: false
 
           entry :coverage, Entry::Coverage,
-            description: 'Coverage configuration for this job.'
+            description: 'Coverage configuration for this job.',
+            inherit: false
 
           entry :retry, Entry::Retry,
-               description: 'Retry configuration for this job.'
+            description: 'Retry configuration for this job.',
+            inherit: false
 
           helpers :before_script, :script, :stage, :type, :after_script,
                   :cache, :image, :services, :only, :except, :variables,
@@ -155,8 +167,6 @@ module Gitlab
                 @entries.delete(:except)
               end
             end
-
-            inherit!(deps)
           end
 
           def name
@@ -185,21 +195,8 @@ module Gitlab
 
           private
 
-          # We inherit config entries from `default:`
-          # if the entry has the `inherit: true` flag set
-          def inherit!(deps)
-            return unless deps
-
-            self.class.nodes.each do |key, factory|
-              next unless factory.inheritable?
-
-              default_entry = deps.default[key]
-              job_entry = self[key]
-
-              if default_entry.specified? && !job_entry.specified?
-                @entries[key] = default_entry
-              end
-            end
+          def overwrite_entry(deps, key, current_entry)
+            deps.default[key] unless current_entry.specified?
           end
 
           def to_hash

lib/gitlab/config/entry/factory.rb

@@ -9,10 +9,12 @@ module Gitlab
       class Factory
         InvalidFactory = Class.new(StandardError)
 
-        def initialize(entry)
-          @entry = entry
+        attr_reader :entry_class
+
+        def initialize(entry_class)
+          @entry_class = entry_class
           @metadata = {}
-          @attributes = { default: entry.default }
+          @attributes = { default: entry_class.default }
         end
 
         def value(value)
@@ -34,6 +36,10 @@ module Gitlab
           @attributes[:description]
         end
 
+        def inherit
+          @attributes[:inherit]
+        end
+
         def inheritable?
           @attributes[:inherit]
         end
@@ -52,7 +58,7 @@ module Gitlab
           if @value.nil?
             Entry::Unspecified.new(fabricate_unspecified)
           else
-            fabricate(@entry, @value)
+            fabricate(entry_class, @value)
           end
         end
 
@@ -68,12 +74,12 @@ module Gitlab
           if default.nil?
             fabricate(Entry::Undefined)
           else
-            fabricate(@entry, default)
+            fabricate(entry_class, default)
           end
         end
 
-        def fabricate(entry, value = nil)
-          entry.new(value, @metadata) do |node|
+        def fabricate(entry_class, value = nil)
+          entry_class.new(value, @metadata) do |node|
             node.key = @attributes[:key]
             node.parent = @attributes[:parent]
             node.default = @attributes[:default]

lib/gitlab/config/entry/inheritable.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Config
+    module Entry
+      ##
+      # Entry that represents an inheritable configs.
+      #
+      module Inheritable
+        InheritError = Class.new(Gitlab::Config::Loader::FormatError)
+
+        def compose!(deps = nil, &blk)
+          super(deps, &blk)
+
+          inherit!(deps)
+        end
+
+        private
+
+        # We inherit config entries from `default:`
+        # if the entry has the `inherit: true` flag set
+        def inherit!(deps)
+          return unless deps
+
+          self.class.nodes.each do |key, factory|
+            next unless factory.inheritable?
+
+            new_entry = overwrite_entry(deps, key, self[key])
+
+            entries[key] = new_entry if new_entry&.specified?
+          end
+        end
+
+        def overwrite_entry(deps, key, current_entry)
+          raise NotImplementedError
+        end
+      end
+    end
+  end
+end

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

@@ -5,6 +5,18 @@ require 'spec_helper'
 describe Gitlab::Ci::Config::Entry::Default do
   let(:entry) { described_class.new(config) }
 
+  it_behaves_like 'with inheritable CI config' do
+    let(:inheritable_key) { nil }
+    let(:inheritable_class) { Gitlab::Ci::Config::Entry::Root }
+
+    # These are entries defined in Root
+    # that we know that we don't want to inherit
+    # as they do not have sense in context of Default
+    let(:ignored_inheritable_columns) do
+      %i[default include variables stages types]
+    end
+  end
+
   describe '.nodes' do
     it 'returns a hash' do
       expect(described_class.nodes).to be_a(Hash)
@@ -87,7 +99,7 @@ describe Gitlab::Ci::Config::Entry::Default do
 
       it 'raises error' do
         expect { entry.compose!(deps) }.to raise_error(
-          Gitlab::Ci::Config::Entry::Default::DuplicateError)
+          Gitlab::Ci::Config::Entry::Default::InheritError)
       end
     end
 

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

@@ -5,6 +5,18 @@ require 'spec_helper'
 describe Gitlab::Ci::Config::Entry::Job do
   let(:entry) { described_class.new(config, name: :rspec) }
 
+  it_behaves_like 'with inheritable CI config' do
+    let(:inheritable_key) { 'default' }
+    let(:inheritable_class) { Gitlab::Ci::Config::Entry::Default }
+
+    # These are entries defined in Default
+    # that we know that we don't want to inherit
+    # as they do not have sense in context of Job
+    let(:ignored_inheritable_columns) do
+      %i[]
+    end
+  end
+
   describe '.nodes' do
     context 'when filtering all the entry/node names' do
       subject { described_class.nodes.keys }

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

@@ -12,6 +12,11 @@ describe Gitlab::Ci::Config::Entry::Root do
 
     context 'when filtering all the entry/node names' do
       it 'contains the expected node names' do
+        # No inheritable fields should be added to the `Root`
+        #
+        # Inheritable configuration can only be added to `default:`
+        #
+        # The purpose of `Root` is have only globally defined configuration.
         expect(described_class.nodes.keys)
           .to match_array(%i[before_script image services
                              after_script variables cache

spec/support/shared_examples/lib/gitlab/config/inheritable_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'with inheritable CI config' do
+  using RSpec::Parameterized::TableSyntax
+
+  let(:ignored_inheritable_columns) { [] }
+
+  it 'does prepend an Inheritable mixin' do
+    expect(described_class).to include_module(Gitlab::Config::Entry::Inheritable)
+  end
+
+  it 'all inheritable entries are covered' do
+    inheritable_entries = inheritable_class.nodes.keys
+    entries = described_class.nodes.keys
+
+    expect(entries + ignored_inheritable_columns).to include(
+      *inheritable_entries)
+  end
+
+  it 'all entries do have inherit flag' do
+    without_inherit_flag = described_class.nodes.map do |key, factory|
+      key if factory.inherit.nil?
+    end.compact
+
+    expect(without_inherit_flag).to be_empty
+  end
+
+  context 'for non-inheritable entries' do
+    where(:entry_key) do
+      described_class.nodes.map do |key, factory|
+        [key] unless factory.inherit
+      end.compact
+    end
+
+    with_them do
+      it 'inheritable_class does not define entry' do
+        expect(inheritable_class.nodes).not_to include(entry_key)
+      end
+    end
+  end
+
+  context 'for inheritable entries' do
+    where(:entry_key, :entry_class) do
+      described_class.nodes.map do |key, factory|
+        [key, factory.entry_class] if factory.inherit
+      end.compact
+    end
+
+    with_them do
+      let(:specified) { double('deps_specified', 'specified?' => true, value: 'specified') }
+      let(:unspecified) { double('unspecified', 'specified?' => false) }
+      let(:inheritable) { double(inheritable_key, '[]' => unspecified) }
+
+      let(:deps) do
+        if inheritable_key
+          double('deps', inheritable_key => inheritable, '[]' => unspecified)
+        else
+          inheritable
+        end
+      end
+
+      it 'inheritable_class does define entry' do
+        expect(inheritable_class.nodes).to include(entry_key)
+        expect(inheritable_class.nodes[entry_key].entry_class).to eq(entry_class)
+      end
+
+      context 'when is specified' do
+        it 'does inherit value' do
+          expect(inheritable).to receive('[]').with(entry_key).and_return(specified)
+
+          entry.compose!(deps)
+
+          expect(entry[entry_key]).to eq(specified)
+        end
+
+        context 'when entry is specified' do
+          let(:entry_specified) do
+            double('entry_specified', 'specified?' => true, value: 'specified', errors: [])
+          end
+
+          it 'does not inherit value' do
+            entry.send(:entries)[entry_key] = entry_specified
+
+            allow(inheritable).to receive('[]').with(entry_key).and_return(specified)
+
+            expect do
+              # we ignore exceptions as `#overwrite_entry`
+              # can raise exception on duplicates
+              entry.compose!(deps) rescue described_class::InheritError
+            end.not_to change { entry[entry_key] }
+          end
+        end
+      end
+
+      context 'when inheritable does not specify' do
+        it 'does not inherit value' do
+          entry.compose!(deps)
+
+          expect(entry[entry_key]).to be_a(
+            Gitlab::Config::Entry::Undefined)
+        end
+      end
+    end
+  end
+end