From ee9fd5c226fcf83b8a4a0bf2814c9d0efc530659 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 17 Aug 2018 10:07:38 +0200
Subject: [PATCH] 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