ignore_skipped not working as expected
Summary
If you want the pipeline status badge to only display the last non-skipped status, the "ignore_skipped" feature is not working as expected.
Steps to reproduce
- push to branch and pipeline succeeds -> passed
- push to branch with "[ci skip]" in commit message -> skipped
The badge for the pipeline should be "passed" if "?ignore_skipped=true" is appended to the badge url.
According to ~~~~~~~~~~~~https://docs.gitlab.com/ee/ci/pipelines/settings.html#display-only-non-skipped-status
According to https://docs.gitlab.com/ee/user/project/badges.html#display-only-non-skipped-status
Example Project
https://gitlab.com/benjaminfuchs/test/-/commits/master
What is the current bug behavior?
Badge shows "unknown"
What is the expected correct behavior?
"unknown" should be "passed"
Relevant logs and/or screenshots
Output of checks
This bug happens on GitLab.com
Possible fixes
Original Suggestion:
https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/badge/pipeline/status.rb#L31
I seems to me "pipelines" only contains pipelines for the same commit hash. But what I would expect is to look for all pipelines of the branch and return the latest status that is not "skipped".So instead of ...
pipelines = @project.ci_pipelines
.where(sha: @sha)
... this would be correct:
pipelines = @project.ci_pipelines
.where(ref: @ref)
Implementation Guide
From the changes and review notes in !59593 (closed), remove the sha
filter from Gitlab::Ci::Badge::Pipeline::Status#status
- pipelines = @project.ci_pipelines
- .where(sha: @sha)
-
- relation = @ignore_skipped ? pipelines.without_statuses([:skipped]) : pipelines
+ relation = @project.all_pipelines.ci_sources.for_branch(@ref)
+ relation = relation.without_statuses([:skipped]) if @ignore_skipped
relation.latest_status(@ref) || 'unknown'
(New)* We can also do,
relation = @project.ci_pipelines.for_branch(@ref)
relation = relation.without_statuses([:skipped]) if @ignore_skipped
because @project.ci_pipelines == @project.all_pipelines.ci_sources
ci_sources
:
- https://gitlab.com/gitlab-org/gitlab/-/blob/4966546283a8e47c26d42da25b72330e5fe3325a/app/models/ci/pipeline.rb#L304
- https://gitlab.com/gitlab-org/gitlab/-/blob/4966546283a8e47c26d42da25b72330e5fe3325a/app/models/concerns/enums/ci/pipeline.rb#L55-59
- I think we should use this scope because there are other pipeline types that should not change the CI status of the branch, like child pipelines.
for_branch
:
- https://gitlab.com/gitlab-org/gitlab/-/blob/4966546283a8e47c26d42da25b72330e5fe3325a/app/models/ci/pipeline.rb#L312
- This scope removes the pipelines for tags. From the docs, it looks like this feature is intended only for branches.
latest_status
:
- https://gitlab.com/gitlab-org/gitlab/-/blob/4966546283a8e47c26d42da25b72330e5fe3325a/app/models/ci/pipeline.rb#L346-369
- This orders the pipelines so we get the latest one.
There is a test that fails and it needs to be updated because the
old_pipeline
is created after thepipeline
that's expected:[6] pry(#<RSpec::ExampleGroups::GitlabCiBadgePipelineStatus::PipelineExists::WhenOutdatedPipelineForGivenRefExists>)> pipeline.id => 3 [7] pry(#<RSpec::ExampleGroups::GitlabCiBadgePipelineStatus::PipelineExists::WhenOutdatedPipelineForGivenRefExists>)> old_pipeline.id => 4
By removing the need to load commits out of the repository to compare the SHA, we can broaden our search across recent statuses in a more performant way: just searching through pipelines in the database. Since we've restricted the source
types to non-dangling CI sources, sorting by pipeline ID becomes a pretty effective approximation of commit order on the ref. This was recently discussed in a Verify Technical Discussions call and we agreed it was effective enough to use.