From 5b9a6ca00a023493c750f9d4a3b06729fdc96fff Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 16 Aug 2018 12:25:13 +0200 Subject: [PATCH 01/21] Add basic support for CI/CD config extension --- lib/gitlab/ci/config/extendable.rb | 31 ++++++++++ spec/lib/gitlab/ci/config/extendable_spec.rb | 63 ++++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 lib/gitlab/ci/config/extendable.rb create mode 100644 spec/lib/gitlab/ci/config/extendable_spec.rb diff --git a/lib/gitlab/ci/config/extendable.rb b/lib/gitlab/ci/config/extendable.rb new file mode 100644 index 00000000000..dea0dbdb44b --- /dev/null +++ b/lib/gitlab/ci/config/extendable.rb @@ -0,0 +1,31 @@ +module Gitlab + module Ci + class Config + class Extendable + include Enumerable + + ExtensionError = Class.new(StandardError) + + def initialize(hash) + @hash = hash + end + + def each + @hash.each_pair do |key, value| + next unless value.key?(:extends) + + yield key, value.fetch(:extends).to_sym, value + end + end + + def extend! + @hash.tap do + each do |key, extends, value| + @hash[key] = @hash.fetch(extends).deep_merge(value) + end + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/config/extendable_spec.rb b/spec/lib/gitlab/ci/config/extendable_spec.rb new file mode 100644 index 00000000000..a23fe560202 --- /dev/null +++ b/spec/lib/gitlab/ci/config/extendable_spec.rb @@ -0,0 +1,63 @@ +require 'fast_spec_helper' + +describe Gitlab::Ci::Config::Extendable do + subject { described_class.new(hash) } + + describe '#each' do + context 'when there is extendable entry in the hash' do + let(:test) do + { extends: 'something', only: %w[master] } + end + + let(:hash) do + { something: { script: 'ls' }, test: test } + end + + it 'yields the test hash' do + expect { |b| subject.each(&b) } + .to yield_with_args(:test, :something, test) + end + end + + pending 'when not extending using a hash' + end + + describe '#extend!' do + context 'when a hash has a single simple extension' do + let(:hash) do + { + something: { + script: 'deploy', + only: { variables: %w[$SOMETHING] } + }, + test: { + extends: 'something', + script: 'ls', + only: { refs: %w[master] } + } + } + end + + it 'extends a hash with reverse merge' do + expect(subject.extend!).to eq( + something: { + script: 'deploy', + only: { variables: %w[$SOMETHING] } + }, + test: { + extends: 'something', + script: 'ls', + only: { + refs: %w[master], + variables: %w[$SOMETHING] + } + } + ) + end + end + + pending 'when a hash recursive extensions' + + pending 'when invalid `extends` is specified' + end +end From 58414c143f928aa229ae871541cd35df293d6f54 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 16 Aug 2018 14:48:38 +0200 Subject: [PATCH 02/21] Support recursive `extends:` in `.gitlab-ci.yml` --- lib/gitlab/ci/config/extendable.rb | 31 ----- lib/gitlab/ci/config/extendable/collection.rb | 34 +++++ lib/gitlab/ci/config/extendable/entry.rb | 51 ++++++++ .../ci/config/extendable/collection_spec.rb | 122 ++++++++++++++++++ spec/lib/gitlab/ci/config/extendable_spec.rb | 63 --------- 5 files changed, 207 insertions(+), 94 deletions(-) delete mode 100644 lib/gitlab/ci/config/extendable.rb create mode 100644 lib/gitlab/ci/config/extendable/collection.rb create mode 100644 lib/gitlab/ci/config/extendable/entry.rb create mode 100644 spec/lib/gitlab/ci/config/extendable/collection_spec.rb delete mode 100644 spec/lib/gitlab/ci/config/extendable_spec.rb diff --git a/lib/gitlab/ci/config/extendable.rb b/lib/gitlab/ci/config/extendable.rb deleted file mode 100644 index dea0dbdb44b..00000000000 --- a/lib/gitlab/ci/config/extendable.rb +++ /dev/null @@ -1,31 +0,0 @@ -module Gitlab - module Ci - class Config - class Extendable - include Enumerable - - ExtensionError = Class.new(StandardError) - - def initialize(hash) - @hash = hash - end - - def each - @hash.each_pair do |key, value| - next unless value.key?(:extends) - - yield key, value.fetch(:extends).to_sym, value - end - end - - def extend! - @hash.tap do - each do |key, extends, value| - @hash[key] = @hash.fetch(extends).deep_merge(value) - end - end - end - end - end - end -end diff --git a/lib/gitlab/ci/config/extendable/collection.rb b/lib/gitlab/ci/config/extendable/collection.rb new file mode 100644 index 00000000000..4ffc4d91c82 --- /dev/null +++ b/lib/gitlab/ci/config/extendable/collection.rb @@ -0,0 +1,34 @@ +module Gitlab + module Ci + class Config + module Extendable + class Collection + include Enumerable + + ExtensionError = Class.new(StandardError) + + def initialize(hash, context = hash) + @hash = hash + @context = context + end + + def each + @hash.each_pair do |key, value| + next unless value.key?(:extends) + + yield Extendable::Entry.new(key, value, @context) + end + end + + def extend! + each do |entry| + raise ExtensionError unless entry.valid? + + @hash[entry.key] = entry.extend! + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/extendable/entry.rb b/lib/gitlab/ci/config/extendable/entry.rb new file mode 100644 index 00000000000..a39fa127e80 --- /dev/null +++ b/lib/gitlab/ci/config/extendable/entry.rb @@ -0,0 +1,51 @@ +module Gitlab + module Ci + class Config + module Extendable + class Entry + attr_reader :key + + def initialize(key, value, context, parent = nil) + @key = key + @value = value + @context = context + @parent = parent + end + + def valid? + true + end + + # def circular_dependency? + # @extends.to_s == @key.to_s + # end + + def base + Extendable::Entry + .new(extends, @context.fetch(extends), @context, self) + .extend! + end + + def extensible? + @value.key?(:extends) + end + + def extends + @value.fetch(:extends).to_sym + end + + def extend! + if extensible? + original = @value.dup + parent = base.dup + + @value.clear.deep_merge!(parent).deep_merge!(original) + else + @value.to_h + end + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb new file mode 100644 index 00000000000..453c7357561 --- /dev/null +++ b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb @@ -0,0 +1,122 @@ +require 'fast_spec_helper' + +describe Gitlab::Ci::Config::Extendable::Collection do + subject { described_class.new(hash) } + + describe '#each' do + context 'when there is extendable entry in the hash' do + let(:test) do + { extends: 'something', only: %w[master] } + end + + let(:hash) do + { something: { script: 'ls' }, test: test } + end + + it 'yields the test hash' do + expect { |b| subject.each(&b) }.to yield_control + end + end + + context 'when not extending using a hash' do + let(:hash) do + { something: { extends: [1], script: 'ls' } } + end + + it 'yields invalid extends as well' do + expect { |b| subject.each(&b) }.to yield_control + end + end + end + + describe '#extend!' do + context 'when a hash has a single simple extension' do + let(:hash) do + { + something: { + script: 'deploy', + only: { variables: %w[$SOMETHING] } + }, + + test: { + extends: 'something', + script: 'ls', + only: { refs: %w[master] } + } + } + end + + it 'extends a hash with a deep reverse merge' do + expect(subject.extend!).to eq( + something: { + script: 'deploy', + only: { variables: %w[$SOMETHING] } + }, + + test: { + extends: 'something', + script: 'ls', + only: { + refs: %w[master], + variables: %w[$SOMETHING] + } + } + ) + end + end + + context 'when a hash uses recursive extensions' do + let(:hash) do + { + test: { + extends: 'something', + script: 'ls', + only: { refs: %w[master] } + }, + + something: { + extends: '.first', + script: 'deploy', + only: { variables: %w[$SOMETHING] } + }, + + '.first': { + script: 'run', + only: { kubernetes: 'active' } + } + } + end + + it 'extends a hash with a deep reverse merge' do + expect(subject.extend!).to eq( + '.first': { + script: 'run', + only: { kubernetes: 'active' } + }, + + something: { + extends: '.first', + script: 'deploy', + only: { + kubernetes: 'active', + variables: %w[$SOMETHING] + } + }, + + test: { + extends: 'something', + script: 'ls', + only: { + refs: %w[master], + variables: %w[$SOMETHING], + kubernetes: 'active' + } + } + ) + end + end + + pending 'when invalid `extends` is specified' + pending 'when circular dependecy has been detected' + end +end diff --git a/spec/lib/gitlab/ci/config/extendable_spec.rb b/spec/lib/gitlab/ci/config/extendable_spec.rb deleted file mode 100644 index a23fe560202..00000000000 --- a/spec/lib/gitlab/ci/config/extendable_spec.rb +++ /dev/null @@ -1,63 +0,0 @@ -require 'fast_spec_helper' - -describe Gitlab::Ci::Config::Extendable do - subject { described_class.new(hash) } - - describe '#each' do - context 'when there is extendable entry in the hash' do - let(:test) do - { extends: 'something', only: %w[master] } - end - - let(:hash) do - { something: { script: 'ls' }, test: test } - end - - it 'yields the test hash' do - expect { |b| subject.each(&b) } - .to yield_with_args(:test, :something, test) - end - end - - pending 'when not extending using a hash' - end - - describe '#extend!' do - context 'when a hash has a single simple extension' do - let(:hash) do - { - something: { - script: 'deploy', - only: { variables: %w[$SOMETHING] } - }, - test: { - extends: 'something', - script: 'ls', - only: { refs: %w[master] } - } - } - end - - it 'extends a hash with reverse merge' do - expect(subject.extend!).to eq( - something: { - script: 'deploy', - only: { variables: %w[$SOMETHING] } - }, - test: { - extends: 'something', - script: 'ls', - only: { - refs: %w[master], - variables: %w[$SOMETHING] - } - } - ) - end - end - - pending 'when a hash recursive extensions' - - pending 'when invalid `extends` is specified' - end -end From ef26622d62fe37371adf0d66c81f8428ad4bb1b6 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 16 Aug 2018 15:01:27 +0200 Subject: [PATCH 03/21] Do not modify extensible CI/CD entries by reference --- lib/gitlab/ci/config/extendable/collection.rb | 7 +++--- lib/gitlab/ci/config/extendable/entry.rb | 24 ++++++++----------- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/lib/gitlab/ci/config/extendable/collection.rb b/lib/gitlab/ci/config/extendable/collection.rb index 4ffc4d91c82..3a71e06e3e2 100644 --- a/lib/gitlab/ci/config/extendable/collection.rb +++ b/lib/gitlab/ci/config/extendable/collection.rb @@ -7,16 +7,15 @@ module Gitlab ExtensionError = Class.new(StandardError) - def initialize(hash, context = hash) + def initialize(hash) @hash = hash - @context = context end def each @hash.each_pair do |key, value| next unless value.key?(:extends) - yield Extendable::Entry.new(key, value, @context) + yield Extendable::Entry.new(key, @hash) end end @@ -24,7 +23,7 @@ module Gitlab each do |entry| raise ExtensionError unless entry.valid? - @hash[entry.key] = entry.extend! + entry.extend! end end end diff --git a/lib/gitlab/ci/config/extendable/entry.rb b/lib/gitlab/ci/config/extendable/entry.rb index a39fa127e80..5844cb098b9 100644 --- a/lib/gitlab/ci/config/extendable/entry.rb +++ b/lib/gitlab/ci/config/extendable/entry.rb @@ -5,10 +5,9 @@ module Gitlab class Entry attr_reader :key - def initialize(key, value, context, parent = nil) + def initialize(key, hash, parent = nil) @key = key - @value = value - @context = context + @hash = hash @parent = parent end @@ -16,32 +15,29 @@ module Gitlab true end - # def circular_dependency? - # @extends.to_s == @key.to_s - # end + def value + @value ||= @hash.fetch(@key) + end def base Extendable::Entry - .new(extends, @context.fetch(extends), @context, self) + .new(extends, @hash, self) .extend! end def extensible? - @value.key?(:extends) + value.key?(:extends) end def extends - @value.fetch(:extends).to_sym + value.fetch(:extends).to_sym end def extend! if extensible? - original = @value.dup - parent = base.dup - - @value.clear.deep_merge!(parent).deep_merge!(original) + @hash[key] = base.deep_merge(value) else - @value.to_h + value end end end From 2c41fbb14821c8028e389c270d2f39380e5fbe04 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 16 Aug 2018 15:30:06 +0200 Subject: [PATCH 04/21] Detect circular dependenies in CI/CD `extends:` entry --- lib/gitlab/ci/config/extendable/collection.rb | 1 + lib/gitlab/ci/config/extendable/entry.rb | 18 ++++++++---- .../ci/config/extendable/collection_spec.rb | 29 ++++++++++++++++++- 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/lib/gitlab/ci/config/extendable/collection.rb b/lib/gitlab/ci/config/extendable/collection.rb index 3a71e06e3e2..e59884439a2 100644 --- a/lib/gitlab/ci/config/extendable/collection.rb +++ b/lib/gitlab/ci/config/extendable/collection.rb @@ -6,6 +6,7 @@ module Gitlab include Enumerable ExtensionError = Class.new(StandardError) + CircularDependencyError = Class.new(ExtensionError) def initialize(hash) @hash = hash diff --git a/lib/gitlab/ci/config/extendable/entry.rb b/lib/gitlab/ci/config/extendable/entry.rb index 5844cb098b9..96b0bd1a2ce 100644 --- a/lib/gitlab/ci/config/extendable/entry.rb +++ b/lib/gitlab/ci/config/extendable/entry.rb @@ -5,9 +5,9 @@ module Gitlab class Entry attr_reader :key - def initialize(key, hash, parent = nil) + def initialize(key, context, parent = nil) @key = key - @hash = hash + @context = context @parent = parent end @@ -16,12 +16,12 @@ module Gitlab end def value - @value ||= @hash.fetch(@key) + @value ||= @context.fetch(@key) end def base Extendable::Entry - .new(extends, @hash, self) + .new(extends, @context, self) .extend! end @@ -33,9 +33,17 @@ module Gitlab value.fetch(:extends).to_sym end + def path + Array(@parent&.path).compact.push(key) + end + def extend! + if path.count(key) > 1 + raise Extendable::Collection::CircularDependencyError + end + if extensible? - @hash[key] = base.deep_merge(value) + @context[key] = base.deep_merge(value) else value end diff --git a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb index 453c7357561..e3be0704f1a 100644 --- a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb @@ -117,6 +117,33 @@ describe Gitlab::Ci::Config::Extendable::Collection do end pending 'when invalid `extends` is specified' - pending 'when circular dependecy has been detected' + context 'when circular dependecy has been detected' do + let(:hash) do + { + test: { + extends: 'something', + script: 'ls', + only: { refs: %w[master] } + }, + + something: { + extends: '.first', + script: 'deploy', + only: { variables: %w[$SOMETHING] } + }, + + '.first': { + extends: 'something', + script: 'run', + only: { kubernetes: 'active' } + } + } + end + + it 'raises an error' do + expect { subject.extend! } + .to raise_error(described_class::CircularDependencyError) + end + end end end From 2f05e34b3c909aaf0d31ab6e10809bf3d0fc6626 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 16 Aug 2018 15:33:24 +0200 Subject: [PATCH 05/21] Add test for simple ciricular dependency in `extends` --- .../ci/config/extendable/collection_spec.rb | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb index e3be0704f1a..08c929ef70d 100644 --- a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb @@ -116,8 +116,7 @@ describe Gitlab::Ci::Config::Extendable::Collection do end end - pending 'when invalid `extends` is specified' - context 'when circular dependecy has been detected' do + context 'when nested circular dependecy has been detected' do let(:hash) do { test: { @@ -145,5 +144,24 @@ describe Gitlab::Ci::Config::Extendable::Collection do .to raise_error(described_class::CircularDependencyError) end end + + context 'when circular dependecy to self has been detected' do + let(:hash) do + { + test: { + extends: 'test', + script: 'ls', + only: { refs: %w[master] } + } + } + end + + it 'raises an error' do + expect { subject.extend! } + .to raise_error(described_class::CircularDependencyError) + end + end + + pending 'when invalid `extends` is specified' end end From ee9fd5c226fcf83b8a4a0bf2814c9d0efc530659 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 17 Aug 2018 10:07:38 +0200 Subject: [PATCH 06/21] Improve extendable CI/CD config hash collection --- lib/gitlab/ci/config/extendable/collection.rb | 17 ++++----- lib/gitlab/ci/config/extendable/entry.rb | 26 ++++++++++--- .../ci/config/extendable/collection_spec.rb | 37 +++++++++---------- 3 files changed, 45 insertions(+), 35 deletions(-) diff --git a/lib/gitlab/ci/config/extendable/collection.rb b/lib/gitlab/ci/config/extendable/collection.rb index e59884439a2..cdc8eb6614d 100644 --- a/lib/gitlab/ci/config/extendable/collection.rb +++ b/lib/gitlab/ci/config/extendable/collection.rb @@ -6,26 +6,23 @@ module Gitlab include Enumerable ExtensionError = Class.new(StandardError) + InvalidExtensionError = Class.new(ExtensionError) CircularDependencyError = Class.new(ExtensionError) def initialize(hash) - @hash = hash + @hash = hash.dup + + each { |entry| entry.extend! if entry.extensible? } end def each - @hash.each_pair do |key, value| - next unless value.key?(:extends) - + @hash.each_key do |key| yield Extendable::Entry.new(key, @hash) end end - def extend! - each do |entry| - raise ExtensionError unless entry.valid? - - entry.extend! - end + def to_hash + @hash.to_h end end end diff --git a/lib/gitlab/ci/config/extendable/entry.rb b/lib/gitlab/ci/config/extendable/entry.rb index 96b0bd1a2ce..d5eef647b3e 100644 --- a/lib/gitlab/ci/config/extendable/entry.rb +++ b/lib/gitlab/ci/config/extendable/entry.rb @@ -19,9 +19,9 @@ module Gitlab @value ||= @context.fetch(@key) end - def base + def base_hash Extendable::Entry - .new(extends, @context, self) + .new(extends_key, @context, self) .extend! end @@ -29,8 +29,8 @@ module Gitlab value.key?(:extends) end - def extends - value.fetch(:extends).to_sym + def extends_key + value.fetch(:extends).to_s.to_sym end def path @@ -38,16 +38,30 @@ module Gitlab end def extend! - if path.count(key) > 1 + if circular_dependency? raise Extendable::Collection::CircularDependencyError end + if invalid_extends_key? + raise Extendable::Collection::InvalidExtensionError + end + if extensible? - @context[key] = base.deep_merge(value) + @context[key] = base_hash.deep_merge(value) else value end end + + private + + def circular_dependency? + path.count(key) > 1 + end + + def invalid_extends_key? + !@context.key?(key) + end end end end diff --git a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb index 08c929ef70d..796683fea63 100644 --- a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb @@ -13,23 +13,13 @@ describe Gitlab::Ci::Config::Extendable::Collection do { something: { script: 'ls' }, test: test } end - it 'yields the test hash' do - expect { |b| subject.each(&b) }.to yield_control - end - end - - context 'when not extending using a hash' do - let(:hash) do - { something: { extends: [1], script: 'ls' } } - end - - it 'yields invalid extends as well' do + it 'yields control' do expect { |b| subject.each(&b) }.to yield_control end end end - describe '#extend!' do + describe '#to_hash' do context 'when a hash has a single simple extension' do let(:hash) do { @@ -47,7 +37,7 @@ describe Gitlab::Ci::Config::Extendable::Collection do end it 'extends a hash with a deep reverse merge' do - expect(subject.extend!).to eq( + expect(subject.to_hash).to eq( something: { script: 'deploy', only: { variables: %w[$SOMETHING] } @@ -88,7 +78,7 @@ describe Gitlab::Ci::Config::Extendable::Collection do end it 'extends a hash with a deep reverse merge' do - expect(subject.extend!).to eq( + expect(subject.to_hash).to eq( '.first': { script: 'run', only: { kubernetes: 'active' } @@ -139,8 +129,8 @@ describe Gitlab::Ci::Config::Extendable::Collection do } end - it 'raises an error' do - expect { subject.extend! } + it 'raises an error about circular dependency' do + expect { subject.to_hash } .to raise_error(described_class::CircularDependencyError) end end @@ -156,12 +146,21 @@ describe Gitlab::Ci::Config::Extendable::Collection do } end - it 'raises an error' do - expect { subject.extend! } + it 'raises an error about circular dependency' do + expect { subject.to_hash } .to raise_error(described_class::CircularDependencyError) end end - pending 'when invalid `extends` is specified' + context 'when invalid extends value is specified' do + let(:hash) do + { something: { extends: 1, script: 'ls' } } + end + + it 'raises an error about invalid extension' do + expect { subject.to_hash } + .to raise_error(described_class::InvalidExtensionError) + end + end end end From fe08bdf3961c6bc7db038b3c4e7c3e7d65d2344d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 17 Aug 2018 11:42:57 +0200 Subject: [PATCH 07/21] Add specs for extendable CI/CD hash entry class --- lib/gitlab/ci/config/extendable/entry.rb | 44 ++-- .../ci/config/extendable/collection_spec.rb | 37 +++- .../gitlab/ci/config/extendable/entry_spec.rb | 188 ++++++++++++++++++ 3 files changed, 249 insertions(+), 20 deletions(-) create mode 100644 spec/lib/gitlab/ci/config/extendable/entry_spec.rb diff --git a/lib/gitlab/ci/config/extendable/entry.rb b/lib/gitlab/ci/config/extendable/entry.rb index d5eef647b3e..5b26faaa806 100644 --- a/lib/gitlab/ci/config/extendable/entry.rb +++ b/lib/gitlab/ci/config/extendable/entry.rb @@ -9,28 +9,26 @@ module Gitlab @key = key @context = context @parent = parent + + raise StandardError, 'Invalid entry key!' unless @context.key?(@key) end - def valid? - true + def extensible? + value.is_a?(Hash) && value.key?(:extends) end def value @value ||= @context.fetch(@key) end - def base_hash - Extendable::Entry + def base_hash! + @base ||= Extendable::Entry .new(extends_key, @context, self) .extend! end - def extensible? - value.key?(:extends) - end - def extends_key - value.fetch(:extends).to_s.to_sym + value.fetch(:extends).to_s.to_sym if extensible? end def path @@ -38,19 +36,23 @@ module Gitlab end def extend! + return value unless extensible? + + if unknown_extension? + raise Extendable::Collection::InvalidExtensionError, + 'Unknown extension!' + end + + if invalid_base? + raise Extendable::Collection::InvalidExtensionError, + 'Invalid base hash!' + end + if circular_dependency? raise Extendable::Collection::CircularDependencyError end - if invalid_extends_key? - raise Extendable::Collection::InvalidExtensionError - end - - if extensible? - @context[key] = base_hash.deep_merge(value) - else - value - end + @context[key] = base_hash!.deep_merge(value) end private @@ -59,9 +61,13 @@ module Gitlab path.count(key) > 1 end - def invalid_extends_key? + def unknown_extension? !@context.key?(key) end + + def invalid_base? + !@context[extends_key].is_a?(Hash) + end end end end diff --git a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb index 796683fea63..20483800315 100644 --- a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb @@ -20,7 +20,23 @@ describe Gitlab::Ci::Config::Extendable::Collection do end describe '#to_hash' do - context 'when a hash has a single simple extension' do + context 'when hash does not contain extensions' do + let(:hash) do + { + test: { script: 'test' }, + production: { + script: 'deploy', + only: { variables: %w[$SOMETHING] } + } + } + end + + it 'does not modify the hash' do + expect(subject.to_hash).to eq hash + end + end + + context 'when hash has a single simple extension' do let(:hash) do { something: { @@ -162,5 +178,24 @@ describe Gitlab::Ci::Config::Extendable::Collection do .to raise_error(described_class::InvalidExtensionError) end end + + context 'when extensible entry has non-hash inheritace defined' do + let(:hash) do + { + test: { + extends: 'something', + script: 'ls', + only: { refs: %w[master] } + }, + + something: 'some text' + } + end + + it 'raises an error about invalid base' do + expect { subject.to_hash } + .to raise_error(described_class::InvalidExtensionError) + end + end end end diff --git a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb new file mode 100644 index 00000000000..7cc6e3f01f1 --- /dev/null +++ b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb @@ -0,0 +1,188 @@ +require 'fast_spec_helper' + +describe Gitlab::Ci::Config::Extendable::Entry do + describe '.new' do + context 'when entry key is not included in the context hash' do + it 'raises error' do + expect { described_class.new(:test, something: 'something') } + .to raise_error StandardError, 'Invalid entry key!' + end + end + end + + describe '#value' do + it 'reads a hash value from the context' do + entry = described_class.new(:test, test: 'something') + + expect(entry.value).to eq 'something' + end + end + + describe '#extensible?' do + context 'when entry has inheritance defined' do + it 'is extensible' do + entry = described_class.new(:test, test: { extends: 'something' }) + + expect(entry).to be_extensible + end + end + + context 'when entry does not have inheritance specified' do + it 'is not extensible' do + entry = described_class.new(:test, test: { script: 'something' }) + + expect(entry).not_to be_extensible + end + end + + context 'when entry value is not a hash' do + it 'is not extensible' do + entry = described_class.new(:test, test: 'something') + + expect(entry).not_to be_extensible + end + end + end + + describe '#extends_key' do + context 'when entry is extensible' do + it 'returns symbolized extends key value' do + entry = described_class.new(:test, test: { extends: 'something' }) + + expect(entry.extends_key).to eq :something + end + end + + context 'when entry is not extensible' do + it 'returns nil' do + entry = described_class.new(:test, test: 'something') + + expect(entry.extends_key).to be_nil + end + end + end + + describe '#path' do + it 'returns inheritance chain path' do + parent = described_class.new(:test, test: { extends: 'something' }) + child = described_class.new(:job, { job: { script: 'something' } }, parent) + + expect(child.path).to eq [:test, :job] + end + end + + describe '#base_hash!' do + subject { described_class.new(:test, hash) } + + context 'when base hash is not extensible' do + let(:hash) do + { + template: { script: 'rspec' }, + test: { extends: 'template' } + } + end + + it 'returns unchanged base hash' do + expect(subject.base_hash!).to eq(script: 'rspec') + end + end + + context 'when base hash is extensible too' do + let(:hash) do + { + first: { script: 'rspec' }, + second: { extends: 'first' }, + test: { extends: 'second' } + } + end + + it 'extends the base hash first' do + expect(subject.base_hash!).to eq(extends: 'first', script: 'rspec') + end + + it 'mutates original context' do + subject.base_hash! + + expect(hash.fetch(:second)).to eq(extends: 'first', script: 'rspec') + end + end + end + + describe '#extend!' do + subject { described_class.new(:test, hash) } + + context 'when extending a non-hash value' do + let(:hash) do + { + first: 'my value', + second: { extends: 'first' }, + test: { extends: 'second' } + } + end + + it 'raises an error' do + expect { subject.extend! }.to raise_error(StandardError) + end + end + + context 'when extending unknown key' do + let(:hash) do + { test: { extends: 'something' } } + end + + it 'raises an error' do + expect { subject.extend! }.to raise_error(StandardError) + end + end + + context 'when extending a hash correctly' do + let(:hash) do + { + first: { script: 'my value' }, + second: { extends: 'first' }, + test: { extends: 'second' } + } + end + + let(:result) do + { + first: { script: 'my value' }, + second: { extends: 'first', script: 'my value' }, + test: { extends: 'second', script: 'my value' } + } + end + + it 'returns extended part of the hash' do + expect(subject.extend!).to eq result[:test] + end + + it 'mutates original context' do + subject.extend! + + expect(hash).to eq result + end + end + + context 'when hash is not extensible' do + let(:hash) do + { + first: { script: 'my value' }, + second: { extends: 'first' }, + test: { value: 'something' } + } + end + + it 'returns original key value' do + expect(subject.extend!).to eq(value: 'something') + end + + it 'does not mutate orignal context' do + original = hash.dup + + subject.extend! + + expect(hash).to eq original + end + end + end +end From a24d4b3c674452ecb63f5a250b619362cc24630c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 17 Aug 2018 12:43:12 +0200 Subject: [PATCH 08/21] Use deep_dup to duplicate hash in CI/CD extendable config --- lib/gitlab/ci/config/extendable/collection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/ci/config/extendable/collection.rb b/lib/gitlab/ci/config/extendable/collection.rb index cdc8eb6614d..b57b3aa5d47 100644 --- a/lib/gitlab/ci/config/extendable/collection.rb +++ b/lib/gitlab/ci/config/extendable/collection.rb @@ -10,7 +10,7 @@ module Gitlab CircularDependencyError = Class.new(ExtensionError) def initialize(hash) - @hash = hash.dup + @hash = hash.deep_dup each { |entry| entry.extend! if entry.extensible? } end From d5eff68576361f43e5979bb713ecb823323c24bb Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 17 Aug 2018 12:53:04 +0200 Subject: [PATCH 09/21] Improve extended CI/CD config specs and fix a bug --- lib/gitlab/ci/config/extendable/collection.rb | 2 +- lib/gitlab/ci/config/extendable/entry.rb | 9 +++++---- .../gitlab/ci/config/extendable/collection_spec.rb | 2 +- spec/lib/gitlab/ci/config/extendable/entry_spec.rb | 11 ++++++----- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/lib/gitlab/ci/config/extendable/collection.rb b/lib/gitlab/ci/config/extendable/collection.rb index b57b3aa5d47..123219c90f2 100644 --- a/lib/gitlab/ci/config/extendable/collection.rb +++ b/lib/gitlab/ci/config/extendable/collection.rb @@ -10,7 +10,7 @@ module Gitlab CircularDependencyError = Class.new(ExtensionError) def initialize(hash) - @hash = hash.deep_dup + @hash = hash.to_h.deep_dup each { |entry| entry.extend! if entry.extensible? } end diff --git a/lib/gitlab/ci/config/extendable/entry.rb b/lib/gitlab/ci/config/extendable/entry.rb index 5b26faaa806..f14d2c1817c 100644 --- a/lib/gitlab/ci/config/extendable/entry.rb +++ b/lib/gitlab/ci/config/extendable/entry.rb @@ -40,16 +40,17 @@ module Gitlab if unknown_extension? raise Extendable::Collection::InvalidExtensionError, - 'Unknown extension!' + "Unknown extends key in extended `#{key}`!" end if invalid_base? raise Extendable::Collection::InvalidExtensionError, - 'Invalid base hash!' + "Invalid base hash in extended `#{key}`!" end if circular_dependency? - raise Extendable::Collection::CircularDependencyError + raise Extendable::Collection::CircularDependencyError, + "Circular dependency detected in extended `#{key}`!" end @context[key] = base_hash!.deep_merge(value) @@ -62,7 +63,7 @@ module Gitlab end def unknown_extension? - !@context.key?(key) + !@context.key?(extends_key) end def invalid_base? diff --git a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb index 20483800315..a47a49fc03a 100644 --- a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable/collection_spec.rb @@ -179,7 +179,7 @@ describe Gitlab::Ci::Config::Extendable::Collection do end end - context 'when extensible entry has non-hash inheritace defined' do + context 'when extensible entry has non-hash inheritance defined' do let(:hash) do { test: { diff --git a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb index 7cc6e3f01f1..39b3aec3165 100644 --- a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb @@ -115,13 +115,13 @@ describe Gitlab::Ci::Config::Extendable::Entry do let(:hash) do { first: 'my value', - second: { extends: 'first' }, - test: { extends: 'second' } + test: { extends: 'first' } } end it 'raises an error' do - expect { subject.extend! }.to raise_error(StandardError) + expect { subject.extend! } + .to raise_error(StandardError, /Invalid base hash/) end end @@ -131,7 +131,8 @@ describe Gitlab::Ci::Config::Extendable::Entry do end it 'raises an error' do - expect { subject.extend! }.to raise_error(StandardError) + expect { subject.extend! } + .to raise_error(StandardError, /Unknown extends key/) end end @@ -177,7 +178,7 @@ describe Gitlab::Ci::Config::Extendable::Entry do end it 'does not mutate orignal context' do - original = hash.dup + original = hash.deep_dup subject.extend! From 880865599f92285150e94f891cad326bf0239435 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 17 Aug 2018 12:58:47 +0200 Subject: [PATCH 10/21] Add test case describing circular dependency in `extends` --- spec/lib/gitlab/ci/config/extendable/entry_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb index 39b3aec3165..2726d91f6d6 100644 --- a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb @@ -185,5 +185,16 @@ describe Gitlab::Ci::Config::Extendable::Entry do expect(hash).to eq original end end + + context 'when circular depenency gets detected' do + let(:hash) do + { test: { extends: 'test' } } + end + + it 'raises an error' do + expect { subject.extend! } + .to raise_error(StandardError, /Circular dependency detected/) + end + end end end From 9a4117ab5bc278e8ee3fac376340808b56c09767 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 17 Aug 2018 15:04:58 +0200 Subject: [PATCH 11/21] Add support for `extends` key in CI/CD configuration --- lib/gitlab/ci/config.rb | 14 ++++++++++-- lib/gitlab/ci/config/entry/job.rb | 11 ++++++---- lib/gitlab/ci/yaml_processor.rb | 2 +- spec/lib/gitlab/ci/config_spec.rb | 36 +++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb index 66ac4a40616..2a53d43e761 100644 --- a/lib/gitlab/ci/config.rb +++ b/lib/gitlab/ci/config.rb @@ -4,12 +4,17 @@ module Gitlab # Base GitLab CI Configuration facade # class Config - # EE would override this and utilize opts argument + ConfigError = Class.new(StandardError) + def initialize(config, opts = {}) - @config = Loader.new(config).load! + @config = Extendable::Collection + .new(build_config(config, opts)) + .to_hash @global = Entry::Global.new(@config) @global.compose! + rescue Loader::FormatError, Extendable::Collection::ExtensionError => e + raise Config::ConfigError, e.message end def valid? @@ -58,6 +63,11 @@ module Gitlab def jobs @global.jobs_value end + + # 'opts' argument is used in EE see /ee/lib/ee/gitlab/ci/config.rb + def build_config(config, opts = {}) + Loader.new(config).load! + end end end end diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb index 91aac6df4b1..016a896bde5 100644 --- a/lib/gitlab/ci/config/entry/job.rb +++ b/lib/gitlab/ci/config/entry/job.rb @@ -9,9 +9,10 @@ module Gitlab include Configurable include Attributable - ALLOWED_KEYS = %i[tags script only except type image services allow_failure - type stage when artifacts cache dependencies before_script - after_script variables environment coverage retry].freeze + ALLOWED_KEYS = %i[tags script only except type image services + allow_failure type stage when artifacts cache + dependencies before_script after_script variables + environment coverage retry extends].freeze validations do validates :config, allowed_keys: ALLOWED_KEYS @@ -32,6 +33,7 @@ module Gitlab 'always or manual' } validates :dependencies, array_of_strings: true + validates :extends, type: String end end @@ -81,7 +83,8 @@ module Gitlab :cache, :image, :services, :only, :except, :variables, :artifacts, :commands, :environment, :coverage, :retry - attributes :script, :tags, :allow_failure, :when, :dependencies, :retry + attributes :script, :tags, :allow_failure, :when, :dependencies, + :retry, :extends def compose!(deps = nil) super do diff --git a/lib/gitlab/ci/yaml_processor.rb b/lib/gitlab/ci/yaml_processor.rb index e829f2a95f8..5d1864ae9e2 100644 --- a/lib/gitlab/ci/yaml_processor.rb +++ b/lib/gitlab/ci/yaml_processor.rb @@ -16,7 +16,7 @@ module Gitlab end initial_parsing - rescue Gitlab::Ci::Config::Loader::FormatError => e + rescue Gitlab::Ci::Config::ConfigError => e raise ValidationError, e.message end diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb index 2e204da307d..2b0ce9a95e0 100644 --- a/spec/lib/gitlab/ci/config_spec.rb +++ b/spec/lib/gitlab/ci/config_spec.rb @@ -1,4 +1,6 @@ -require 'spec_helper' +require 'fast_spec_helper' + +require_dependency 'active_model' describe Gitlab::Ci::Config do let(:config) do @@ -42,6 +44,36 @@ describe Gitlab::Ci::Config do end end + context 'when using extendable hash' do + let(:yml) do + <<-EOS + image: ruby:2.2 + + rspec: + script: rspec + + test: + extends: rspec + image: ruby:alpine + EOS + end + + it 'correctly extends the hash' do + hash = { + image: 'ruby:2.2', + rspec: { script: 'rspec' }, + test: { + extends: 'rspec', + image: 'ruby:alpine', + script: 'rspec' + } + } + + expect(config).to be_valid + expect(config.to_hash).to eq hash + end + end + context 'when config is invalid' do context 'when yml is incorrect' do let(:yml) { '// invalid' } @@ -49,7 +81,7 @@ describe Gitlab::Ci::Config do describe '.new' do it 'raises error' do expect { config }.to raise_error( - ::Gitlab::Ci::Config::Loader::FormatError, + described_class::ConfigError, /Invalid configuration format/ ) end From d2f46c3025cd2735ad020c400725c6ad5e98723c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 17 Aug 2018 15:27:15 +0200 Subject: [PATCH 12/21] Add feature tests for CI/CD `extends` keyword --- spec/lib/gitlab/ci/config_spec.rb | 16 ++++++ spec/lib/gitlab/ci/yaml_processor_spec.rb | 60 +++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb index 2b0ce9a95e0..ac3edbd22e3 100644 --- a/spec/lib/gitlab/ci/config_spec.rb +++ b/spec/lib/gitlab/ci/config_spec.rb @@ -107,5 +107,21 @@ describe Gitlab::Ci::Config do end end end + + context 'when invalid extended hash has been provided' do + let(:yml) do + <<-EOS + test: + extends: test + script: rspec + EOS + end + + it 'raises an error' do + expect { config }.to raise_error( + described_class::ConfigError, /Circular dependency detected/ + ) + 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 e73cdc54a15..d406a3d7409 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -562,6 +562,58 @@ module Gitlab end end + context 'when using `extends`' do + let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config) } + + subject { config_processor.builds.first } + + context 'when using simple `extends`' do + let(:config) do + <<~YAML + .template: + script: test + + rspec: + extends: .template + image: ruby:alpine + YAML + end + + it 'correctly extends rspec job' do + expect(config_processor.builds).to be_one + expect(subject.dig(:commands)).to eq 'test' + expect(subject.dig(:options, :image, :name)).to eq 'ruby:alpine' + end + end + + context 'when using recursive `extends`' do + let(:config) do + <<~YAML + rspec: + extends: .test + script: rspec + when: always + + .template: + before_script: + - bundle install + + .test: + extends: .template + script: test + image: image:test + YAML + end + + it 'correctly extends rspec job' do + expect(config_processor.builds).to be_one + expect(subject.dig(:commands)).to eq "bundle install\nrspec" + expect(subject.dig(:options, :image, :name)).to eq 'image:test' + expect(subject.dig(:when)).to eq 'always' + end + end + end + describe "When" do %w(on_success on_failure always).each do |when_state| it "returns #{when_state} when defined" do @@ -1309,6 +1361,14 @@ module Gitlab .to raise_error(Gitlab::Ci::YamlProcessor::ValidationError, 'jobs:rspec:only variables invalid expression syntax') end + + it 'returns errors if extended hash configuration is invalid' do + config = YAML.dump({ rspec: { extends: 'something', script: 'test' } }) + + expect { Gitlab::Ci::YamlProcessor.new(config) } + .to raise_error(Gitlab::Ci::YamlProcessor::ValidationError, + 'Unknown extends key in extended `rspec`!') + end end describe "Validate configuration templates" do From afbe5490f0d092382774eef2a9695d0ac655826b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 5 Sep 2018 09:57:36 +0200 Subject: [PATCH 13/21] Limit extendable CI/CD config entry nesting levels --- lib/gitlab/ci/config/extendable/collection.rb | 1 + lib/gitlab/ci/config/extendable/entry.rb | 21 +++++++++--- .../gitlab/ci/config/extendable/entry_spec.rb | 34 ++++++++++++++++--- 3 files changed, 47 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/ci/config/extendable/collection.rb b/lib/gitlab/ci/config/extendable/collection.rb index 123219c90f2..13d81987b7a 100644 --- a/lib/gitlab/ci/config/extendable/collection.rb +++ b/lib/gitlab/ci/config/extendable/collection.rb @@ -8,6 +8,7 @@ module Gitlab ExtensionError = Class.new(StandardError) InvalidExtensionError = Class.new(ExtensionError) CircularDependencyError = Class.new(ExtensionError) + NestingTooDeepError = Class.new(ExtensionError) def initialize(hash) @hash = hash.to_h.deep_dup diff --git a/lib/gitlab/ci/config/extendable/entry.rb b/lib/gitlab/ci/config/extendable/entry.rb index f14d2c1817c..d1fd8005b71 100644 --- a/lib/gitlab/ci/config/extendable/entry.rb +++ b/lib/gitlab/ci/config/extendable/entry.rb @@ -3,6 +3,8 @@ module Gitlab class Config module Extendable class Entry + MAX_NESTING_LEVELS = 10 + attr_reader :key def initialize(key, context, parent = nil) @@ -10,7 +12,9 @@ module Gitlab @context = context @parent = parent - raise StandardError, 'Invalid entry key!' unless @context.key?(@key) + unless @context.key?(@key) + raise StandardError, 'Invalid entry key!' + end end def extensible? @@ -31,8 +35,8 @@ module Gitlab value.fetch(:extends).to_s.to_sym if extensible? end - def path - Array(@parent&.path).compact.push(key) + def ancestors + @ancestors ||= Array(@parent&.ancestors) + Array(@parent&.key) end def extend! @@ -48,6 +52,11 @@ module Gitlab "Invalid base hash in extended `#{key}`!" end + if nesting_too_deep? + raise Extendable::Collection::NestingTooDeepError, + "`extends` nesting too deep in `#{key}`!" + end + if circular_dependency? raise Extendable::Collection::CircularDependencyError, "Circular dependency detected in extended `#{key}`!" @@ -58,8 +67,12 @@ module Gitlab private + def nesting_too_deep? + ancestors.count > MAX_NESTING_LEVELS + end + def circular_dependency? - path.count(key) > 1 + ancestors.include?(key) end def unknown_extension? diff --git a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb index 2726d91f6d6..8afdff99c7f 100644 --- a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb @@ -62,12 +62,17 @@ describe Gitlab::Ci::Config::Extendable::Entry do end end - describe '#path' do - it 'returns inheritance chain path' do - parent = described_class.new(:test, test: { extends: 'something' }) - child = described_class.new(:job, { job: { script: 'something' } }, parent) + describe '#ancestors' do + let(:parent) do + described_class.new(:test, test: { extends: 'something' }) + end - expect(child.path).to eq [:test, :job] + let(:child) do + described_class.new(:job, { job: { script: 'something' } }, parent) + end + + it 'returns ancestors keys' do + expect(child.ancestors).to eq [:test] end end @@ -196,5 +201,24 @@ describe Gitlab::Ci::Config::Extendable::Entry do .to raise_error(StandardError, /Circular dependency detected/) end end + + context 'when nesting level is too deep' do + before do + stub_const("#{described_class}::MAX_NESTING_LEVELS", 0) + end + + let(:hash) do + { + first: { script: 'my value' }, + second: { extends: 'first' }, + test: { extends: 'second' } + } + end + + it 'raises an error' do + expect { subject.extend! } + .to raise_error(Gitlab::Ci::Config::Extendable::Collection::NestingTooDeepError) + end + end end end From 609cb3e0f755369f862babec146f25aeb2a4f8f3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 5 Sep 2018 10:08:12 +0200 Subject: [PATCH 14/21] Simplify classes and exceptions of extendable config --- lib/gitlab/ci/config.rb | 4 +-- lib/gitlab/ci/config/extendable.rb | 27 ++++++++++++++++ lib/gitlab/ci/config/extendable/collection.rb | 32 ------------------- lib/gitlab/ci/config/extendable/entry.rb | 14 +++++--- .../gitlab/ci/config/extendable/entry_spec.rb | 5 +-- .../collection_spec.rb => extendable_spec.rb} | 10 +++--- 6 files changed, 46 insertions(+), 46 deletions(-) create mode 100644 lib/gitlab/ci/config/extendable.rb delete mode 100644 lib/gitlab/ci/config/extendable/collection.rb rename spec/lib/gitlab/ci/config/{extendable/collection_spec.rb => extendable_spec.rb} (92%) diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb index 2a53d43e761..46dad59eb8c 100644 --- a/lib/gitlab/ci/config.rb +++ b/lib/gitlab/ci/config.rb @@ -7,13 +7,13 @@ module Gitlab ConfigError = Class.new(StandardError) def initialize(config, opts = {}) - @config = Extendable::Collection + @config = Config::Extendable .new(build_config(config, opts)) .to_hash @global = Entry::Global.new(@config) @global.compose! - rescue Loader::FormatError, Extendable::Collection::ExtensionError => e + rescue Loader::FormatError, Extendable::ExtensionError => e raise Config::ConfigError, e.message end diff --git a/lib/gitlab/ci/config/extendable.rb b/lib/gitlab/ci/config/extendable.rb new file mode 100644 index 00000000000..551e0c37b61 --- /dev/null +++ b/lib/gitlab/ci/config/extendable.rb @@ -0,0 +1,27 @@ +module Gitlab + module Ci + class Config + class Extendable + include Enumerable + + ExtensionError = Class.new(StandardError) + + def initialize(hash) + @hash = hash.to_h.deep_dup + + each { |entry| entry.extend! if entry.extensible? } + end + + def each + @hash.each_key do |key| + yield Extendable::Entry.new(key, @hash) + end + end + + def to_hash + @hash.to_h + end + end + end + end +end diff --git a/lib/gitlab/ci/config/extendable/collection.rb b/lib/gitlab/ci/config/extendable/collection.rb deleted file mode 100644 index 13d81987b7a..00000000000 --- a/lib/gitlab/ci/config/extendable/collection.rb +++ /dev/null @@ -1,32 +0,0 @@ -module Gitlab - module Ci - class Config - module Extendable - class Collection - include Enumerable - - ExtensionError = Class.new(StandardError) - InvalidExtensionError = Class.new(ExtensionError) - CircularDependencyError = Class.new(ExtensionError) - NestingTooDeepError = Class.new(ExtensionError) - - def initialize(hash) - @hash = hash.to_h.deep_dup - - each { |entry| entry.extend! if entry.extensible? } - end - - def each - @hash.each_key do |key| - yield Extendable::Entry.new(key, @hash) - end - end - - def to_hash - @hash.to_h - end - end - end - end - end -end diff --git a/lib/gitlab/ci/config/extendable/entry.rb b/lib/gitlab/ci/config/extendable/entry.rb index d1fd8005b71..9bcb0e2c784 100644 --- a/lib/gitlab/ci/config/extendable/entry.rb +++ b/lib/gitlab/ci/config/extendable/entry.rb @@ -1,10 +1,14 @@ module Gitlab module Ci class Config - module Extendable + class Extendable class Entry MAX_NESTING_LEVELS = 10 + InvalidExtensionError = Class.new(Extendable::ExtensionError) + CircularDependencyError = Class.new(Extendable::ExtensionError) + NestingTooDeepError = Class.new(Extendable::ExtensionError) + attr_reader :key def initialize(key, context, parent = nil) @@ -43,22 +47,22 @@ module Gitlab return value unless extensible? if unknown_extension? - raise Extendable::Collection::InvalidExtensionError, + raise Entry::InvalidExtensionError, "Unknown extends key in extended `#{key}`!" end if invalid_base? - raise Extendable::Collection::InvalidExtensionError, + raise Entry::InvalidExtensionError, "Invalid base hash in extended `#{key}`!" end if nesting_too_deep? - raise Extendable::Collection::NestingTooDeepError, + raise Entry::NestingTooDeepError, "`extends` nesting too deep in `#{key}`!" end if circular_dependency? - raise Extendable::Collection::CircularDependencyError, + raise Entry::CircularDependencyError, "Circular dependency detected in extended `#{key}`!" end diff --git a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb index 8afdff99c7f..86a3d13ee97 100644 --- a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb @@ -198,7 +198,8 @@ describe Gitlab::Ci::Config::Extendable::Entry do it 'raises an error' do expect { subject.extend! } - .to raise_error(StandardError, /Circular dependency detected/) + .to raise_error(described_class::CircularDependencyError, + /Circular dependency detected/) end end @@ -217,7 +218,7 @@ describe Gitlab::Ci::Config::Extendable::Entry do it 'raises an error' do expect { subject.extend! } - .to raise_error(Gitlab::Ci::Config::Extendable::Collection::NestingTooDeepError) + .to raise_error(described_class::NestingTooDeepError) end end end diff --git a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb b/spec/lib/gitlab/ci/config/extendable_spec.rb similarity index 92% rename from spec/lib/gitlab/ci/config/extendable/collection_spec.rb rename to spec/lib/gitlab/ci/config/extendable_spec.rb index a47a49fc03a..b985b95ada6 100644 --- a/spec/lib/gitlab/ci/config/extendable/collection_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable_spec.rb @@ -1,6 +1,6 @@ require 'fast_spec_helper' -describe Gitlab::Ci::Config::Extendable::Collection do +describe Gitlab::Ci::Config::Extendable do subject { described_class.new(hash) } describe '#each' do @@ -147,7 +147,7 @@ describe Gitlab::Ci::Config::Extendable::Collection do it 'raises an error about circular dependency' do expect { subject.to_hash } - .to raise_error(described_class::CircularDependencyError) + .to raise_error(described_class::Entry::CircularDependencyError) end end @@ -164,7 +164,7 @@ describe Gitlab::Ci::Config::Extendable::Collection do it 'raises an error about circular dependency' do expect { subject.to_hash } - .to raise_error(described_class::CircularDependencyError) + .to raise_error(described_class::Entry::CircularDependencyError) end end @@ -175,7 +175,7 @@ describe Gitlab::Ci::Config::Extendable::Collection do it 'raises an error about invalid extension' do expect { subject.to_hash } - .to raise_error(described_class::InvalidExtensionError) + .to raise_error(described_class::Entry::InvalidExtensionError) end end @@ -194,7 +194,7 @@ describe Gitlab::Ci::Config::Extendable::Collection do it 'raises an error about invalid base' do expect { subject.to_hash } - .to raise_error(described_class::InvalidExtensionError) + .to raise_error(described_class::Entry::InvalidExtensionError) end end end From d7133d946c0ecd13a2f5be723ccb541f37b8c1f2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 5 Sep 2018 10:11:25 +0200 Subject: [PATCH 15/21] Add changelog for extendable CI/CD configuration --- .../feature-gb-allow-to-extend-keys-in-gitlab-ci-yml.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/feature-gb-allow-to-extend-keys-in-gitlab-ci-yml.yml diff --git a/changelogs/unreleased/feature-gb-allow-to-extend-keys-in-gitlab-ci-yml.yml b/changelogs/unreleased/feature-gb-allow-to-extend-keys-in-gitlab-ci-yml.yml new file mode 100644 index 00000000000..b46dfd47e7a --- /dev/null +++ b/changelogs/unreleased/feature-gb-allow-to-extend-keys-in-gitlab-ci-yml.yml @@ -0,0 +1,5 @@ +--- +title: Add support for extendable CI/CD config with +merge_request: 21243 +author: +type: added From b83fa58f063b0b086b2c4def68835dbec4fd6bcd Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 5 Sep 2018 10:17:13 +0200 Subject: [PATCH 16/21] Improve extended CI/CD config error messages --- lib/gitlab/ci/config/extendable/entry.rb | 12 ++++++------ spec/lib/gitlab/ci/config/extendable/entry_spec.rb | 8 +++++--- spec/lib/gitlab/ci/config_spec.rb | 2 +- spec/lib/gitlab/ci/yaml_processor_spec.rb | 2 +- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/lib/gitlab/ci/config/extendable/entry.rb b/lib/gitlab/ci/config/extendable/entry.rb index 9bcb0e2c784..f9b258590d9 100644 --- a/lib/gitlab/ci/config/extendable/entry.rb +++ b/lib/gitlab/ci/config/extendable/entry.rb @@ -3,12 +3,12 @@ module Gitlab class Config class Extendable class Entry - MAX_NESTING_LEVELS = 10 - InvalidExtensionError = Class.new(Extendable::ExtensionError) CircularDependencyError = Class.new(Extendable::ExtensionError) NestingTooDeepError = Class.new(Extendable::ExtensionError) + MAX_NESTING_LEVELS = 10 + attr_reader :key def initialize(key, context, parent = nil) @@ -48,22 +48,22 @@ module Gitlab if unknown_extension? raise Entry::InvalidExtensionError, - "Unknown extends key in extended `#{key}`!" + "#{key}: unknown `extends` key" end if invalid_base? raise Entry::InvalidExtensionError, - "Invalid base hash in extended `#{key}`!" + "#{key}: invalid base hash in `extends`" end if nesting_too_deep? raise Entry::NestingTooDeepError, - "`extends` nesting too deep in `#{key}`!" + "#{key}: `extends` nesting too deep" end if circular_dependency? raise Entry::CircularDependencyError, - "Circular dependency detected in extended `#{key}`!" + "#{key}: circular dependency detected in `extends`" end @context[key] = base_hash!.deep_merge(value) diff --git a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb index 86a3d13ee97..e05bd22ebd0 100644 --- a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb @@ -126,7 +126,8 @@ describe Gitlab::Ci::Config::Extendable::Entry do it 'raises an error' do expect { subject.extend! } - .to raise_error(StandardError, /Invalid base hash/) + .to raise_error(described_class::InvalidExtensionError, + /invalid base hash/) end end @@ -137,7 +138,8 @@ describe Gitlab::Ci::Config::Extendable::Entry do it 'raises an error' do expect { subject.extend! } - .to raise_error(StandardError, /Unknown extends key/) + .to raise_error(described_class::InvalidExtensionError, + /unknown `extends` key/) end end @@ -199,7 +201,7 @@ describe Gitlab::Ci::Config::Extendable::Entry do it 'raises an error' do expect { subject.extend! } .to raise_error(described_class::CircularDependencyError, - /Circular dependency detected/) + /circular dependency detected/) end end diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb index ac3edbd22e3..5a78ce783dd 100644 --- a/spec/lib/gitlab/ci/config_spec.rb +++ b/spec/lib/gitlab/ci/config_spec.rb @@ -119,7 +119,7 @@ describe Gitlab::Ci::Config do it 'raises an error' do expect { config }.to raise_error( - described_class::ConfigError, /Circular dependency detected/ + described_class::ConfigError, /circular dependency detected/ ) end end diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index d406a3d7409..a75c8246758 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -1367,7 +1367,7 @@ module Gitlab expect { Gitlab::Ci::YamlProcessor.new(config) } .to raise_error(Gitlab::Ci::YamlProcessor::ValidationError, - 'Unknown extends key in extended `rspec`!') + 'rspec: unknown `extends` key') end end From b19d2e1e6d75a3568ad7b9ea00a9bee216ed55ec Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 5 Sep 2018 10:22:09 +0200 Subject: [PATCH 17/21] Add missing frozen_string_literal to some classes --- lib/gitlab/ci/config/extendable.rb | 2 ++ lib/gitlab/ci/config/extendable/entry.rb | 2 ++ 2 files changed, 4 insertions(+) diff --git a/lib/gitlab/ci/config/extendable.rb b/lib/gitlab/ci/config/extendable.rb index 551e0c37b61..a43901c69fe 100644 --- a/lib/gitlab/ci/config/extendable.rb +++ b/lib/gitlab/ci/config/extendable.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Gitlab module Ci class Config diff --git a/lib/gitlab/ci/config/extendable/entry.rb b/lib/gitlab/ci/config/extendable/entry.rb index f9b258590d9..c6bfbad543e 100644 --- a/lib/gitlab/ci/config/extendable/entry.rb +++ b/lib/gitlab/ci/config/extendable/entry.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Gitlab module Ci class Config From 7efef550da5e0901b7a4b6f94d49395fabbf2d38 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 5 Sep 2018 10:27:21 +0200 Subject: [PATCH 18/21] Add unit test for `extends` in .gitlab-ci.yml --- spec/lib/gitlab/ci/config/entry/job_spec.rb | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb index 6769f64f950..2c9758401b7 100644 --- a/spec/lib/gitlab/ci/config/entry/job_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb @@ -1,4 +1,5 @@ -require 'spec_helper' +require 'fast_spec_helper' +require_dependency 'active_model' describe Gitlab::Ci::Config::Entry::Job do let(:entry) { described_class.new(config, name: :rspec) } @@ -81,6 +82,15 @@ describe Gitlab::Ci::Config::Entry::Job do 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 a string" + end + end + context 'when retry value is not correct' do context 'when it is not a numeric value' do let(:config) { { retry: true } } @@ -124,6 +134,8 @@ describe Gitlab::Ci::Config::Entry::Job do describe '#relevant?' do it 'is a relevant entry' do + entry = described_class.new({ script: 'rspec' }, name: :rspec) + expect(entry).to be_relevant end end From eedebb2bac097ddc4e6f1f5d958e9d54bb1540a2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 5 Sep 2018 10:52:53 +0200 Subject: [PATCH 19/21] Improve specs and error messages in extendable config --- lib/gitlab/ci/config/extendable/entry.rb | 4 +-- .../gitlab/ci/config/extendable/entry_spec.rb | 2 +- spec/lib/gitlab/ci/config/extendable_spec.rb | 31 +++++++++++++++++-- spec/lib/gitlab/ci/yaml_processor_spec.rb | 2 +- 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/lib/gitlab/ci/config/extendable/entry.rb b/lib/gitlab/ci/config/extendable/entry.rb index c6bfbad543e..7793db09d33 100644 --- a/lib/gitlab/ci/config/extendable/entry.rb +++ b/lib/gitlab/ci/config/extendable/entry.rb @@ -50,7 +50,7 @@ module Gitlab if unknown_extension? raise Entry::InvalidExtensionError, - "#{key}: unknown `extends` key" + "#{key}: unknown key in `extends`" end if invalid_base? @@ -60,7 +60,7 @@ module Gitlab if nesting_too_deep? raise Entry::NestingTooDeepError, - "#{key}: `extends` nesting too deep" + "#{key}: nesting too deep in `extends`" end if circular_dependency? diff --git a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb index e05bd22ebd0..0a148375d11 100644 --- a/spec/lib/gitlab/ci/config/extendable/entry_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable/entry_spec.rb @@ -139,7 +139,7 @@ describe Gitlab::Ci::Config::Extendable::Entry do it 'raises an error' do expect { subject.extend! } .to raise_error(described_class::InvalidExtensionError, - /unknown `extends` key/) + /unknown key/) end end diff --git a/spec/lib/gitlab/ci/config/extendable_spec.rb b/spec/lib/gitlab/ci/config/extendable_spec.rb index b985b95ada6..90213f6603d 100644 --- a/spec/lib/gitlab/ci/config/extendable_spec.rb +++ b/spec/lib/gitlab/ci/config/extendable_spec.rb @@ -80,9 +80,19 @@ describe Gitlab::Ci::Config::Extendable do only: { refs: %w[master] } }, + build: { + extends: 'something', + stage: 'build' + }, + + deploy: { + stage: 'deploy', + extends: '.first' + }, + something: { extends: '.first', - script: 'deploy', + script: 'exec', only: { variables: %w[$SOMETHING] } }, @@ -102,7 +112,24 @@ describe Gitlab::Ci::Config::Extendable do something: { extends: '.first', - script: 'deploy', + script: 'exec', + only: { + kubernetes: 'active', + variables: %w[$SOMETHING] + } + }, + + deploy: { + script: 'run', + stage: 'deploy', + only: { kubernetes: 'active' }, + extends: '.first' + }, + + build: { + extends: 'something', + script: 'exec', + stage: 'build', only: { kubernetes: 'active', variables: %w[$SOMETHING] diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index a75c8246758..fcbdf71a4e9 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -1367,7 +1367,7 @@ module Gitlab expect { Gitlab::Ci::YamlProcessor.new(config) } .to raise_error(Gitlab::Ci::YamlProcessor::ValidationError, - 'rspec: unknown `extends` key') + 'rspec: unknown key in `extends`') end end From 2e73c248c9f75e63068dfa84adaced4eefbb7297 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 5 Sep 2018 11:26:49 +0200 Subject: [PATCH 20/21] Add docs for extended CI/CD config with `extends` --- doc/ci/yaml/README.md | 78 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index e93060fec85..fa8a083c7a8 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -56,6 +56,7 @@ A job is defined by a list of parameters that define the job behavior. | Keyword | Required | Description | |---------------|----------|-------------| | script | yes | Defines a shell script which is executed by Runner | +| extends | no | Defines a configuration entry that this job is going to inherit from | | image | no | Use docker image, covered in [Using Docker Images](../docker/using_docker_images.md#define-image-and-services-from-gitlab-ciyml) | | services | no | Use docker services, covered in [Using Docker Images](../docker/using_docker_images.md#define-image-and-services-from-gitlab-ciyml) | | stage | no | Defines a job stage (default: `test`) | @@ -75,6 +76,83 @@ A job is defined by a list of parameters that define the job behavior. | coverage | no | Define code coverage settings for a given job | | retry | no | Define how many times a job can be auto-retried in case of a failure | +### `extends` + +> Introduced in GitLab 11.3 + +`extends` defines an entry name that a job, that uses `extends` is going to +inherit from. + +`extends` in an alternative to using [YAML anchors](#anchors) that is a little +more flexible and readable. + +```yaml +.tests: + only: + refs: + - master + - branches + +rspec: + extends: .tests + script: rake rspec + stage: test + only: + variables: + - $RSPEC +``` + +In the example above the `rspec` job is going to inherit from `.tests` +template. GitLab will perform a reverse deep merge, what means that it will +merge `rspec` contents into `.tests` recursively, and it is going to result in +following configuration of the `rspec` job: + +```yaml +rspec: + script: rake rspec + stage: test + only: + refs: + - master + - branches + variables: + - $RSPEC +``` + +`.tests` in this example is a [hidden key](#hidden-keys-jobs), but it is +possible to inherit from regular jobs as well. + +`extends` supports multi-level inheritance, however it is not recommended to +use more than three levels of inheritance. Maximum nesting level supported is +10 levels. + + +```yaml +.tests: + only: + refs: + - master + - branches + +.rspec: + extends: .tests + script: rake rspec + +rspec 1: + variables + RSPEC_SUITE: 1 + extends: .rspec + +rspec 2: + variables + RSPEC_SUITE: 2 + extends: .rspec + +spinach: + extends: .tests + script: rake spinach +``` + ### `pages` `pages` is a special job that is used to upload static content to GitLab that From e8648241e4af7d5f9ae1345fef9636bfdbb85557 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 5 Sep 2018 14:55:27 +0200 Subject: [PATCH 21/21] Fix extended CI/CD configuration docs for `extends` --- doc/ci/yaml/README.md | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index fa8a083c7a8..c1ebe39e076 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -90,7 +90,6 @@ more flexible and readable. .tests: only: refs: - - master - branches rspec: @@ -113,7 +112,6 @@ rspec: stage: test only: refs: - - master - branches variables: - $RSPEC @@ -130,22 +128,20 @@ use more than three levels of inheritance. Maximum nesting level supported is ```yaml .tests: only: - refs: - - master - - branches + - pushes .rspec: extends: .tests script: rake rspec rspec 1: - variables - RSPEC_SUITE: 1 + variables: + RSPEC_SUITE: '1' extends: .rspec rspec 2: - variables - RSPEC_SUITE: 2 + variables: + RSPEC_SUITE: '2' extends: .rspec spinach: