Refactor CI YAML processor's validators

2 files changed, 43 insertions, 29 deletions

diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index 3beafcad117..9c11fc3c81d 100644
--- a/lib/ci/gitlab_ci_yaml_processor.rb
+++ b/lib/ci/gitlab_ci_yaml_processor.rb
@@ -132,26 +132,36 @@ module Ci
     end
 
     def validate_job!(name, job)
+      validate_job_name!(name)
+      validate_job_keys!(name, job)
+      validate_job_types!(name, job)
+
+      validate_job_stage!(name, job) if job[:stage]
+      validate_job_cache!(name, job) if job[:cache]
+      validate_job_artifacts!(name, job) if job[:artifacts]
+    end
+
+    private
+
+    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)
       job.keys.each do |key|
         unless ALLOWED_JOB_KEYS.include? key
           raise ValidationError, "#{name} job: unknown parameter #{key}"
         end
       end
+    end
 
+    def validate_job_types!(name, job)
       if !validate_string(job[:script]) && !validate_array_of_strings(job[:script])
         raise ValidationError, "#{name} job: script should be a string or an array of a strings"
       end
 
-      if job[:stage]
-        unless job[:stage].is_a?(String) && job[:stage].in?(stages)
-          raise ValidationError, "#{name} job: stage parameter should be #{stages.join(", ")}"
-        end
-      end
-
       if job[:image] && !validate_string(job[:image])
         raise ValidationError, "#{name} job: image should be a string"
       end
@@ -172,36 +182,40 @@ module Ci
         raise ValidationError, "#{name} job: except parameter should be an array of strings"
       end
 
-      if job[:cache]
-        if job[:cache][:untracked] && !validate_boolean(job[:cache][:untracked])
-          raise ValidationError, "#{name} job: cache:untracked parameter should be an boolean"
-        end
-
-        if job[:cache][:paths] && !validate_array_of_strings(job[:cache][:paths])
-          raise ValidationError, "#{name} job: cache:paths parameter should be an array of strings"
-        end
+      if job[:allow_failure] && !validate_boolean(job[:allow_failure])
+        raise ValidationError, "#{name} job: allow_failure parameter should be an boolean"
       end
 
-      if job[:artifacts]
-        if job[:artifacts][:untracked] && !validate_boolean(job[:artifacts][:untracked])
-          raise ValidationError, "#{name} job: artifacts:untracked parameter should be an boolean"
-        end
+      if job[:when] && !job[:when].in?(%w(on_success on_failure always))
+        raise ValidationError, "#{name} job: when parameter should be on_success, on_failure or always"
+      end
+    end
 
-        if job[:artifacts][:paths] && !validate_array_of_strings(job[:artifacts][:paths])
-          raise ValidationError, "#{name} job: artifacts:paths parameter should be an array of strings"
-        end
+    def validate_job_stage!(name, job)
+      unless job[:stage].is_a?(String) && job[:stage].in?(stages)
+        raise ValidationError, "#{name} job: stage parameter should be #{stages.join(", ")}"
       end
+    end
 
-      if job[:allow_failure] && !validate_boolean(job[:allow_failure])
-        raise ValidationError, "#{name} job: allow_failure parameter should be an boolean"
+    def validate_job_cache!(name, job)
+      if job[:cache][:untracked] && !validate_boolean(job[:cache][:untracked])
+        raise ValidationError, "#{name} job: cache:untracked parameter should be an boolean"
       end
 
-      if job[:when] && !job[:when].in?(%w(on_success on_failure always))
-        raise ValidationError, "#{name} job: when parameter should be on_success, on_failure or always"
+      if job[:cache][:paths] && !validate_array_of_strings(job[:cache][:paths])
+        raise ValidationError, "#{name} job: cache:paths parameter should be an array of strings"
       end
     end
 
-    private
+    def validate_job_artifacts!(name, job)
+      if job[:artifacts][:untracked] && !validate_boolean(job[:artifacts][:untracked])
+        raise ValidationError, "#{name} job: artifacts:untracked parameter should be an boolean"
+      end
+
+      if job[:artifacts][:paths] && !validate_array_of_strings(job[:artifacts][:paths])
+        raise ValidationError, "#{name} job: artifacts:paths parameter should be an array of strings"
+      end
+    end
 
     def validate_array_of_strings(values)
       values.is_a?(Array) && values.all? { |value| validate_string(value) }
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index 6f287719ba6..0d50f20ae34 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -532,21 +532,21 @@ module Ci
       end
 
       it "returns errors if job stage is not a string" do
-        config = YAML.dump({ rspec: { script: "test", type: 1, allow_failure: "string" } })
+        config = YAML.dump({ rspec: { script: "test", type: 1 } })
         expect do
           GitlabCiYamlProcessor.new(config, path)
         end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: stage parameter should be build, test, deploy")
       end
 
       it "returns errors if job stage is not a pre-defined stage" do
-        config = YAML.dump({ rspec: { script: "test", type: "acceptance", allow_failure: "string" } })
+        config = YAML.dump({ rspec: { script: "test", type: "acceptance" } })
         expect do
           GitlabCiYamlProcessor.new(config, path)
         end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: stage parameter should be build, test, deploy")
       end
 
       it "returns errors if job stage is not a defined stage" do
-        config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", type: "acceptance", allow_failure: "string" } })
+        config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", type: "acceptance" } })
         expect do
           GitlabCiYamlProcessor.new(config, path)
         end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: stage parameter should be build, test")