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.

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

fix: Fix cancel in progress to act on same pr/branch #1826

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

chmouel
Copy link
Member

@chmouel chmouel commented Nov 21, 2024

Changes

We need to ensure that we only cancel the pipeline run that is running
on the same branch as the current PR or push event. This ensures that we
don't cancel the wrong pipeline run.

On PullRequest events, we only cancel the pipeline run that is running
on the same Pull Request number.

On Push events, we only cancel the pipeline run that is running on the
same SourceBranch (which is the same as HeadBranch on push)

This avoids the issue when you have multiple pull request running on a
repo and we don't cancel the wrong ones that are running for a different
PR.

Tried to do e2e tests but it ended too flaky.

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).
    X
  • ♽ 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.
    X
  • ✨ 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).
    X
  • 📖 If you're introducing a user-facing feature or changing existing behavior, please ensure it's properly documented.
    X
  • 🧪 While 100% coverage isn't a requirement, we encourage unit tests for any code changes where possible.
    X
  • 🎁 If feasible, please check if an end-to-end test can be added. See README for more details.
    X
  • 🔎 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).

Copy link

cloudflare-workers-and-pages bot commented Nov 21, 2024

Deploying pipelines-as-code with  Cloudflare Pages  Cloudflare Pages

Latest commit: bb7375f
Status: ✅  Deploy successful!
Preview URL: https://d4b97290.pipelines-as-code.pages.dev
Branch Preview URL: https://fix-cancel-in-progress.pipelines-as-code.pages.dev

View logs

@chmouel
Copy link
Member Author

chmouel commented Nov 21, 2024

I have a new laptop and the setup create pr against origin instead of my fork, should be fixed for next pr

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 65.34%. Comparing base (eb50fac) to head (bb7375f).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pkg/pipelineascode/cancel_pipelineruns.go 83.33% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1826      +/-   ##
==========================================
+ Coverage   65.28%   65.34%   +0.06%     
==========================================
  Files         177      177              
  Lines       13674    13692      +18     
==========================================
+ Hits         8927     8947      +20     
  Misses       4143     4143              
+ Partials      604      602       -2     

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


🚨 Try these New Features:

@chmouel
Copy link
Member Author

chmouel commented Nov 21, 2024

copyng a comment i made offline on a slack discussion:

maybe in the future we would need to have concurrency groups to let the user choose their groupping
that may come up whenever we integrate that feature in a new concurrency abstracted from pac controller (ie: the far future)
but i think the way that pr implemented the grouping is good and conservative enough for the most common use case 

@chmouel chmouel force-pushed the fix-cancel-in-progress branch 3 times, most recently from a150f0a to 6e8c278 Compare November 21, 2024 15:50
@chmouel
Copy link
Member Author

chmouel commented Nov 21, 2024

fixed the e2e-tests and made it to test over over multiple pull request, rephrased documentation to make it clear how the cancellation selection works.

We need to ensure that we only cancel the pipeline run that is running
on the same branch as the current PR or push event. This ensures that we
don't cancel the wrong pipeline run.

On PullRequest events, we only cancel the pipeline run that is running
on the same Pull Request number.

On Push events, we only cancel the pipeline run that is running on the
same SourceBranch (which is the same as HeadBranch on push)

This avoids the issue when you have multiple pull request running on a
repo and we don't cancel the wrong ones that are running for a different
PR.

Signed-off-by: Chmouel Boudjnah <[email protected]>
@vdemeester vdemeester merged commit 2f78eb8 into main Nov 22, 2024
11 checks passed
@vdemeester vdemeester deleted the fix-cancel-in-progress branch November 22, 2024 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants