Merge branch 'refactor/ci-config-add-logical-validation' into 'master'

lib/ci/gitlab_ci_yaml_processor.rb

@@ -55,12 +55,7 @@ module Ci
       {
         stage_idx: @stages.index(job[:stage]),
         stage: job[:stage],
-        ##
-        # Refactoring note:
-        #  - before script behaves differently than after script
-        #  - after script returns an array of commands
-        #  - before script should be a concatenated command
-        commands: [job[:before_script] || @before_script, job[:script]].flatten.compact.join("\n"),
+        commands: job[:commands],
         tag_list: job[:tags] || [],
         name: job[:name].to_s,
         allow_failure: job[:allow_failure] || false,
@@ -68,12 +63,12 @@ module Ci
         environment: job[:environment],
         yaml_variables: yaml_variables(name),
         options: {
-          image: job[:image] || @image,
-          services: job[:services] || @services,
+          image: job[:image],
+          services: job[:services],
           artifacts: job[:artifacts],
-          cache: job[:cache] || @cache,
+          cache: job[:cache],
           dependencies: job[:dependencies],
-          after_script: job[:after_script] || @after_script,
+          after_script: job[:after_script],
         }.compact
       }
     end

lib/gitlab/ci/config.rb

@@ -14,7 +14,7 @@ module Gitlab
         @config = Loader.new(config).load!
 
         @global = Node::Global.new(@config)
-        @global.process!
+        @global.compose!
       end
 
       def valid?

lib/gitlab/ci/config/node/configurable.rb

@@ -23,9 +23,9 @@ module Gitlab
             end
           end
 
-          private
+          def compose!(deps = nil)
+            return unless valid?
 
-          def compose!
             self.class.nodes.each do |key, factory|
               factory
                 .value(@config[key])
@@ -33,6 +33,12 @@ module Gitlab
 
               @entries[key] = factory.create!
             end
+
+            yield if block_given?
+
+            @entries.each_value do |entry|
+              entry.compose!(deps)
+            end
           end
 
           class_methods do

lib/gitlab/ci/config/node/entry.rb

@@ -20,11 +20,14 @@ module Gitlab
             @validator.validate(:new)
           end
 
-          def process!
+          def [](key)
+            @entries[key] || Node::Undefined.new
+          end
+
+          def compose!(deps = nil)
             return unless valid?
 
-            compose!
-            descendants.each(&:process!)
+            yield if block_given?
           end
 
           def leaf?
@@ -73,11 +76,6 @@ module Gitlab
           def self.validator
             Validator
           end
-
-          private
-
-          def compose!
-          end
         end
       end
     end

lib/gitlab/ci/config/node/factory.rb

@@ -37,8 +37,8 @@ module Gitlab
             # See issue #18775.
             #
             if @value.nil?
-              Node::Undefined.new(
-                fabricate_undefined
+              Node::Unspecified.new(
+                fabricate_unspecified
               )
             else
               fabricate(@node, @value)
@@ -47,13 +47,13 @@ module Gitlab
 
           private
 
-          def fabricate_undefined
+          def fabricate_unspecified
             ##
             # If node has a default value we fabricate concrete node
             # with default value.
             #
             if @node.default.nil?
-              fabricate(Node::Null)
+              fabricate(Node::Undefined)
             else
               fabricate(@node, @node.default)
             end

lib/gitlab/ci/config/node/global.rb

@@ -36,15 +36,15 @@ module Gitlab
           helpers :before_script, :image, :services, :after_script,
                   :variables, :stages, :types, :cache, :jobs
 
-          private
-
-          def compose!
-            super
-
-            compose_jobs!
-            compose_deprecated_entries!
+          def compose!(_deps = nil)
+            super(self) do
+              compose_jobs!
+              compose_deprecated_entries!
+            end
           end
 
+          private
+
           def compose_jobs!
             factory = Node::Factory.new(Node::Jobs)
               .value(@config.except(*self.class.nodes.keys))

lib/gitlab/ci/config/node/job.rb

@@ -80,7 +80,19 @@ module Gitlab
 
           helpers :before_script, :script, :stage, :type, :after_script,
                   :cache, :image, :services, :only, :except, :variables,
-                  :artifacts
+                  :artifacts, :commands
+
+          def compose!(deps = nil)
+            super do
+              if type_defined? && !stage_defined?
+                @entries[:stage] = @entries[:type]
+              end
+
+              @entries.delete(:type)
+            end
+
+            inherit!(deps)
+          end
 
           def name
             @metadata[:name]
@@ -90,12 +102,30 @@ module Gitlab
             @config.merge(to_hash.compact)
           end
 
+          def commands
+            (before_script_value.to_a + script_value.to_a).join("\n")
+          end
+
           private
 
+          def inherit!(deps)
+            return unless deps
+
+            self.class.nodes.each_key do |key|
+              global_entry = deps[key]
+              job_entry = @entries[key]
+
+              if global_entry.specified? && !job_entry.specified?
+                @entries[key] = global_entry
+              end
+            end
+          end
+
           def to_hash
             { name: name,
               before_script: before_script,
               script: script,
+              commands: commands,
               image: image,
               services: services,
               stage: stage,
@@ -106,16 +136,6 @@ module Gitlab
               artifacts: artifacts,
               after_script: after_script }
           end
-
-          def compose!
-            super
-
-            if type_defined? && !stage_defined?
-              @entries[:stage] = @entries[:type]
-            end
-
-            @entries.delete(:type)
-          end
         end
       end
     end

lib/gitlab/ci/config/node/jobs.rb

@@ -26,19 +26,23 @@ module Gitlab
             name.to_s.start_with?('.')
           end
 
-          private
-
-          def compose!
-            @config.each do |name, config|
-              node = hidden?(name) ? Node::Hidden : Node::Job
-
-              factory = Node::Factory.new(node)
-                .value(config || {})
-                .metadata(name: name)
-                .with(key: name, parent: self,
-                      description: "#{name} job definition.")
+          def compose!(deps = nil)
+            super do
+              @config.each do |name, config|
+                node = hidden?(name) ? Node::Hidden : Node::Job
+
+                factory = Node::Factory.new(node)
+                  .value(config || {})
+                  .metadata(name: name)
+                  .with(key: name, parent: self,
+                        description: "#{name} job definition.")
+
+                @entries[name] = factory.create!
+              end
 
-              @entries[name] = factory.create!
+              @entries.each_value do |entry|
+                entry.compose!(deps)
+              end
             end
           end
         end

lib/gitlab/ci/config/node/undefined.rb

@@ -3,15 +3,34 @@ module Gitlab
     class Config
       module Node
         ##
-        # This class represents an unspecified entry node.
+        # This class represents an undefined node.
         #
-        # It decorates original entry adding method that indicates it is
-        # unspecified.
+        # Implements the Null Object pattern.
         #
-        class Undefined < SimpleDelegator
+        class Undefined < Entry
+          def initialize(*)
+            super(nil)
+          end
+
+          def value
+            nil
+          end
+
+          def valid?
+            true
+          end
+
+          def errors
+            []
+          end
+
           def specified?
             false
           end
+
+          def relevant?
+            false
+          end
         end
       end
     end

lib/gitlab/ci/config/node/null.rb → lib/gitlab/ci/config/node/unspecified.rb

@@ -3,30 +3,15 @@ module Gitlab
     class Config
       module Node
         ##
-        # This class represents an undefined node.
+        # This class represents an unspecified entry node.
         #
-        # Implements the Null Object pattern.
+        # It decorates original entry adding method that indicates it is
+        # unspecified.
         #
-        class Null < Entry
-          def value
-            nil
-          end
-
-          def valid?
-            true
-          end
-
-          def errors
-            []
-          end
-
+        class Unspecified < SimpleDelegator
           def specified?
             false
           end
-
-          def relevant?
-            false
-          end
         end
       end
     end

spec/lib/gitlab/ci/config/node/cache_spec.rb

@@ -4,7 +4,7 @@ describe Gitlab::Ci::Config::Node::Cache do
   let(:entry) { described_class.new(config) }
 
   describe 'validations' do
-    before { entry.process! }
+    before { entry.compose! }
 
     context 'when entry config value is correct' do
       let(:config) do

spec/lib/gitlab/ci/config/node/factory_spec.rb

@@ -65,7 +65,8 @@ describe Gitlab::Ci::Config::Node::Factory do
           .value(nil)
           .create!
 
-        expect(entry).to be_an_instance_of Gitlab::Ci::Config::Node::Undefined
+        expect(entry)
+          .to be_an_instance_of Gitlab::Ci::Config::Node::Unspecified
       end
     end
 

spec/lib/gitlab/ci/config/node/global_spec.rb

@@ -14,7 +14,7 @@ describe Gitlab::Ci::Config::Node::Global do
   end
 
   context 'when hash is valid' do
-    context 'when all entries defined' do
+    context 'when some entries defined' do
       let(:hash) do
         { before_script: ['ls', 'pwd'],
           image: 'ruby:2.2',
@@ -24,11 +24,11 @@ describe Gitlab::Ci::Config::Node::Global do
           stages: ['build', 'pages'],
           cache: { key: 'k', untracked: true, paths: ['public/'] },
           rspec: { script: %w[rspec ls] },
-          spinach: { script: 'spinach' } }
+          spinach: { before_script: [], variables: {}, script: 'spinach' } }
       end
 
-      describe '#process!' do
-        before { global.process! }
+      describe '#compose!' do
+        before { global.compose! }
 
         it 'creates nodes hash' do
           expect(global.descendants).to be_an Array
@@ -59,7 +59,7 @@ describe Gitlab::Ci::Config::Node::Global do
         end
       end
 
-      context 'when not processed' do
+      context 'when not composed' do
         describe '#before_script' do
           it 'returns nil' do
             expect(global.before_script).to be nil
@@ -73,8 +73,14 @@ describe Gitlab::Ci::Config::Node::Global do
         end
       end
 
-      context 'when processed' do
-        before { global.process! }
+      context 'when composed' do
+        before { global.compose! }
+
+        describe '#errors' do
+          it 'has no errors' do
+            expect(global.errors).to be_empty
+          end
+        end
 
         describe '#before_script' do
           it 'returns correct script' do
@@ -137,10 +143,24 @@ describe Gitlab::Ci::Config::Node::Global do
             expect(global.jobs).to eq(
               rspec: { name: :rspec,
                        script: %w[rspec ls],
-                       stage: 'test' },
+                       before_script: ['ls', 'pwd'],
+                       commands: "ls\npwd\nrspec\nls",
+                       image: 'ruby:2.2',
+                       services: ['postgres:9.1', 'mysql:5.5'],
+                       stage: 'test',
+                       cache: { key: 'k', untracked: true, paths: ['public/'] },
+                       variables: { VAR: 'value' },
+                       after_script: ['make clean'] },
               spinach: { name: :spinach,
+                         before_script: [],
                          script: %w[spinach],
-                         stage: 'test' }
+                         commands: 'spinach',
+                         image: 'ruby:2.2',
+                         services: ['postgres:9.1', 'mysql:5.5'],
+                         stage: 'test',
+                         cache: { key: 'k', untracked: true, paths: ['public/'] },
+                         variables: {},
+                         after_script: ['make clean'] },
             )
           end
         end
@@ -148,17 +168,20 @@ describe Gitlab::Ci::Config::Node::Global do
     end
 
     context 'when most of entires not defined' do
-      let(:hash) { { cache: { key: 'a' }, rspec: { script: %w[ls] } } }
-      before { global.process! }
+      before { global.compose! }
+
+      let(:hash) do
+        { cache: { key: 'a' }, rspec: { script: %w[ls] } }
+      end
 
       describe '#nodes' do
         it 'instantizes all nodes' do
           expect(global.descendants.count).to eq 8
         end
 
-        it 'contains undefined nodes' do
+        it 'contains unspecified nodes' do
           expect(global.descendants.first)
-            .to be_an_instance_of Gitlab::Ci::Config::Node::Undefined
+            .to be_an_instance_of Gitlab::Ci::Config::Node::Unspecified
         end
       end
 
@@ -188,8 +211,11 @@ describe Gitlab::Ci::Config::Node::Global do
     # details.
     #
     context 'when entires specified but not defined' do
-      let(:hash) { { variables: nil, rspec: { script: 'rspec' } } }
-      before { global.process! }
+      before { global.compose! }
+
+      let(:hash) do
+        { variables: nil, rspec: { script: 'rspec' } }
+      end
 
       describe '#variables' do
         it 'undefined entry returns a default value' do
@@ -200,7 +226,7 @@ describe Gitlab::Ci::Config::Node::Global do
   end
 
   context 'when hash is not valid' do
-    before { global.process! }
+    before { global.compose! }
 
     let(:hash) do
       { before_script: 'ls' }
@@ -247,4 +273,27 @@ describe Gitlab::Ci::Config::Node::Global do
       expect(global.specified?).to be true
     end
   end
+
+  describe '#[]' do
+    before { global.compose! }
+
+    let(:hash) do
+      { cache: { key: 'a' }, rspec: { script: 'ls' } }
+    end
+
+    context 'when node exists' do
+      it 'returns correct entry' do
+        expect(global[:cache])
+          .to be_an_instance_of Gitlab::Ci::Config::Node::Cache
+        expect(global[:jobs][:rspec][:script].value).to eq ['ls']
+      end
+    end
+
+    context 'when node does not exist' do
+      it 'always return unspecified node' do
+        expect(global[:some][:unknown][:node])
+          .not_to be_specified
+      end
+    end
+  end
 end

spec/lib/gitlab/ci/config/node/job_spec.rb

@@ -3,9 +3,9 @@ require 'spec_helper'
 describe Gitlab::Ci::Config::Node::Job do
   let(:entry) { described_class.new(config, name: :rspec) }
 
-  before { entry.process! }
-
   describe 'validations' do
+    before { entry.compose! }
+
     context 'when entry config value is correct' do
       let(:config) { { script: 'rspec' } }
 
@@ -59,28 +59,82 @@ describe Gitlab::Ci::Config::Node::Job do
     end
   end
 
-  describe '#value' do
-    context 'when entry is correct' do
+  describe '#relevant?' do
+    it 'is a relevant entry' do
+      expect(entry).to be_relevant
+    end
+  end
+
+  describe '#compose!' do
+    let(:unspecified) { double('unspecified', 'specified?' => false) }
+
+    let(:specified) do
+      double('specified', 'specified?' => true, value: 'specified')
+    end
+
+    let(:deps) { double('deps', '[]' => unspecified) }
+
+    context 'when job config overrides global config' do
+      before { entry.compose!(deps) }
+
       let(:config) do
-        { before_script: %w[ls pwd],
-          script: 'rspec',
-          after_script: %w[cleanup] }
+        { image: 'some_image', cache: { key: 'test' } }
+      end
+
+      it 'overrides global config' do
+        expect(entry[:image].value).to eq 'some_image'
+        expect(entry[:cache].value).to eq(key: 'test')
+      end
+    end
+
+    context 'when job config does not override global config' do
+      before do
+        allow(deps).to receive('[]').with(:image).and_return(specified)
+        entry.compose!(deps)
       end
 
-      it 'returns correct value' do
-        expect(entry.value)
-          .to eq(name: :rspec,
-                 before_script: %w[ls pwd],
-                 script: %w[rspec],
-                 stage: 'test',
-                 after_script: %w[cleanup])
+      let(:config) { { script: 'ls', cache: { key: 'test' } } }
+
+      it 'uses config from global entry' do
+        expect(entry[:image].value).to eq 'specified'
+        expect(entry[:cache].value).to eq(key: 'test')
       end
     end
   end
 
-  describe '#relevant?' do
-    it 'is a relevant entry' do
-      expect(entry).to be_relevant
+  context 'when composed' do
+    before { entry.compose! }
+
+    describe '#value' do
+      before { entry.compose! }
+
+      context 'when entry is correct' do
+        let(:config) do
+          { before_script: %w[ls pwd],
+            script: 'rspec',
+            after_script: %w[cleanup] }
+        end
+
+        it 'returns correct value' do
+          expect(entry.value)
+            .to eq(name: :rspec,
+                   before_script: %w[ls pwd],
+                   script: %w[rspec],
+                   commands: "ls\npwd\nrspec",
+                   stage: 'test',
+                   after_script: %w[cleanup])
+        end
+      end
+    end
+
+    describe '#commands' do
+      let(:config) do
+        { before_script: %w[ls pwd], script: 'rspec' }
+      end
+
+      it 'returns a string of commands concatenated with new line character' do
+        expect(entry.commands).to eq "ls\npwd\nrspec"
+      end
     end
   end
 end

spec/lib/gitlab/ci/config/node/jobs_spec.rb

@@ -4,7 +4,7 @@ describe Gitlab::Ci::Config::Node::Jobs do
   let(:entry) { described_class.new(config) }
 
   describe 'validations' do
-    before { entry.process! }
+    before { entry.compose! }
 
     context 'when entry config value is correct' do
       let(:config) { { rspec: { script: 'rspec' } } }
@@ -47,8 +47,8 @@ describe Gitlab::Ci::Config::Node::Jobs do
     end
   end
 
-  context 'when valid job entries processed' do
-    before { entry.process! }
+  context 'when valid job entries composed' do
+    before { entry.compose! }
 
     let(:config) do
       { rspec: { script: 'rspec' },
@@ -61,9 +61,11 @@ describe Gitlab::Ci::Config::Node::Jobs do
         expect(entry.value).to eq(
           rspec: { name: :rspec,
                    script: %w[rspec],
+                   commands: 'rspec',
                    stage: 'test' },
           spinach: { name: :spinach,
                      script: %w[spinach],
+                     commands: 'spinach',
                      stage: 'test' })
       end
     end

spec/lib/gitlab/ci/config/node/null_spec.rb

-require 'spec_helper'
-
-describe Gitlab::Ci::Config::Node::Null do
-  let(:null) { described_class.new(nil) }
-
-  describe '#leaf?' do
-    it 'is leaf node' do
-      expect(null).to be_leaf
-    end
-  end
-
-  describe '#valid?' do
-    it 'is always valid' do
-      expect(null).to be_valid
-    end
-  end
-
-  describe '#errors' do
-    it 'is does not contain errors' do
-      expect(null.errors).to be_empty
-    end
-  end
-
-  describe '#value' do
-    it 'returns nil' do
-      expect(null.value).to eq nil
-    end
-  end
-
-  describe '#relevant?' do
-    it 'is not relevant' do
-      expect(null.relevant?).to eq false
-    end
-  end
-
-  describe '#specified?' do
-    it 'is not defined' do
-      expect(null.specified?).to eq false
-    end
-  end
-end

spec/lib/gitlab/ci/config/node/script_spec.rb

@@ -3,9 +3,7 @@ require 'spec_helper'
 describe Gitlab::Ci::Config::Node::Script do
   let(:entry) { described_class.new(config) }
 
-  describe '#process!' do
-    before { entry.process! }
-
+  describe 'validations' do
     context 'when entry config value is correct' do
       let(:config) { ['ls', 'pwd'] }
 

spec/lib/gitlab/ci/config/node/undefined_spec.rb

 require 'spec_helper'
 
 describe Gitlab::Ci::Config::Node::Undefined do
-  let(:undefined) { described_class.new(entry) }
-  let(:entry) { spy('Entry') }
+  let(:entry) { described_class.new }
+
+  describe '#leaf?' do
+    it 'is leaf node' do
+      expect(entry).to be_leaf
+    end
+  end
 
   describe '#valid?' do
-    it 'delegates method to entry' do
-      expect(undefined.valid).to eq entry
+    it 'is always valid' do
+      expect(entry).to be_valid
     end
   end
 
   describe '#errors' do
-    it 'delegates method to entry' do
-      expect(undefined.errors).to eq entry
+    it 'is does not contain errors' do
+      expect(entry.errors).to be_empty
     end
   end
 
   describe '#value' do
-    it 'delegates method to entry' do
-      expect(undefined.value).to eq entry
+    it 'returns nil' do
+      expect(entry.value).to eq nil
     end
   end
 
-  describe '#specified?' do
-    it 'is always false' do
-      allow(entry).to receive(:specified?).and_return(true)
+  describe '#relevant?' do
+    it 'is not relevant' do
+      expect(entry.relevant?).to eq false
+    end
+  end
 
-      expect(undefined.specified?).to be false
+  describe '#specified?' do
+    it 'is not defined' do
+      expect(entry.specified?).to eq false
     end
   end
 end

spec/lib/gitlab/ci/config/node/unspecified_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Node::Unspecified do
+  let(:unspecified) { described_class.new(entry) }
+  let(:entry) { spy('Entry') }
+
+  describe '#valid?' do
+    it 'delegates method to entry' do
+      expect(unspecified.valid?).to eq entry
+    end
+  end
+
+  describe '#errors' do
+    it 'delegates method to entry' do
+      expect(unspecified.errors).to eq entry
+    end
+  end
+
+  describe '#value' do
+    it 'delegates method to entry' do
+      expect(unspecified.value).to eq entry
+    end
+  end
+
+  describe '#specified?' do
+    it 'is always false' do
+      allow(entry).to receive(:specified?).and_return(true)
+
+      expect(unspecified.specified?).to be false
+    end
+  end
+end