diff --git a/lib/gitlab/ci/config/entry/bridge.rb b/lib/gitlab/ci/config/entry/bridge.rb index c0247dca73d882807d3bf63164d4692dface67ee..721c7c8b6d706284f8b9a9a2d72f91f50df98a7c 100644 --- a/lib/gitlab/ci/config/entry/bridge.rb +++ b/lib/gitlab/ci/config/entry/bridge.rb @@ -9,34 +9,21 @@ 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, 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 @@ -58,32 +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 :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 - helpers(*ALLOWED_KEYS) - attributes(*ALLOWED_KEYS) + helpers :trigger, :needs, :variables + + attributes :when, :allow_failure def self.matching?(name, config) !name.to_s.start_with?('.') && @@ -95,56 +63,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, + when: self.when, 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 666c6e23eb4f7b10ab63212dc83eb755d72cac4d..931f769e920b6bb3c2691a092e607ff9833da83e 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, - :parallel, :needs, :interruptible, :release, :tags + helpers :before_script, :script, :type, :after_script, + :cache, :image, :services, :variables, + :artifacts, :environment, :coverage, :retry, + :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,38 +164,27 @@ 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, + 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, @@ -260,7 +193,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 0000000000000000000000000000000000000000..19e6601e31f4f702ea54c649eb8e82654b235d8a --- /dev/null +++ b/lib/gitlab/ci/config/entry/processable.rb @@ -0,0 +1,102 @@ +# 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 + + 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 + + yield if block_given? + 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, + rules: rules_value, + only: only_value, + except: except_value }.compact + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/entry/root.rb b/lib/gitlab/ci/config/entry/root.rb index 12dd942fc1c915ae8778de50f90d667e17a18584..620f6a95e9dcf72e170d548e64e7788166f4d386 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 e7d441bb21c0933d50e5167836cc5c53f82fd8b9..75e15cd8cb1d362f86e397db6c7492b77e3bee4a 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,8 +88,16 @@ 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 #{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 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 0000000000000000000000000000000000000000..410aef1cd53470a9d95d3c370f1f7af54fee4152 --- /dev/null +++ b/spec/lib/gitlab/ci/config/entry/processable_spec.rb @@ -0,0 +1,265 @@ +# 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 + 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 diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index cfc3d852de0dc3205319ae38cb2c034c7f81fa3f..e303557bd00be6b030c5e33c6ca48c703b1854a3 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