Skip to content

WIP: Implement skipping ignored jobs in CI

Furkan Ayhan requested to merge 31264-bug-when-using-dag-with-manual into master

What does this MR do?

This MR fixes the bug stated in #31264 (closed), reverts the change in !23405 (merged) and fixes #31526 (closed) again :)

We have problems with handling skipped statuses.

As @fabiopitino stated, ignored and skipped are two different statuses. Actually there is no such status ignored. We handle ignored and skipped in same way. And that causes problems.

From the perspective of Gitlab::Ci::Status::Composite;

  • ignored means; Job has allow_failure:true & either it is failed/cancelled or it is manual and waiting for an action.
  • skipped means; Job has when:manual & allow_failure:true and waiting for an action.
    • It also means that the job is ignored. And that's the wrong part of it.

We should treat skipped and ignored differently, because skipped is not a "success" case, but ignored is. And if I understand correctly, it is the reason why we first added skipped into the "success" states in this commit: 6d80b94a

Why this problem arises right now?

We first tried to handle this confusion for the issue #31526 (closed) and MR !23405 (merged). I think we went for a wrong way to solve the problem.

Here is the actual problem.

Please follow these two examples:

Example 1

build:
  stage: build
  script: exit 1
  
rspec:
  stage: test
  script: echo rspec

deploy:
  stage: deploy
  script: echo deploy

When we have that, all_statuses passing into Gitlab::Ci::Status::Composite for deploy is this:

[
    {
                   :id => 1,
                 :name => "build",
               :status => "failed",
        :allow_failure => false,
            :stage_idx => 0,
            :processed => false,
         :lock_version => 3
    },
    {
                   :id => 2,
                 :name => "rspec",
               :status => "skipped",
        :allow_failure => false,
            :stage_idx => 1,
            :processed => true,
         :lock_version => 2
    }
]

and we get failed.

Example 2

build:
  stage: build
  script: exit 1
  
rspec:
  stage: test
  script: echo rspec

deploy:
  stage: deploy
  script: echo deploy
  needs: [rspec]

When we have that, all_statuses passing into Gitlab::Ci::Status::Composite for deploy is this:

[
    {
                   :id => 2,
                 :name => "rspec",
               :status => "skipped",
        :allow_failure => false,
            :stage_idx => 1,
            :processed => true,
         :lock_version => 2
    }
]

and we get skipped.

Since skipped is a "success" case, deploy will run. That was the problem in #31526 (closed), and "fixed" in !23405 (merged).

Key Points

  • This MR is a proposal. Instead of introducing a new status ignored (if only_of?(:ignored) then :ignored), I've tried to make a workaround to pass the "ignored" information to Ci::ProcessBuildService.
  • I've added some test cases to spec/services/ci/pipeline_processing/shared_processing_service.rb. And those are passed for Ci::PipelineProcessing::AtomicProcessingService.
  • I don't know how to make these changes for Ci::PipelineProcessing::LegacyProcessingService ❗
  • Of course there are so many missing and failing tests 🚫

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Thao Yeager

Merge request reports