Improve CI job entry validations in new config

9 files changed, 54 insertions, 39 deletions

diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index 61075d3b923..e18d5b907b4 100644
--- a/lib/ci/gitlab_ci_yaml_processor.rb
+++ b/lib/ci/gitlab_ci_yaml_processor.rb
@@ -102,7 +102,6 @@ module Ci
     def validate_job!(name, job)
       raise ValidationError, "Unknown parameter: #{name}" unless job.is_a?(Hash) && job.has_key?(:script)
 
-      validate_job_name!(name)
       validate_job_keys!(name, job)
       validate_job_types!(name, job)
 
@@ -112,12 +111,6 @@ module Ci
       validate_job_dependencies!(name, job) if job[:dependencies]
     end
 
-    def validate_job_name!(name)
-      if name.blank? || !validate_string(name)
-        raise ValidationError, "job name should be non-empty string"
-      end
-    end
-
     def validate_job_keys!(name, job)
       ##
       # TODO, remove refactoring keys
diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb
index 8bd752b0e2a..b33d743e2c3 100644
--- a/lib/gitlab/ci/config/node/configurable.rb
+++ b/lib/gitlab/ci/config/node/configurable.rb
@@ -55,8 +55,10 @@ module Gitlab
                 end
 
                 define_method("#{symbol}_value") do
-                  return unless valid?
-                  @entries[symbol].value if @entries[symbol]
+                  if @entries[symbol]
+                    return unless @entries[symbol].valid?
+                    @entries[symbol].value
+                  end
                 end
 
                 alias_method symbol.to_sym, "#{symbol}_value".to_sym
diff --git a/lib/gitlab/ci/config/node/job.rb b/lib/gitlab/ci/config/node/job.rb
index bb1c3386bd4..822b428926a 100644
--- a/lib/gitlab/ci/config/node/job.rb
+++ b/lib/gitlab/ci/config/node/job.rb
@@ -10,7 +10,12 @@ module Gitlab
 
           validations do
             validates :config, presence: true
-            validates :global, required: true, on: :processed
+
+            with_options on: :processed do
+              validates :global, required: true
+              validates :name, presence: true
+              validates :name, type: Symbol
+            end
           end
 
           node :before_script, Script,
@@ -30,11 +35,11 @@ module Gitlab
 
           helpers :before_script, :script, :stage, :type, :after_script
 
+          def name
+            @key
+          end
+
           def value
-            ##
-            # TODO, refactoring step: do not expose internal configuration,
-            # return only hash value without merging it to internal config.
-            #
             @config.merge(to_hash.compact)
           end
 
@@ -53,10 +58,9 @@ module Gitlab
           private
 
           def to_hash
-            { before_script: before_script,
-              script: script,
-              commands: commands,
+            { script: script,
               stage: stage,
+              commands: commands,
               after_script: after_script }
           end
 
diff --git a/lib/gitlab/ci/config/node/jobs.rb b/lib/gitlab/ci/config/node/jobs.rb
index 548441df37c..3c1851b9fea 100644
--- a/lib/gitlab/ci/config/node/jobs.rb
+++ b/lib/gitlab/ci/config/node/jobs.rb
@@ -10,11 +10,12 @@ module Gitlab
 
           validations do
             validates :config, type: Hash
-            validate :jobs_presence, on: :processed
 
-            def jobs_presence
-              unless relevant?
-                errors.add(:config, 'should contain at least one visible job')
+            with_options on: :processed do
+              validate do
+                unless has_visible_job?
+                  errors.add(:config, 'should contain at least one visible job')
+                end
               end
             end
           end
@@ -23,7 +24,7 @@ module Gitlab
             @config
           end
 
-          def relevant?
+          def has_visible_job?
             @entries.values.any?(&:relevant?)
           end
 
diff --git a/lib/gitlab/ci/config/node/validator.rb b/lib/gitlab/ci/config/node/validator.rb
index dcfeb194374..ca000f245aa 100644
--- a/lib/gitlab/ci/config/node/validator.rb
+++ b/lib/gitlab/ci/config/node/validator.rb
@@ -31,8 +31,15 @@ module Gitlab
 
           def location
             predecessors = ancestors.map(&:key).compact
-            current = key || @node.class.name.demodulize.underscore.humanize
-            predecessors.append(current).join(':')
+            predecessors.append(key_name).join(':')
+          end
+
+          def key_name
+            if key.blank? || key.nil?
+              @node.class.name.demodulize.underscore.humanize
+            else
+              key
+            end
           end
         end
       end
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index 1017f79cc6e..e88f5cfc6dd 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -998,14 +998,14 @@ EOT
         config = YAML.dump({ '' => { script: "test" } })
         expect do
           GitlabCiYamlProcessor.new(config, path)
-        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "job name should be non-empty string")
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:job name can't be blank")
       end
 
       it "returns errors if job name is non-string" do
         config = YAML.dump({ 10 => { script: "test" } })
         expect do
           GitlabCiYamlProcessor.new(config, path)
-        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "job name should be non-empty string")
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:10 name should be a symbol")
       end
 
       it "returns errors if job image parameter is invalid" do
diff --git a/spec/lib/gitlab/ci/config/node/global_spec.rb b/spec/lib/gitlab/ci/config/node/global_spec.rb
index f46359f7ee6..fa5ff016995 100644
--- a/spec/lib/gitlab/ci/config/node/global_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/global_spec.rb
@@ -129,14 +129,12 @@ describe Gitlab::Ci::Config::Node::Global do
         describe '#jobs' do
           it 'returns jobs configuration' do
             expect(global.jobs)
-              .to eq(rspec: { before_script: %w[ls pwd],
-                              script: %w[rspec ls],
-                              commands: "ls\npwd\nrspec\nls",
-                              stage: 'test' },
-                     spinach: { before_script: %w[ls pwd],
-                                script: %w[spinach],
-                                commands: "ls\npwd\nspinach",
-                                stage: 'test' })
+              .to eq(rspec: { script: %w[rspec ls],
+                              stage: 'test',
+                              commands: "ls\npwd\nrspec\nls" },
+                     spinach: { script: %w[spinach],
+                                stage: 'test',
+                                commands: "ls\npwd\nspinach" })
           end
         end
       end
diff --git a/spec/lib/gitlab/ci/config/node/job_spec.rb b/spec/lib/gitlab/ci/config/node/job_spec.rb
index 816c0f275d6..2ac7509cb6d 100644
--- a/spec/lib/gitlab/ci/config/node/job_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/job_spec.rb
@@ -1,8 +1,9 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::Node::Job do
-  let(:entry) { described_class.new(config, global: global) }
-  let(:global) { spy('Global') }
+  let(:entry) { described_class.new(config, attributes) }
+  let(:attributes) { { key: :rspec, global: global } }
+  let(:global) { double('global', stages: %w[test]) }
 
   before do
     entry.process!
@@ -18,6 +19,15 @@ describe Gitlab::Ci::Config::Node::Job do
           expect(entry).to be_valid
         end
       end
+
+      context 'when job name is empty' do
+        let(:attributes) { { key: '', global: global } }
+
+        it 'reports error' do
+          expect(entry.errors)
+            .to include "job name can't be blank"
+        end
+      end
     end
 
     context 'when entry value is not correct' do
@@ -27,7 +37,7 @@ describe Gitlab::Ci::Config::Node::Job do
         describe '#errors' do
           it 'reports error about a config type' do
             expect(entry.errors)
-              .to include 'job config should be a hash'
+              .to include 'rspec config should be a hash'
           end
         end
       end
diff --git a/spec/lib/gitlab/ci/config/node/jobs_spec.rb b/spec/lib/gitlab/ci/config/node/jobs_spec.rb
index 60ab1d2150d..fe7ae61ed81 100644
--- a/spec/lib/gitlab/ci/config/node/jobs_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/jobs_spec.rb
@@ -34,8 +34,8 @@ describe Gitlab::Ci::Config::Node::Jobs do
         context 'when job is unspecified' do
           let(:config) { { rspec: nil } }
 
-          it 'is not valid' do
-            expect(entry).not_to be_valid
+          it 'reports error' do
+            expect(entry.errors).to include "rspec config can't be blank"
           end
         end