Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: auto-cancel PipelineRuns on PR close #1867

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: auto-cancel PipelineRuns on PR close #1867

wants to merge 1 commit into from

Conversation

chmouel
Copy link
Member

@chmouel chmouel commented Dec 16, 2024 •
edited
Loading

Changes

The pipelinesascode.tekton.dev/cancel-in-progress: "true" feature
annotation has now been enhanced to include automatic cancellation of
PipelineRuns when the associated pull request is closed/declined or merged.

Jira: https://issues.redhat.com/browse/SRVKP-6908

Supported providers:

Git Provider Supported
GitHub App ✅️
GitHub Webhook ✅️
Gitea ✅️
GitLab ✅️
Bitbucket Cloud ✅️
Bitbucket Server ❌️

Submitter Checklist

  • 📝 Please ensure your commit message is clear and informative. For guidance on crafting effective commit messages, refer to the How to write a git commit message guide. We prefer the commit message to be included in the PR body itself rather than a link to an external website (ie: Jira ticket).
  • ♽ Before submitting a PR, run make test lint to avoid unnecessary CI processing. For an even more efficient workflow, consider installing pre-commit and running pre-commit install in the root of this repository.
  • ✨ We use linters to maintain clean and consistent code. Please ensure you've run make lint before submitting a PR. Some linters offer a --fix mode, which can be executed with the command make fix-linters (ensure markdownlint and golangci-lint tools are installed first).
  • 📖 If you're introducing a user-facing feature or changing existing behavior, please ensure it's properly documented.
  • 🧪 While 100% coverage isn't a requirement, we encourage unit tests for any code changes where possible.
  • 🎁 If feasible, please check if an end-to-end test can be added. See README for more details.
  • 🔎 If there's any flakiness in the CI tests, don't necessarily ignore it. It's better to address the issue before merging, or provide a valid reason to bypass it if fixing isn't possible (e.g., token rate limitations).

@codecov Codecov
Copy link

codecov bot commented Dec 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 60.67416% with 35 lines in your changes missing coverage. Please review.

Project coverage is 65.71%. Comparing base (7db78c0) to head (ff7c7be).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
pkg/provider/github/parse_payload.go 50.00% 6 Missing and 3 partials ⚠️
pkg/pipelineascode/cancel_pipelineruns.go 86.04% 4 Missing and 2 partials ⚠️
pkg/pipelineascode/pipelineascode.go 0.00% 4 Missing and 2 partials ⚠️
pkg/provider/gitea/parse_payload.go 0.00% 3 Missing ⚠️
pkg/provider/github/detect.go 0.00% 2 Missing and 1 partial ⚠️
pkg/provider/github/status.go 0.00% 3 Missing ⚠️
pkg/provider/gitlab/parse_payload.go 25.00% 2 Missing and 1 partial ⚠️
pkg/pipelineascode/match.go 0.00% 1 Missing ⚠️
pkg/reconciler/reconciler.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1867      +/-   ##
==========================================
- Coverage   65.74%   65.71%   -0.04%     
==========================================
  Files         178      178              
  Lines       13836    13886      +50     
==========================================
+ Hits         9097     9125      +28     
- Misses       4124     4144      +20     
- Partials      615      617       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chmouel chmouel force-pushed the SRVKP-6908-add-support-for-cancel-in-progress-when-pull-request-is-closedmerged branch 13 times, most recently from 47f1470 to 5de31a6 Compare December 18, 2024 15:24
@chmouel
Copy link
Member Author

chmouel commented Dec 18, 2024

/test go-testing

@chmouel chmouel force-pushed the SRVKP-6908-add-support-for-cancel-in-progress-when-pull-request-is-closedmerged branch 2 times, most recently from ba4e927 to 3495b53 Compare December 20, 2024 16:44
@chmouel chmouel force-pushed the SRVKP-6908-add-support-for-cancel-in-progress-when-pull-request-is-closedmerged branch from f086178 to 92aefdf Compare January 14, 2025 20:37
@chmouel chmouel mentioned this pull request Jan 15, 2025
Open
13 tasks
@chmouel chmouel force-pushed the SRVKP-6908-add-support-for-cancel-in-progress-when-pull-request-is-closedmerged branch 2 times, most recently from b26e7be to 11c3a3c Compare January 15, 2025 09:35
@chmouel chmouel requested review from vdemeester and zakisk January 15, 2025 09:37
@chmouel chmouel marked this pull request as ready for review January 15, 2025 09:37
@chmouel
feat: auto-cancel PipelineRuns on PR close
e5f70c1 
The pipelinesascode.tekton.dev/cancel-in-progress: "true" feature
annotation has now been enhanced to include automatic cancellation of
PipelineRuns when the associated pull request is closed or merged.

Jira: https://issues.redhat.com/browse/SRVKP-6908
Signed-off-by: Chmouel Boudjnah <[email protected]>
@chmouel chmouel force-pushed the SRVKP-6908-add-support-for-cancel-in-progress-when-pull-request-is-closedmerged branch from 11c3a3c to e5f70c1 Compare January 15, 2025 11:11
vdemeester
vdemeester reviewed Jan 15, 2025
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment about cancelling the Pending ones.

pkg/pipelineascode/cancel_pipelineruns.go
continue
}

if pr.IsPending() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to skip pending ones ? Would it match pipelineruns from new commits/push ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's pending there is nothing to cancel?

deleting the prun directly could be an issue for the concurrency q and i'd like to don't mess with that...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and furthermore cancel-in-progress is explicitely documented as not working with the concurrency feature (mostly because of the complexitiy involved for such a simple implementation) so that should not be supported...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chmouel but a pending pipelinerun is waiting to be started or cancelled. If we do not cancel, it could stay here forever.

@zakisk
Copy link
Contributor

zakisk commented Jan 15, 2025

/test go-testing

1 similar comment
@chmouel
Copy link
Member Author

chmouel commented Jan 15, 2025

/test go-testing

@chmouel chmouel requested a review from vdemeester January 15, 2025 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zakisk zakisk Awaiting requested review from zakisk

@vdemeester vdemeester Awaiting requested review from vdemeester

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chmouel @zakisk @vdemeester