Commit f220c7fc authored by Kamil Trzciński's avatar Kamil Trzciński Committed by Sean McGivern
Browse files

Make `Job`, `Bridge` and `Default` inheritable

This changes `Job`, `Bridge` and `Default`
to use `Inheritable` to inherit the configuration.

Shared example ensure that all fields are annotated
with `inherit:` and does have matching type when inherited.

Since the `Default:` is shared, it requires that
inheritable fields to be also added to `Job` and `Bridge`.
parent e4293eb1
---
title: Make `Job`, `Bridge` and `Default` inheritable
merge_request: 18867
author:
type: added
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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]
......
# 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
......@@ -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
 
......
......@@ -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 }
......
......@@ -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
......
# 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
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment