Merge branch 'fix/hidden-build-validation-in-ci-yaml' into 'master'

Add posibility to define a hidden job without 'script' in .gitlab-ci.yml

References #15451

/cc @ayufan 

See merge request !3849

3 files changed, 122 insertions, 64 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 062a8cc4553..61254fd9b26 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -5,6 +5,7 @@ v 8.8.0 (unreleased)
 
 v 8.7.1 (unreleased)
   - Use the `can?` helper instead of `current_user.can?`
+  - Fix .gitlab-ci.yml parsing issue when hidde job is a template without script definition
 
 v 8.7.0
   - Gitlab::GitAccess and Gitlab::GitAccessWiki are now instrumented
diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index ff9887cba1e..504d3df9d34 100644
--- a/lib/ci/gitlab_ci_yaml_processor.rb
+++ b/lib/ci/gitlab_ci_yaml_processor.rb
@@ -61,23 +61,21 @@ module Ci
       @stages = @config[:stages] || @config[:types]
       @variables = @config[:variables] || {}
       @cache = @config[:cache]
+      @jobs = {}
+
       @config.except!(*ALLOWED_YAML_KEYS)
+      @config.each { |name, param| add_job(name, param) }
 
-      # anything that doesn't have script is considered as unknown
-      @config.each do |name, param|
-        raise ValidationError, "Unknown parameter: #{name}" unless param.is_a?(Hash) && param.has_key?(:script)
-      end
+      raise ValidationError, "Please define at least one job" if @jobs.none?
+    end
 
-      unless @config.values.any?{|job| job.is_a?(Hash)}
-        raise ValidationError, "Please define at least one job"
-      end
+    def add_job(name, job)
+      return if name.to_s.start_with?('.')
 
-      @jobs = {}
-      @config.each do |key, job|
-        next if key.to_s.start_with?('.')
-        stage = job[:stage] || job[:type] || DEFAULT_STAGE
-        @jobs[key] = { stage: stage }.merge(job)
-      end
+      raise ValidationError, "Unknown parameter: #{name}" unless job.is_a?(Hash) && job.has_key?(:script)
+
+      stage = job[:stage] || job[:type] || DEFAULT_STAGE
+      @jobs[name] = { stage: stage }.merge(job)
     end
 
     def build_job(name, job)
@@ -112,8 +110,6 @@ module Ci
       true
     end
 
-    private
-
     def validate_global!
       unless validate_array_of_strings(@before_script)
         raise ValidationError, "before_script should be an array of strings"
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index 643acf0303c..c7ab3185378 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -648,70 +648,131 @@ module Ci
     end
 
     describe "Hidden jobs" do
-      let(:config) do
-        YAML.dump({
-                    '.hidden_job' => { script: 'test' },
-                    'normal_job' => { script: 'test' }
-                  })
+      let(:config_processor) { GitlabCiYamlProcessor.new(config) }
+      subject { config_processor.builds_for_stage_and_ref("test", "master") }
+
+      shared_examples 'hidden_job_handling' do
+        it "doesn't create jobs that start with dot" do
+          expect(subject.size).to eq(1)
+          expect(subject.first).to eq({
+            except: nil,
+            stage: "test",
+            stage_idx: 1,
+            name: :normal_job,
+            only: nil,
+            commands: "test",
+            tag_list: [],
+            options: {},
+            when: "on_success",
+            allow_failure: false
+          })
+        end
       end
 
-      let(:config_processor) { GitlabCiYamlProcessor.new(config) }
+      context 'when hidden job have a script definition' do
+        let(:config) do
+          YAML.dump({
+                      '.hidden_job' => { image: 'ruby:2.1', script: 'test' },
+                      'normal_job' => { script: 'test' }
+                    })
+        end
 
-      subject { config_processor.builds_for_stage_and_ref("test", "master") }
+        it_behaves_like 'hidden_job_handling'
+      end
 
-      it "doesn't create jobs that starts with dot" do
-        expect(subject.size).to eq(1)
-        expect(subject.first).to eq({
-          except: nil,
-          stage: "test",
-          stage_idx: 1,
-          name: :normal_job,
-          only: nil,
-          commands: "test",
-          tag_list: [],
-          options: {},
-          when: "on_success",
-          allow_failure: false
-        })
+      context "when hidden job doesn't have a script definition" do
+        let(:config) do
+          YAML.dump({
+                      '.hidden_job' => { image: 'ruby:2.1' },
+                      'normal_job' => { script: 'test' }
+                    })
+        end
+
+        it_behaves_like 'hidden_job_handling'
       end
     end
 
     describe "YAML Alias/Anchor" do
-      it "is correctly supported for jobs" do
-        config = <<EOT
+      let(:config_processor) { GitlabCiYamlProcessor.new(config) }
+      subject { config_processor.builds_for_stage_and_ref("build", "master") }
+
+      shared_examples 'job_templates_handling' do
+        it "is correctly supported for jobs" do
+          expect(subject.size).to eq(2)
+          expect(subject.first).to eq({
+            except: nil,
+            stage: "build",
+            stage_idx: 0,
+            name: :job1,
+            only: nil,
+            commands: "execute-script-for-job",
+            tag_list: [],
+            options: {},
+            when: "on_success",
+            allow_failure: false
+          })
+          expect(subject.second).to eq({
+            except: nil,
+            stage: "build",
+            stage_idx: 0,
+            name: :job2,
+            only: nil,
+            commands: "execute-script-for-job",
+            tag_list: [],
+            options: {},
+            when: "on_success",
+            allow_failure: false
+          })
+        end
+      end
+
+      context 'when template is a job' do
+        let(:config) do
+          <<EOT
 job1: &JOBTMPL
+  stage: build
   script: execute-script-for-job
 
 job2: *JOBTMPL
 EOT
+        end
 
-        config_processor = GitlabCiYamlProcessor.new(config)
+        it_behaves_like 'job_templates_handling'
+      end
 
-        expect(config_processor.builds_for_stage_and_ref("test", "master").size).to eq(2)
-        expect(config_processor.builds_for_stage_and_ref("test", "master").first).to eq({
-          except: nil,
-          stage: "test",
-          stage_idx: 1,
-          name: :job1,
-          only: nil,
-          commands: "execute-script-for-job",
-          tag_list: [],
-          options: {},
-          when: "on_success",
-          allow_failure: false
-        })
-        expect(config_processor.builds_for_stage_and_ref("test", "master").second).to eq({
-          except: nil,
-          stage: "test",
-          stage_idx: 1,
-          name: :job2,
-          only: nil,
-          commands: "execute-script-for-job",
-          tag_list: [],
-          options: {},
-          when: "on_success",
-          allow_failure: false
-        })
+      context 'when template is a hidden job' do
+        let(:config) do
+          <<EOT
+.template: &JOBTMPL
+  stage: build
+  script: execute-script-for-job
+
+job1: *JOBTMPL
+
+job2: *JOBTMPL
+EOT
+        end
+
+        it_behaves_like 'job_templates_handling'
+      end
+
+      context 'when job adds its own keys to a template definition' do
+        let(:config) do
+          <<EOT
+.template: &JOBTMPL
+  stage: build
+
+job1:
+  <<: *JOBTMPL
+  script: execute-script-for-job
+
+job2:
+  <<: *JOBTMPL
+  script: execute-script-for-job
+EOT
+        end
+
+        it_behaves_like 'job_templates_handling'
       end
     end