Merge branch '213080-fix-skipped-status-problem-of-needs' into 'master'

Fix skipped status of DAG pipelines

See merge request gitlab-org/gitlab!39205

app/services/ci/pipeline_processing/atomic_processing_service/status_collection.rb

@@ -77,15 +77,8 @@ module Ci
         private
 
         def status_for_array(statuses, dag:)
-          # TODO: This is hack to support
-          # the same exact behaviour for Atomic and Legacy processing
-          # that DAG is blocked from executing if dependent is not "complete"
-          if dag && statuses.any? { |status| Ci::HasStatus::COMPLETED_STATUSES.exclude?(status[:status]) }
-            return 'pending'
-          end
-
           result = Gitlab::Ci::Status::Composite
-            .new(statuses)
+            .new(statuses, dag: dag)
             .status
           result || 'success'
         end

changelogs/unreleased/213080-fix-skipped-status-problem-of-needs.yml

+---
+title: Fix skipped status of DAG pipelines
+merge_request: 39205
+author:
+type: fixed

lib/gitlab/ci/status/composite.rb

@@ -7,7 +7,7 @@ module Gitlab
         include Gitlab::Utils::StrongMemoize
 
         # This class accepts an array of arrays/hashes/or objects
-        def initialize(all_statuses, with_allow_failure: true)
+        def initialize(all_statuses, with_allow_failure: true, dag: false)
           unless all_statuses.respond_to?(:pluck)
             raise ArgumentError, "all_statuses needs to respond to `.pluck`"
           end
@@ -15,6 +15,7 @@ module Gitlab
           @status_set = Set.new
           @status_key = 0
           @allow_failure_key = 1 if with_allow_failure
+          @dag = dag
 
           consume_all_statuses(all_statuses)
         end
@@ -31,7 +32,13 @@ module Gitlab
           return if none?
 
           strong_memoize(:status) do
-            if only_of?(:skipped, :ignored)
+            if @dag && any_of?(:skipped)
+              # The DAG job is skipped if one of the needs does not run at all.
+              'skipped'
+            elsif @dag && !only_of?(:success, :failed, :canceled, :skipped, :success_with_warnings)
+              # DAG is blocked from executing if a dependent is not "complete"
+              'pending'
+            elsif only_of?(:skipped, :ignored)
               'skipped'
             elsif only_of?(:success, :skipped, :success_with_warnings, :ignored)
               'success'

spec/lib/gitlab/ci/status/composite_spec.rb

@@ -20,7 +20,7 @@ RSpec.describe Gitlab::Ci::Status::Composite do
 
     shared_examples 'compares status and warnings' do
       let(:composite_status) do
-        described_class.new(all_statuses)
+        described_class.new(all_statuses, dag: dag)
       end
 
       it 'returns status and warnings?' do
@@ -30,21 +30,29 @@ RSpec.describe Gitlab::Ci::Status::Composite do
     end
 
     context 'allow_failure: false' do
-      where(:build_statuses, :result, :has_warnings) do
-        %i(skipped) | 'skipped' | false
-        %i(skipped success) | 'success' | false
-        %i(created) | 'created' | false
-        %i(preparing) | 'preparing' | false
-        %i(canceled success skipped) | 'canceled' | false
-        %i(pending created skipped) | 'pending' | false
-        %i(pending created skipped success) | 'running' | false
-        %i(running created skipped success) | 'running' | false
-        %i(success waiting_for_resource) | 'waiting_for_resource' | false
-        %i(success manual) | 'manual' | false
-        %i(success scheduled) | 'scheduled' | false
-        %i(created preparing) | 'preparing' | false
-        %i(created success pending) | 'running' | false
-        %i(skipped success failed) | 'failed' | false
+      where(:build_statuses, :dag, :result, :has_warnings) do
+        %i(skipped)                         | false | 'skipped'              | false
+        %i(skipped success)                 | false | 'success'              | false
+        %i(skipped success)                 | true  | 'skipped'              | false
+        %i(created)                         | false | 'created'              | false
+        %i(preparing)                       | false | 'preparing'            | false
+        %i(canceled success skipped)        | false | 'canceled'             | false
+        %i(canceled success skipped)        | true  | 'skipped'              | false
+        %i(pending created skipped)         | false | 'pending'              | false
+        %i(pending created skipped success) | false | 'running'              | false
+        %i(running created skipped success) | false | 'running'              | false
+        %i(pending created skipped)         | true  | 'skipped'              | false
+        %i(pending created skipped success) | true  | 'skipped'              | false
+        %i(running created skipped success) | true  | 'skipped'              | false
+        %i(success waiting_for_resource)    | false | 'waiting_for_resource' | false
+        %i(success manual)                  | false | 'manual'               | false
+        %i(success scheduled)               | false | 'scheduled'            | false
+        %i(created preparing)               | false | 'preparing'            | false
+        %i(created success pending)         | false | 'running'              | false
+        %i(skipped success failed)          | false | 'failed'               | false
+        %i(skipped success failed)          | true  | 'skipped'              | false
+        %i(success manual)                  | true  | 'pending'              | false
+        %i(success failed created)          | true  | 'pending'              | false
       end
 
       with_them do
@@ -57,11 +65,12 @@ RSpec.describe Gitlab::Ci::Status::Composite do
     end
 
     context 'allow_failure: true' do
-      where(:build_statuses, :result, :has_warnings) do
-        %i(manual) | 'skipped' | false
-        %i(skipped failed) | 'success' | true
-        %i(created failed) | 'created' | true
-        %i(preparing manual) | 'preparing' | false
+      where(:build_statuses, :dag, :result, :has_warnings) do
+        %i(manual)           | false | 'skipped'   | false
+        %i(skipped failed)   | false | 'success'   | true
+        %i(skipped failed)   | true  | 'skipped'   | true
+        %i(created failed)   | false | 'created'   | true
+        %i(preparing manual) | false | 'preparing' | false
       end
 
       with_them do

spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds_deploy_needs_one_build_and_test.yml

@@ -47,16 +47,13 @@ transitions:
   - event: drop
     jobs: [build_2]
     expect:
-      pipeline: running
+      pipeline: failed
       stages:
         build: failed
         test: skipped
-        deploy: pending
+        deploy: skipped
       jobs:
         build_1: success
         build_2: failed
         test: skipped
-        deploy: pending
-
-# TODO: should we run deploy?
-# Further discussions: https://gitlab.com/gitlab-org/gitlab/-/issues/213080
+        deploy: skipped

spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure_deploy_needs_one_build_and_test.yml

@@ -33,31 +33,14 @@ init:
 transitions:
   - event: success
     jobs: [build_1, build_2]
-    expect:
-      pipeline: running
-      stages:
-        build: success
-        test: skipped
-        deploy: pending
-      jobs:
-        build_1: success
-        build_2: success
-        test: skipped
-        deploy: pending
-
-  - event: success
-    jobs: [deploy]
     expect:
       pipeline: success
       stages:
         build: success
         test: skipped
-        deploy: success
+        deploy: skipped
       jobs:
         build_1: success
         build_2: success
         test: skipped
-        deploy: success
-
-# TODO: should we run deploy?
-# Further discussions: https://gitlab.com/gitlab-org/gitlab/-/issues/213080
+        deploy: skipped