From 3ec80328fce8869e8dfc43520e8476cd55b938b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Fri, 21 Feb 2020 11:29:15 +0100 Subject: [PATCH 1/2] 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 | 75 +---- lib/gitlab/ci/config/entry/job.rb | 95 +------ lib/gitlab/ci/config/entry/processable.rb | 100 +++++++ lib/gitlab/config/entry/configurable.rb | 4 + .../ci/config/entry/processable_spec.rb | 269 ++++++++++++++++++ 5 files changed, 392 insertions(+), 151 deletions(-) create mode 100644 lib/gitlab/ci/config/entry/processable.rb create mode 100644 spec/lib/gitlab/ci/config/entry/processable_spec.rb diff --git a/lib/gitlab/ci/config/entry/bridge.rb b/lib/gitlab/ci/config/entry/bridge.rb index c0247dca73d882..bf1c7f2e6341eb 100644 --- a/lib/gitlab/ci/config/entry/bridge.rb +++ b/lib/gitlab/ci/config/entry/bridge.rb @@ -9,23 +9,12 @@ module Entry # 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 @@ class Bridge < ::Gitlab::Config::Entry::Node 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 @@ def self.visible? 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 diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb index 666c6e23eb4f7b..f46c0728f621a0 100644 --- a/lib/gitlab/ci/config/entry/job.rb +++ b/lib/gitlab/ci/config/entry/job.rb @@ -8,33 +8,21 @@ module Entry # 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 @@ class Job < ::Gitlab::Config::Entry::Node } 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 @@ class Job < ::Gitlab::Config::Entry::Node 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 @@ class Job < ::Gitlab::Config::Entry::Node 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 @@ class Job < ::Gitlab::Config::Entry::Node 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 @@ def compose!(deps = nil) 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 @@ def delayed? 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 @@ def to_hash 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 diff --git a/lib/gitlab/ci/config/entry/processable.rb b/lib/gitlab/ci/config/entry/processable.rb new file mode 100644 index 00000000000000..e6c6027d68479a --- /dev/null +++ b/lib/gitlab/ci/config/entry/processable.rb @@ -0,0 +1,100 @@ +# 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 diff --git a/lib/gitlab/config/entry/configurable.rb b/lib/gitlab/config/entry/configurable.rb index e7d441bb21c093..2498470134ba3d 100644 --- a/lib/gitlab/config/entry/configurable.rb +++ b/lib/gitlab/config/entry/configurable.rb @@ -88,6 +88,10 @@ def entry(key, entry, description: nil, default: nil, inherit: nil, reserved: ni 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 diff --git a/spec/lib/gitlab/ci/config/entry/processable_spec.rb b/spec/lib/gitlab/ci/config/entry/processable_spec.rb new file mode 100644 index 00000000000000..2bcfda23918418 --- /dev/null +++ b/spec/lib/gitlab/ci/config/entry/processable_spec.rb @@ -0,0 +1,269 @@ +# 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 -- GitLab From dcae09d1970dda79f6187e8dbf046791fea995c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Fri, 21 Feb 2020 12:36:07 +0100 Subject: [PATCH 2/2] Add additional entry validations Fix support for Bridge job when Fix support for dependencies --- lib/gitlab/ci/config/entry/bridge.rb | 15 +++++---------- lib/gitlab/ci/config/entry/job.rb | 7 +++++-- lib/gitlab/ci/config/entry/processable.rb | 8 +++++--- lib/gitlab/ci/config/entry/root.rb | 4 +++- lib/gitlab/config/entry/configurable.rb | 10 ++++++++-- .../gitlab/ci/config/entry/processable_spec.rb | 4 ---- spec/lib/gitlab/ci/yaml_processor_spec.rb | 4 +++- 7 files changed, 29 insertions(+), 23 deletions(-) diff --git a/lib/gitlab/ci/config/entry/bridge.rb b/lib/gitlab/ci/config/entry/bridge.rb index bf1c7f2e6341eb..721c7c8b6d7062 100644 --- a/lib/gitlab/ci/config/entry/bridge.rb +++ b/lib/gitlab/ci/config/entry/bridge.rb @@ -20,12 +20,10 @@ class Bridge < ::Gitlab::Config::Entry::Node validates :when, inclusion: { in: %w[on_success on_failure always], message: 'should be on_success, on_failure or always' } - validates :extends, type: String - validates :rules, array_of_hashes: true end validate on: :composed do - unless trigger.present? || bridge_needs.present? + unless trigger_defined? || bridge_needs.present? errors.add(:config, 'should contain either a trigger or a needs:pipeline') end end @@ -47,16 +45,13 @@ class Bridge < ::Gitlab::Config::Entry::Node inherit: false, metadata: { allowed_needs: %i[job bridge] } - entry :stage, ::Gitlab::Ci::Config::Entry::Stage, - description: 'Pipeline stage this job will be executed into.', - inherit: false - entry :variables, ::Gitlab::Ci::Config::Entry::Variables, description: 'Environment variables available for this job.', inherit: false - helpers(*ALLOWED_KEYS) - attributes(*ALLOWED_KEYS) + helpers :trigger, :needs, :variables + + attributes :when, :allow_failure def self.matching?(name, config) !name.to_s.start_with?('.') && @@ -73,7 +68,7 @@ def value trigger: (trigger_value if trigger_defined?), needs: (needs_value if needs_defined?), ignore: !!allow_failure, - when: when_value, + when: self.when, variables: (variables_value if variables_defined?), scheduling_type: needs_defined? && !bridge_needs ? :dag : :stage ).compact diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb index f46c0728f621a0..931f769e920b6b 100644 --- a/lib/gitlab/ci/config/entry/job.rb +++ b/lib/gitlab/ci/config/entry/job.rb @@ -131,7 +131,7 @@ class Job < ::Gitlab::Config::Entry::Node helpers :before_script, :script, :type, :after_script, :cache, :image, :services, :variables, :artifacts, :environment, :coverage, :retry, - :parallel, :needs, :interruptible, :release, :tags + :needs, :interruptible, :release, :tags attributes :script, :tags, :allow_failure, :when, :dependencies, :needs, :retry, :parallel, :start_in, @@ -176,12 +176,15 @@ def value services: services_value, cache: cache_value, tags: tags_value, + when: self.when, + start_in: self.start_in, + dependencies: dependencies, variables: variables_defined? ? variables_value : {}, environment: environment_defined? ? environment_value : nil, environment_name: environment_defined? ? environment_value[:name] : nil, coverage: coverage_defined? ? coverage_value : nil, retry: retry_defined? ? retry_value : nil, - parallel: parallel_defined? ? parallel_value.to_i : nil, + parallel: has_parallel? ? parallel.to_i : nil, interruptible: interruptible_defined? ? interruptible_value : nil, timeout: has_timeout? ? ChronicDuration.parse(timeout.to_s) : nil, artifacts: artifacts_value, diff --git a/lib/gitlab/ci/config/entry/processable.rb b/lib/gitlab/ci/config/entry/processable.rb index e6c6027d68479a..19e6601e31f4f7 100644 --- a/lib/gitlab/ci/config/entry/processable.rb +++ b/lib/gitlab/ci/config/entry/processable.rb @@ -54,7 +54,7 @@ module Processable allowed_when: %w[on_success on_failure always never manual delayed].freeze } - helpers :stage, :only, :except, :rules, :extends + helpers :stage, :only, :except, :rules attributes :extends, :rules end @@ -74,6 +74,8 @@ def compose!(deps = nil) @entries.delete(:only) unless only_defined? # rubocop:disable Gitlab/ModuleWithInstanceVariables @entries.delete(:except) unless except_defined? # rubocop:disable Gitlab/ModuleWithInstanceVariables end + + yield if block_given? end end @@ -88,8 +90,8 @@ def overwrite_entry(deps, key, current_entry) def value { name: name, stage: stage_value, - extends: extends_value, - rules: (rules_value if has_rules?), + extends: extends, + rules: rules_value, only: only_value, except: except_value }.compact end diff --git a/lib/gitlab/ci/config/entry/root.rb b/lib/gitlab/ci/config/entry/root.rb index 12dd942fc1c915..620f6a95e9dcf7 100644 --- a/lib/gitlab/ci/config/entry/root.rb +++ b/lib/gitlab/ci/config/entry/root.rb @@ -67,7 +67,9 @@ class Root < ::Gitlab::Config::Entry::Node entry :workflow, Entry::Workflow, description: 'List of evaluable rules to determine Pipeline status' - helpers :default, :jobs, :stages, :types, :variables, :workflow + helpers :default, :stages, :types, :variables, :workflow + + helpers :jobs, dynamic: true delegate :before_script_value, :image_value, diff --git a/lib/gitlab/config/entry/configurable.rb b/lib/gitlab/config/entry/configurable.rb index 2498470134ba3d..75e15cd8cb1d36 100644 --- a/lib/gitlab/config/entry/configurable.rb +++ b/lib/gitlab/config/entry/configurable.rb @@ -75,6 +75,8 @@ def reserved_node_names # rubocop: disable CodeReuse/ActiveRecord def entry(key, entry, description: nil, default: nil, inherit: nil, reserved: nil, metadata: {}) + raise ArgumentError, "Entry #{key} already defined" if @nodes.to_h[key.to_sym] + factory = ::Gitlab::Config::Entry::Factory.new(entry) .with(description: description) .with(default: default) @@ -86,10 +88,14 @@ def entry(key, entry, description: nil, default: nil, inherit: nil, reserved: ni end # rubocop: enable CodeReuse/ActiveRecord - def helpers(*nodes) + def helpers(*nodes, dynamic: false) nodes.each do |symbol| if method_defined?("#{symbol}_defined?") || method_defined?("#{symbol}_value") - raise ArgumentError, "Method #{attribute}_defined? or #{attribute}_value already defined" + raise ArgumentError, "Method #{symbol}_defined? or #{symbol}_value already defined" + end + + unless @nodes.to_h[symbol] + raise ArgumentError, "Entry for #{symbol} is undefined" unless dynamic end define_method("#{symbol}_defined?") do diff --git a/spec/lib/gitlab/ci/config/entry/processable_spec.rb b/spec/lib/gitlab/ci/config/entry/processable_spec.rb index 2bcfda23918418..410aef1cd53470 100644 --- a/spec/lib/gitlab/ci/config/entry/processable_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/processable_spec.rb @@ -248,10 +248,6 @@ def self.name end describe '#value' do - before do - entry.compose! - end - context 'when entry is correct' do let(:config) do { stage: 'test' } diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index cfc3d852de0dc3..e303557bd00be6 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -2419,7 +2419,9 @@ module Ci it 'returns errors and empty configuration' do expect(subject.valid?).to eq(false) - expect(subject.errors).to eq(['jobs:rspec config contains unknown keys: bad_tags', 'jobs:rspec rules should be an array of hashes']) + expect(subject.errors).to contain_exactly( + 'jobs:rspec config contains unknown keys: bad_tags', + 'jobs:rspec rules should be an array of hashes') expect(subject.content).to be_blank end end -- GitLab