Fix support for `needs:` and `parallel:`

This adds a missing support for the `parallel:`
that got broken after merging the
https://gitlab.com/gitlab-org/gitlab/merge_requests/17539.

This also improves `Undefined error` to log the error
and add correlation ID.

app/models/ci/pipeline.rb

@@ -607,8 +607,14 @@ module Ci
       rescue Gitlab::Ci::YamlProcessor::ValidationError => e
         self.yaml_errors = e.message
         nil
-      rescue
-        self.yaml_errors = 'Undefined error'
+      rescue => ex
+        self.yaml_errors = "Undefined error (#{Labkit::Correlation::CorrelationId.current_id})"
+
+        Gitlab::Sentry.track_acceptable_exception(ex, extra: {
+          project_id: project.id,
+          sha: sha,
+          ci_yaml_file: ci_yaml_file_path
+        })
         nil
       end
     end

lib/gitlab/ci/config/normalizer.rb

@@ -18,8 +18,8 @@ module Gitlab
               config[:dependencies] = expand_names(config[:dependencies])
             end
 
-            if config[:needs]
-              config[:needs] = expand_names(config[:needs])
+            if job_needs = config.dig(:needs, :job)
+              config[:needs][:job] = expand_needs(job_needs)
             end
 
             config
@@ -36,6 +36,22 @@ module Gitlab
           end
         end
 
+        def expand_needs(job_needs)
+          return unless job_needs
+
+          job_needs.flat_map do |job_need|
+            job_need_name = job_need[:name].to_sym
+
+            if all_job_names = parallelized_jobs[job_need_name]
+              all_job_names.map do |job_name|
+                { name: job_name }
+              end
+            else
+              job_need
+            end
+          end
+        end
+
         def parallelized_jobs
           strong_memoize(:parallelized_jobs) do
             @jobs_config.each_with_object({}) do |(job_name, config), hash|

spec/lib/gitlab/ci/config/normalizer_spec.rb

@@ -7,6 +7,16 @@ describe Gitlab::Ci::Config::Normalizer do
   let(:job_config) { { script: 'rspec', parallel: 5, name: 'rspec' } }
   let(:config) { { job_name => job_config } }
 
+  let(:expanded_job_names) do
+    [
+      "rspec 1/5",
+      "rspec 2/5",
+      "rspec 3/5",
+      "rspec 4/5",
+      "rspec 5/5"
+    ]
+  end
+
   describe '.normalize_jobs' do
     subject { described_class.new(config).normalize_jobs }
 
@@ -15,9 +25,7 @@ describe Gitlab::Ci::Config::Normalizer do
     end
 
     it 'has parallelized jobs' do
-      job_names = [:"rspec 1/5", :"rspec 2/5", :"rspec 3/5", :"rspec 4/5", :"rspec 5/5"]
-
-      is_expected.to include(*job_names)
+      is_expected.to include(*expanded_job_names.map(&:to_sym))
     end
 
     it 'sets job instance in options' do
@@ -43,49 +51,109 @@ describe Gitlab::Ci::Config::Normalizer do
       let(:job_name) { :"rspec 35/2" }
 
       it 'properly parallelizes job names' do
-        job_names = [:"rspec 35/2 1/5", :"rspec 35/2 2/5", :"rspec 35/2 3/5", :"rspec 35/2 4/5", :"rspec 35/2 5/5"]
+        job_names = [
+          :"rspec 35/2 1/5",
+          :"rspec 35/2 2/5",
+          :"rspec 35/2 3/5",
+          :"rspec 35/2 4/5",
+          :"rspec 35/2 5/5"
+        ]
 
         is_expected.to include(*job_names)
       end
     end
 
-    %i[dependencies needs].each do |context|
-      context "when job has #{context} on parallelized jobs" do
+    context 'for dependencies' do
+      context "when job has dependencies on parallelized jobs" do
         let(:config) do
           {
             job_name => job_config,
-            other_job: { script: 'echo 1', context => [job_name.to_s] }
+            other_job: { script: 'echo 1', dependencies: [job_name.to_s] }
           }
         end
 
-        it "parallelizes #{context}" do
-          job_names = ["rspec 1/5", "rspec 2/5", "rspec 3/5", "rspec 4/5", "rspec 5/5"]
-
-          expect(subject[:other_job][context]).to include(*job_names)
+        it "parallelizes dependencies" do
+          expect(subject[:other_job][:dependencies]).to eq(expanded_job_names)
         end
 
         it "does not include original job name in #{context}" do
-          expect(subject[:other_job][context]).not_to include(job_name)
+          expect(subject[:other_job][:dependencies]).not_to include(job_name)
         end
       end
 
-      context "when there are #{context} which are both parallelized and not" do
+      context "when there are dependencies which are both parallelized and not" do
         let(:config) do
           {
             job_name => job_config,
             other_job: { script: 'echo 1' },
-            final_job: { script: 'echo 1', context => [job_name.to_s, "other_job"] }
+            final_job: { script: 'echo 1', dependencies: [job_name.to_s, "other_job"] }
           }
         end
 
-        it "parallelizes #{context}" do
+        it "parallelizes dependencies" do
           job_names = ["rspec 1/5", "rspec 2/5", "rspec 3/5", "rspec 4/5", "rspec 5/5"]
 
-          expect(subject[:final_job][context]).to include(*job_names)
+          expect(subject[:final_job][:dependencies]).to include(*job_names)
+        end
+
+        it "includes the regular job in dependencies" do
+          expect(subject[:final_job][:dependencies]).to include('other_job')
+        end
+      end
+    end
+
+    context 'for needs' do
+      let(:expanded_job_attributes) do
+        expanded_job_names.map do |job_name|
+          { name: job_name }
+        end
+      end
+
+      context "when job has needs on parallelized jobs" do
+        let(:config) do
+          {
+            job_name => job_config,
+            other_job: {
+              script: 'echo 1',
+              needs: {
+                job: [
+                  { name: job_name.to_s }
+                ]
+              }
+            }
+          }
+        end
+
+        it "parallelizes needs" do
+          expect(subject.dig(:other_job, :needs, :job)).to eq(expanded_job_attributes)
+        end
+      end
+
+      context "when there are dependencies which are both parallelized and not" do
+        let(:config) do
+          {
+            job_name => job_config,
+            other_job: {
+              script: 'echo 1'
+            },
+            final_job: {
+              script: 'echo 1',
+              needs: {
+                job: [
+                  { name: job_name.to_s },
+                  { name: "other_job" }
+                ]
+              }
+            }
+          }
+        end
+
+        it "parallelizes dependencies" do
+          expect(subject.dig(:final_job, :needs, :job)).to include(*expanded_job_attributes)
         end
 
-        it "includes the regular job in #{context}" do
-          expect(subject[:final_job][context]).to include('other_job')
+        it "includes the regular job in dependencies" do
+          expect(subject.dig(:final_job, :needs, :job)).to include(name: 'other_job')
         end
       end
     end

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -1301,6 +1301,7 @@ module Gitlab
           {
             build1: { stage: 'build', script: 'test' },
             build2: { stage: 'build', script: 'test' },
+            parallel: { stage: 'build', script: 'test', parallel: 2 },
             test1: { stage: 'test', script: 'test', needs: needs, dependencies: dependencies },
             test2: { stage: 'test', script: 'test' },
             deploy: { stage: 'test', script: 'test' }
@@ -1317,7 +1318,7 @@ module Gitlab
           let(:needs) { %w(build1 build2) }
 
           it "does create jobs with valid specification" do
-            expect(subject.builds.size).to eq(5)
+            expect(subject.builds.size).to eq(7)
             expect(subject.builds[0]).to eq(
               stage: "build",
               stage_idx: 1,
@@ -1329,7 +1330,7 @@ module Gitlab
               allow_failure: false,
               yaml_variables: []
             )
-            expect(subject.builds[2]).to eq(
+            expect(subject.builds[4]).to eq(
               stage: "test",
               stage_idx: 2,
               name: "test1",
@@ -1345,6 +1346,27 @@ module Gitlab
           end
         end
 
+        context 'needs parallel job' do
+          let(:needs) { %w(parallel) }
+
+          it "does create jobs with valid specification" do
+            expect(subject.builds.size).to eq(7)
+            expect(subject.builds[4]).to eq(
+              stage: "test",
+              stage_idx: 2,
+              name: "test1",
+              options: { script: ["test"] },
+              needs_attributes: [
+                { name: "parallel 1/2" },
+                { name: "parallel 2/2" }
+              ],
+              when: "on_success",
+              allow_failure: false,
+              yaml_variables: []
+            )
+          end
+        end
+
         context 'undefined need' do
           let(:needs) { ['undefined'] }
 

spec/models/ci/pipeline_spec.rb

@@ -2893,6 +2893,25 @@ describe Ci::Pipeline, :mailer do
 
       it 'contains yaml errors' do
         expect(pipeline).to have_yaml_errors
+        expect(pipeline.yaml_errors).to include('contains unknown keys')
+      end
+    end
+
+    context 'when pipeline has undefined error' do
+      let(:pipeline) do
+        create(:ci_pipeline, config: {})
+      end
+
+      it 'contains yaml errors' do
+        expect(::Gitlab::Ci::YamlProcessor).to receive(:new)
+          .and_raise(RuntimeError, 'undefined failure')
+
+        expect(Gitlab::Sentry).to receive(:track_acceptable_exception)
+          .with(be_a(RuntimeError), anything)
+          .and_call_original
+
+        expect(pipeline).to have_yaml_errors
+        expect(pipeline.yaml_errors).to include('Undefined error')
       end
     end