Introduce Processable concern for config

The Job and Bridge share a lot of configuration.
To better align the configuration, the best is to
share them via Concern

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

@@ -9,23 +9,12 @@ module Gitlab
         # defining a downstream project trigger.
         #
         class Bridge < ::Gitlab::Config::Entry::Node
-          include ::Gitlab::Config::Entry::Configurable
-          include ::Gitlab::Config::Entry::Attributable
-          include ::Gitlab::Config::Entry::Inheritable
+          include ::Gitlab::Ci::Config::Entry::Processable
 
-          ALLOWED_KEYS = %i[trigger stage allow_failure only except
-                            when extends variables needs rules].freeze
+          ALLOWED_KEYS = %i[trigger allow_failure when variables needs].freeze
 
           validations do
-            validates :config, allowed_keys: ALLOWED_KEYS
-            validates :config, presence: true
-            validates :name, presence: true
-            validates :name, type: Symbol
-            validates :config, disallowed_keys: {
-                in: %i[only except when start_in],
-                message: 'key may not be used with `rules`'
-              },
-              if: :has_rules?
+            validates :config, allowed_keys: ALLOWED_KEYS + PROCESSABLE_ALLOWED_KEYS
 
             with_options allow_nil: true do
               validates :when,
@@ -62,22 +51,6 @@ module Gitlab
             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,
-            inherit: false
-
-          entry :except, ::Gitlab::Ci::Config::Entry::Policy,
-            description: 'Refs policy this job will be executed for.',
-            inherit: false
-
-          entry :rules, ::Gitlab::Ci::Config::Entry::Rules,
-            description: 'List of evaluable Rules to determine job inclusion.',
-            inherit: false,
-            metadata: {
-              allowed_when: %w[on_success on_failure always never manual delayed].freeze
-            }
-
           entry :variables, ::Gitlab::Ci::Config::Entry::Variables,
             description: 'Environment variables available for this job.',
             inherit: false
@@ -95,56 +68,20 @@ module Gitlab
             true
           end
 
-          def compose!(deps = nil)
-            super do
-              has_workflow_rules = deps&.workflow&.has_rules?
-
-              # If workflow:rules: or rules: are used
-              # they are considered not compatible
-              # with `only/except` defaults
-              #
-              # Context: https://gitlab.com/gitlab-org/gitlab/merge_requests/21742
-              if has_rules? || has_workflow_rules
-                # Remove only/except defaults
-                # defaults are not considered as defined
-                @entries.delete(:only) unless only_defined?
-                @entries.delete(:except) unless except_defined?
-              end
-            end
-          end
-
-          def has_rules?
-            @config&.key?(:rules)
-          end
-
-          def name
-            @metadata[:name]
-          end
-
           def value
-            { name: name,
+            super.merge(
               trigger: (trigger_value if trigger_defined?),
               needs: (needs_value if needs_defined?),
               ignore: !!allow_failure,
-              stage: stage_value,
               when: when_value,
-              extends: extends_value,
               variables: (variables_value if variables_defined?),
-              rules: (rules_value if has_rules?),
-              only: only_value,
-              except: except_value,
-              scheduling_type: needs_defined? && !bridge_needs ? :dag : :stage }.compact
+              scheduling_type: needs_defined? && !bridge_needs ? :dag : :stage
+            ).compact
           end
 
           def bridge_needs
             needs_value[:bridge] if needs_value
           end
-
-          private
-
-          def overwrite_entry(deps, key, current_entry)
-            deps.default[key] unless current_entry.specified?
-          end
         end
       end
     end

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

@@ -8,33 +8,21 @@ module Gitlab
         # Entry that represents a concrete CI/CD job.
         #
         class Job < ::Gitlab::Config::Entry::Node
-          include ::Gitlab::Config::Entry::Configurable
-          include ::Gitlab::Config::Entry::Attributable
-          include ::Gitlab::Config::Entry::Inheritable
+          include ::Gitlab::Ci::Config::Entry::Processable
 
           ALLOWED_WHEN = %w[on_success on_failure always manual delayed].freeze
-          ALLOWED_KEYS = %i[tags script only except rules type image services
-                            allow_failure type stage when start_in artifacts cache
+          ALLOWED_KEYS = %i[tags script type image services
+                            allow_failure type when start_in artifacts cache
                             dependencies before_script needs after_script variables
-                            environment coverage retry parallel extends interruptible timeout
+                            environment coverage retry parallel interruptible timeout
                             resource_group release].freeze
 
           REQUIRED_BY_NEEDS = %i[stage].freeze
 
           validations do
-            validates :config, type: Hash
-            validates :config, allowed_keys: ALLOWED_KEYS
+            validates :config, allowed_keys: ALLOWED_KEYS + PROCESSABLE_ALLOWED_KEYS
             validates :config, required_keys: REQUIRED_BY_NEEDS, if: :has_needs?
-            validates :config, presence: true
             validates :script, presence: true
-            validates :name, presence: true
-            validates :name, type: Symbol
-            validates :config,
-              disallowed_keys: {
-                in: %i[only except when start_in],
-                message: 'key may not be used with `rules`'
-              },
-              if: :has_rules?
             validates :config,
               disallowed_keys: {
                 in: %i[release],
@@ -53,8 +41,6 @@ module Gitlab
               }
 
               validates :dependencies, array_of_strings: true
-              validates :extends, array_of_strings_or_string: true
-              validates :rules, array_of_hashes: true
               validates :resource_group, type: String
             end
 
@@ -81,10 +67,6 @@ module Gitlab
             description: 'Commands that will be executed in this job.',
             inherit: false
 
-          entry :stage, Entry::Stage,
-            description: 'Pipeline stage this job will be executed into.',
-            inherit: false
-
           entry :type, Entry::Stage,
             description: 'Deprecated: stage this job will be executed into.',
             inherit: false
@@ -125,22 +107,6 @@ module Gitlab
             description: 'Artifacts configuration for this job.',
             inherit: true
 
-          entry :only, Entry::Policy,
-            description: 'Refs policy this job will be executed for.',
-            default: ::Gitlab::Ci::Config::Entry::Policy::DEFAULT_ONLY,
-            inherit: false
-
-          entry :except, Entry::Policy,
-            description: 'Refs policy this job will be executed for.',
-            inherit: false
-
-          entry :rules, Entry::Rules,
-            description: 'List of evaluable Rules to determine job inclusion.',
-            inherit: false,
-            metadata: {
-              allowed_when: %w[on_success on_failure always never manual delayed].freeze
-            }
-
           entry :needs, Entry::Needs,
             description: 'Needs configuration for this job.',
             metadata: { allowed_needs: %i[job cross_dependency] },
@@ -162,13 +128,13 @@ module Gitlab
             description: 'This job will produce a release.',
             inherit: false
 
-          helpers :before_script, :script, :stage, :type, :after_script,
-                  :cache, :image, :services, :only, :except, :variables,
-                  :artifacts, :environment, :coverage, :retry, :rules,
+          helpers :before_script, :script, :type, :after_script,
+                  :cache, :image, :services, :variables,
+                  :artifacts, :environment, :coverage, :retry,
                   :parallel, :needs, :interruptible, :release, :tags
 
           attributes :script, :tags, :allow_failure, :when, :dependencies,
-                     :needs, :retry, :parallel, :extends, :start_in, :rules,
+                     :needs, :retry, :parallel, :start_in,
                      :interruptible, :timeout, :resource_group, :release
 
           def self.matching?(name, config)
@@ -187,31 +153,9 @@ module Gitlab
               end
 
               @entries.delete(:type)
-
-              has_workflow_rules = deps&.workflow&.has_rules?
-
-              # If workflow:rules: or rules: are used
-              # they are considered not compatible
-              # with `only/except` defaults
-              #
-              # Context: https://gitlab.com/gitlab-org/gitlab/merge_requests/21742
-              if has_rules? || has_workflow_rules
-                # Remove only/except defaults
-                # defaults are not considered as defined
-                @entries.delete(:only) unless only_defined?
-                @entries.delete(:except) unless except_defined?
-              end
             end
           end
 
-          def name
-            @metadata[:name]
-          end
-
-          def value
-            @config.merge(to_hash.compact)
-          end
-
           def manual_action?
             self.when == 'manual'
           end
@@ -220,32 +164,18 @@ module Gitlab
             self.when == 'delayed'
           end
 
-          def has_rules?
-            @config.try(:key?, :rules)
-          end
-
           def ignored?
             allow_failure.nil? ? manual_action? : allow_failure
           end
 
-          private
-
-          def overwrite_entry(deps, key, current_entry)
-            deps.default[key] unless current_entry.specified?
-          end
-
-          def to_hash
-            { name: name,
+          def value
+            super.merge(
               before_script: before_script_value,
               script: script_value,
               image: image_value,
               services: services_value,
-              stage: stage_value,
               cache: cache_value,
               tags: tags_value,
-              only: only_value,
-              except: except_value,
-              rules: has_rules? ? rules_value : nil,
               variables: variables_defined? ? variables_value : {},
               environment: environment_defined? ? environment_value : nil,
               environment_name: environment_defined? ? environment_value[:name] : nil,
@@ -260,7 +190,8 @@ module Gitlab
               ignore: ignored?,
               needs: needs_defined? ? needs_value : nil,
               resource_group: resource_group,
-              scheduling_type: needs_defined? ? :dag : :stage }
+              scheduling_type: needs_defined? ? :dag : :stage
+            ).compact
           end
         end
       end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module Entry
+        ##
+        # Entry that represents a CI/CD Processable (a job)
+        #
+        module Processable
+          extend ActiveSupport::Concern
+
+          include ::Gitlab::Config::Entry::Configurable
+          include ::Gitlab::Config::Entry::Attributable
+          include ::Gitlab::Config::Entry::Inheritable
+
+          PROCESSABLE_ALLOWED_KEYS = %i[extends stage only except rules].freeze
+
+          included do
+            validations do
+              validates :config, presence: true
+              validates :name, presence: true
+              validates :name, type: Symbol
+
+              validates :config, disallowed_keys: {
+                  in: %i[only except when start_in],
+                  message: 'key may not be used with `rules`'
+                },
+                if: :has_rules?
+
+              with_options allow_nil: true do
+                validates :extends, array_of_strings_or_string: true
+                validates :rules, array_of_hashes: true
+              end
+            end
+
+            entry :stage, Entry::Stage,
+              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,
+              inherit: false
+
+            entry :except, ::Gitlab::Ci::Config::Entry::Policy,
+              description: 'Refs policy this job will be executed for.',
+              inherit: false
+
+            entry :rules, ::Gitlab::Ci::Config::Entry::Rules,
+              description: 'List of evaluable Rules to determine job inclusion.',
+              inherit: false,
+              metadata: {
+                allowed_when: %w[on_success on_failure always never manual delayed].freeze
+              }
+
+            helpers :stage, :only, :except, :rules, :extends
+
+            attributes :extends, :rules
+          end
+
+          def compose!(deps = nil)
+            super do
+              has_workflow_rules = deps&.workflow&.has_rules?
+
+              # If workflow:rules: or rules: are used
+              # they are considered not compatible
+              # with `only/except` defaults
+              #
+              # Context: https://gitlab.com/gitlab-org/gitlab/merge_requests/21742
+              if has_rules? || has_workflow_rules
+                # Remove only/except defaults
+                # defaults are not considered as defined
+                @entries.delete(:only) unless only_defined? # rubocop:disable Gitlab/ModuleWithInstanceVariables
+                @entries.delete(:except) unless except_defined? # rubocop:disable Gitlab/ModuleWithInstanceVariables
+              end
+            end
+          end
+
+          def name
+            metadata[:name]
+          end
+
+          def overwrite_entry(deps, key, current_entry)
+            deps.default[key] unless current_entry.specified?
+          end
+
+          def value
+            { name: name,
+              stage: stage_value,
+              extends: extends_value,
+              rules: (rules_value if has_rules?),
+              only: only_value,
+              except: except_value }.compact
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/config/entry/configurable.rb

@@ -88,6 +88,10 @@ module Gitlab
 
           def helpers(*nodes)
             nodes.each do |symbol|
+              if method_defined?("#{symbol}_defined?") || method_defined?("#{symbol}_value")
+                raise ArgumentError, "Method #{attribute}_defined? or #{attribute}_value already defined"
+              end
+
               define_method("#{symbol}_defined?") do
                 entries[symbol]&.specified?
               end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Entry::Processable do
+  let(:node_class) do
+    Class.new(::Gitlab::Config::Entry::Node) do
+      include Gitlab::Ci::Config::Entry::Processable
+
+      def self.name
+        'job'
+      end
+    end
+  end
+
+  let(:entry) { node_class.new(config, name: :rspec) }
+
+  describe 'validations' do
+    before do
+      entry.compose!
+    end
+
+    context 'when entry config value is correct' do
+      let(:config) { { stage: 'test' } }
+
+      describe '#valid?' do
+        it 'is valid' do
+          expect(entry).to be_valid
+        end
+      end
+
+      context 'when job name is empty' do
+        let(:entry) { node_class.new(config, name: ''.to_sym) }
+
+        it 'reports error' do
+          expect(entry.errors).to include "job name can't be blank"
+        end
+      end
+    end
+
+    context 'when entry value is not correct' do
+      context 'incorrect config value type' do
+        let(:config) { ['incorrect'] }
+
+        describe '#errors' do
+          it 'reports error about a config type' do
+            expect(entry.errors)
+              .to include 'job config should be a hash'
+          end
+        end
+      end
+
+      context 'when config is empty' do
+        let(:config) { {} }
+
+        describe '#valid' do
+          it 'is invalid' do
+            expect(entry).not_to be_valid
+          end
+        end
+      end
+
+      context 'when extends key is not a string' do
+        let(:config) { { extends: 123 } }
+
+        it 'returns error about wrong value type' do
+          expect(entry).not_to be_valid
+          expect(entry.errors).to include "job extends should be an array of strings or a string"
+        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
+
+      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
+    end
+  end
+
+  describe '#relevant?' do
+    it 'is a relevant entry' do
+      entry = node_class.new({ stage: 'test' }, name: :rspec)
+
+      expect(entry).to be_relevant
+    end
+  end
+
+  describe '#compose!' do
+    let(:specified) do
+      double('specified', 'specified?' => true, value: 'specified')
+    end
+
+    let(:unspecified) { double('unspecified', 'specified?' => false) }
+    let(:default) { double('default', '[]' => unspecified) }
+    let(:workflow) { double('workflow', 'has_rules?' => false) }
+    let(:deps) { double('deps', 'default' => default, '[]' => unspecified, 'workflow' => workflow) }
+
+    context 'with workflow rules' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:name, :has_workflow_rules?, :only, :rules, :result) do
+        "uses default only"    | false | nil          | nil    | { refs: %w[branches tags] }
+        "uses user only"       | false | %w[branches] | nil    | { refs: %w[branches] }
+        "does not define only" | false | nil          | []     | nil
+        "does not define only" | true  | nil          | nil    | nil
+        "uses user only"       | true  | %w[branches] | nil    | { refs: %w[branches] }
+        "does not define only" | true  | nil          | []     | nil
+      end
+
+      with_them do
+        let(:config) { { script: 'ls', rules: rules, only: only }.compact }
+
+        it "#{name}" do
+          expect(workflow).to receive(:has_rules?) { has_workflow_rules? }
+
+          entry.compose!(deps)
+
+          expect(entry.only_value).to eq(result)
+        end
+      end
+    end
+
+    context 'when workflow rules is used' do
+      context 'when rules are used' do
+        let(:config) { { script: 'ls', cache: { key: 'test' }, rules: [] } }
+
+        it 'does not define only' do
+          expect(entry).not_to be_only_defined
+        end
+      end
+
+      context 'when rules are not used' do
+        let(:config) { { script: 'ls', cache: { key: 'test' }, only: [] } }
+
+        it 'does not define only' do
+          expect(entry).not_to be_only_defined
+        end
+      end
+    end
+  end
+
+  context 'when composed' do
+    before do
+      entry.compose!
+    end
+
+    describe '#value' do
+      before do
+        entry.compose!
+      end
+
+      context 'when entry is correct' do
+        let(:config) do
+          { stage: 'test' }
+        end
+
+        it 'returns correct value' do
+          expect(entry.value)
+            .to eq(name: :rspec,
+                   stage: 'test',
+                   only: { refs: %w[branches tags] })
+        end
+      end
+    end
+  end
+end