Fix pipeline status when DAG jobs needs manual jobs

We achieve this with skipping DAG jobs when it needs a manual job.

app/models/ci/group.rb

@@ -39,7 +39,7 @@ module Ci
     def status_struct
       strong_memoize(:status_struct) do
         Gitlab::Ci::Status::Composite
-          .new(@jobs)
+          .new(@jobs, project: project)
       end
     end
 

app/models/ci/legacy_stage.rb

@@ -32,7 +32,7 @@ module Ci
     end
 
     def status
-      @status ||= statuses.latest.composite_status
+      @status ||= statuses.latest.composite_status(project: project)
     end
 
     def detailed_status(current_user)

app/models/ci/stage.rb

@@ -138,7 +138,7 @@ module Ci
     end
 
     def latest_stage_status
-      statuses.latest.composite_status || 'skipped'
+      statuses.latest.composite_status(project: project) || 'skipped'
     end
   end
 end

app/models/concerns/ci/has_status.rb

@@ -20,9 +20,11 @@ module Ci
     UnknownStatusError = Class.new(StandardError)
 
     class_methods do
-      def composite_status
+      # The parameter `project` is only used for the feature flag check, and will be removed with
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/321972
+      def composite_status(project: nil)
         Gitlab::Ci::Status::Composite
-          .new(all, with_allow_failure: columns_hash.key?('allow_failure'))
+          .new(all, with_allow_failure: columns_hash.key?('allow_failure'), project: project)
           .status
       end
 

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

@@ -78,7 +78,7 @@ module Ci
 
         def status_for_array(statuses, dag:)
           result = Gitlab::Ci::Status::Composite
-            .new(statuses, dag: dag)
+            .new(statuses, dag: dag, project: pipeline.project)
             .status
           result || 'success'
         end

config/feature_flags/development/ci_fix_pipeline_status_for_dag_needs_manual.yml

+---
+name: ci_fix_pipeline_status_for_dag_needs_manual
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/53476
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/321972
+milestone: '13.10'
+type: development
+group: group::pipeline authoring
+default_enabled: false

lib/gitlab/ci/status/composite.rb

@@ -7,7 +7,10 @@ module Gitlab
         include Gitlab::Utils::StrongMemoize
 
         # This class accepts an array of arrays/hashes/or objects
-        def initialize(all_statuses, with_allow_failure: true, dag: false)
+        #
+        # The parameter `project` is only used for the feature flag check, and will be removed with
+        # https://gitlab.com/gitlab-org/gitlab/-/issues/321972
+        def initialize(all_statuses, with_allow_failure: true, dag: false, project: nil)
           unless all_statuses.respond_to?(:pluck)
             raise ArgumentError, "all_statuses needs to respond to `.pluck`"
           end
@@ -16,6 +19,7 @@ module Gitlab
           @status_key = 0
           @allow_failure_key = 1 if with_allow_failure
           @dag = dag
+          @project = project
 
           consume_all_statuses(all_statuses)
         end
@@ -32,7 +36,7 @@ module Gitlab
           return if none?
 
           strong_memoize(:status) do
-            if @dag && any_of?(:skipped)
+            if @dag && any_skipped_or_ignored?
               # 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)
@@ -90,6 +94,14 @@ module Gitlab
             matching == @status_set.size
         end
 
+        def any_skipped_or_ignored?
+          if ::Feature.enabled?(:ci_fix_pipeline_status_for_dag_needs_manual, @project, default_enabled: :yaml)
+            any_of?(:skipped) || any_of?(:ignored)
+          else
+            any_of?(:skipped)
+          end
+        end
+
         def consume_all_statuses(all_statuses)
           columns = []
           columns[@status_key] = :status

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

@@ -69,6 +69,8 @@ RSpec.describe Gitlab::Ci::Status::Composite do
         %i(manual)           | false | 'skipped'   | false
         %i(skipped failed)   | false | 'success'   | true
         %i(skipped failed)   | true  | 'skipped'   | true
+        %i(success manual)   | true  | 'skipped'   | false
+        %i(success manual)   | false | 'success'   | false
         %i(created failed)   | false | 'created'   | true
         %i(preparing manual) | false | 'preparing' | false
       end
@@ -80,6 +82,25 @@ RSpec.describe Gitlab::Ci::Status::Composite do
 
         it_behaves_like 'compares status and warnings'
       end
+
+      context 'when FF ci_fix_pipeline_status_for_dag_needs_manual is disabled' do
+        before do
+          stub_feature_flags(ci_fix_pipeline_status_for_dag_needs_manual: false)
+        end
+
+        where(:build_statuses, :dag, :result, :has_warnings) do
+          %i(success manual) | true  | 'pending' | false
+          %i(success manual) | false | 'success' | false
+        end
+
+        with_them do
+          let(:all_statuses) do
+            build_statuses.map { |status| @statuses_with_allow_failure[status] }
+          end
+
+          it_behaves_like 'compares status and warnings'
+        end
+      end
     end
   end
 end

spec/services/ci/pipeline_processing/shared_processing_service.rb

@@ -843,6 +843,19 @@ RSpec.shared_examples 'Pipeline Processing Service' do
       create(:ci_build_need, build: deploy, name: 'linux:build')
     end
 
+    it 'makes deploy DAG to be skipped' do
+      expect(process_pipeline).to be_truthy
+
+      expect(stages).to eq(%w(skipped skipped))
+      expect(all_builds.manual).to contain_exactly(linux_build)
+      expect(all_builds.skipped).to contain_exactly(deploy)
+    end
+
+    context 'when FF ci_fix_pipeline_status_for_dag_needs_manual is disabled' do
+      before do
+        stub_feature_flags(ci_fix_pipeline_status_for_dag_needs_manual: false)
+      end
+
       it 'makes deploy DAG to be waiting for optional manual to finish' do
         expect(process_pipeline).to be_truthy
 
@@ -851,6 +864,7 @@ RSpec.shared_examples 'Pipeline Processing Service' do
         expect(all_builds.created).to contain_exactly(deploy)
       end
     end
+  end
 
   private
 

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

@@ -30,12 +30,12 @@ transitions:
   - event: success
     jobs: [build]
     expect:
-      pipeline: running
+      pipeline: success
       stages:
         build: success
         test: skipped
-        deploy: created
+        deploy: skipped
       jobs:
         build: success
         test: manual
-        deploy: created
+        deploy: skipped

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

@@ -30,12 +30,12 @@ transitions:
   - event: success
     jobs: [build]
     expect:
-      pipeline: running
+      pipeline: success
       stages:
         build: success
         test: skipped
-        deploy: created
+        deploy: skipped
       jobs:
         build: success
         test: manual
-        deploy: created
+        deploy: skipped

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

@@ -54,29 +54,29 @@ transitions:
       stages:
         build: success
         test: pending
-        review: created
-        deploy: created
+        review: skipped
+        deploy: skipped
       jobs:
         build: success
         test: pending
         release_test: manual
-        review: created
-        staging: created
-        production: created
+        review: skipped
+        staging: skipped
+        production: skipped
 
   - event: success
     jobs: [test]
     expect:
-      pipeline: running
+      pipeline: success
       stages:
         build: success
         test: success
-        review: created
-        deploy: created
+        review: skipped
+        deploy: skipped
       jobs:
         build: success
         test: success
         release_test: manual
-        review: created
-        staging: created
-        production: created
+        review: skipped
+        staging: skipped
+        production: skipped

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

@@ -12,13 +12,13 @@ config:
 
 init:
   expect:
-    pipeline: created
+    pipeline: skipped
     stages:
       test: skipped
-      deploy: created
+      deploy: skipped
     jobs:
       test: manual
-      deploy: created
+      deploy: skipped
 
 transitions:
   - event: enqueue
@@ -27,10 +27,10 @@ transitions:
       pipeline: pending
       stages:
         test: pending
-        deploy: created
+        deploy: skipped
       jobs:
         test: pending
-        deploy: created
+        deploy: skipped
 
   - event: run
     jobs: [test]
@@ -38,21 +38,18 @@ transitions:
       pipeline: running
       stages:
         test: running
-        deploy: created
+        deploy: skipped
       jobs:
         test: running
-        deploy: created
+        deploy: skipped
 
   - event: drop
     jobs: [test]
     expect:
-      pipeline: running
+      pipeline: success
       stages:
         test: success
-        deploy: pending
+        deploy: skipped
       jobs:
         test: failed
-        deploy: pending
-
-# TOOD: should we run deploy?
-# Further discussions: https://gitlab.com/gitlab-org/gitlab/-/issues/213080
+        deploy: skipped

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

@@ -13,15 +13,12 @@ config:
 
 init:
   expect:
-    pipeline: created
+    pipeline: pending
     stages:
       test: skipped
-      deploy: created
+      deploy: pending
     jobs:
       test: manual
-      deploy: created
+      deploy: pending
 
 transitions: []
-
-# TODO: should we run `deploy`?
-# Further discussions: https://gitlab.com/gitlab-org/gitlab/-/issues/213080

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

@@ -13,13 +13,13 @@ config:
 
 init:
   expect:
-    pipeline: created
+    pipeline: skipped
     stages:
       test: skipped
-      deploy: created
+      deploy: skipped
     jobs:
       test: manual
-      deploy: created
+      deploy: skipped
 
 transitions:
   - event: enqueue
@@ -28,10 +28,10 @@ transitions:
       pipeline: pending
       stages:
         test: pending
-        deploy: created
+        deploy: skipped
       jobs:
         test: pending
-        deploy: created
+        deploy: skipped
 
   - event: drop
     jobs: [test]
@@ -43,6 +43,3 @@ transitions:
       jobs:
         test: failed
         deploy: skipped
-
-# TODO: should we run `deploy`?
-# Further discussions: https://gitlab.com/gitlab-org/gitlab/-/issues/213080

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

@@ -19,24 +19,21 @@ init:
     pipeline: pending
     stages:
       test: pending
-      deploy: created
+      deploy: skipped
     jobs:
       test1: pending
       test2: manual
-      deploy: created
+      deploy: skipped
 
 transitions:
   - event: success
     jobs: [test1]
     expect:
-      pipeline: running
+      pipeline: success
       stages:
         test: success
-        deploy: created
+        deploy: skipped
       jobs:
         test1: success
         test2: manual
-        deploy: created
-
-# TODO: should deploy run?
-# Further discussions: https://gitlab.com/gitlab-org/gitlab/-/issues/213080
+        deploy: skipped