Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-134309: Fix premature cancellation of CI tests when multiple PRs have the same branch name#134310
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Addresses issue encountered during PyCon US sprints where having multiple workflows with the same concurrency group causes one of the two to be canceled.Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
.github/workflows/build.yml Outdated
| group: >- | ||
| ${{ | ||
| github.workflow | ||
| }}-${{ | ||
| github.ref_name | ||
| }}-${{ | ||
| github.event.pull_request.number | ||
| || github.sha | ||
| }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove whitespace
| group:>- | |
| ${{ | |
| github.workflow | |
| }}-${{ | |
| github.ref_name | |
| }}-${{ | |
| github.event.pull_request.number | |
| || github.sha | |
| }} | |
| group:${{ github.workflow }}-${{ github.ref_name }}-${{ github.event.pull_request.number || github.sha }}-reusable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It was me who asked for wrapped lines FTR.
Also, we decided to drop the reusable suffix. We talked about it with Cam a lot yesterday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think wrapped lines meaningfully help here, and because of the interpolation markers it just adds more visual clutter.
Fine with keeping or removing-reusable, it wasn't mentioned in the PR so I thought it was unintentional.
I'm surprised that this happens, I had thought that A |
Is it worth doing? I always thought that this behavior was to avoid consuming resources, since the previous push will have obsolete results. |
The problem is when new/inexperienced users create a PR from the default branch, all of the concurrency keys collide as there is no namespacing. This becomes an issue at events like the US PyCon. I've considered suggesting a workflow to auto-close PRs from the default branch, as they come with various problems, but I've held off as it does seem a little heavy handed. A simpler fix would just be to prepend the account name of the fork, but I don't know if there's an easy GH context variable for that (currently on mobile). A |
|
.github/workflows/build.yml Outdated
| @@ -15,7 +15,15 @@ permissions: | |||
| contents: read | |||
| concurrency: | |||
| group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}-reusable | |||
| group: >- | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you please add a comment to explain why we have to put these information into the group? I understand that it's a way to create a global unique identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just to clarify, the info from lines 19-26 or just what's on line 18?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The entire thing I imagine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Document how the group string is created.
Proposed different fix after doing some further testing. This ensuresthat concurrency workflows will have a unique name in any situation thattriggers them, and better navigates some quirks in github actions.The improved comments should make it more visible how the namingworks.Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
I'd like some opinions here (/nudging@webknjaz and@CAM-Gerlach if they would like to weigh in), I did some extra testing on a boilerplate repo and it seems like github may have changed how some of these work. |
@hugovk FYI that actor isn't always the PR author, it might be somebody from core restarting the workflow, or closing+reopening the issue, or adding a commit on top. I'll try to spend some time thinking this though in the morning... Maybe check w/ Kira IRL. |
Uh oh!
There was an error while loading.Please reload this page.
Ah right,
|
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM and addresses all of my original concerns.
979d81a intopython:mainUh oh!
There was an error while loading.Please reload this page.
Thanks@abstractedfox for the PR, and@AA-Turner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…cy key (pythonGH-134310)When inexperienced users create a PR from their default branch, all of the concurrency keyscollide as there is no namespacing. This becomes an issue at events with many new contributors,where workflow runs are cancelled on other pull requests.Disambiguate by adding the username of the relevant 'actor' to the concurrency key.(cherry picked from commit979d81a)Co-authored-by: Kira <coldcaption@gmail.com>Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>Authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
…cy key (pythonGH-134310)When inexperienced users create a PR from their default branch, all of the concurrency keyscollide as there is no namespacing. This becomes an issue at events with many new contributors,where workflow runs are cancelled on other pull requests.Disambiguate by adding the username of the relevant 'actor' to the concurrency key.(cherry picked from commit979d81a)Co-authored-by: Kira <coldcaption@gmail.com>Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>Authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
GH-134484 is a backport of this pull request to the3.14 branch. |
GH-134485 is a backport of this pull request to the3.13 branch. |
…ncy key (GH-134310) (#134485)gh-134309: Add ``github.actor`` to the GitHub Actions concurrency key (GH-134310)When inexperienced users create a PR from their default branch, all of the concurrency keyscollide as there is no namespacing. This becomes an issue at events with many new contributors,where workflow runs are cancelled on other pull requests.Disambiguate by adding the username of the relevant 'actor' to the concurrency key.(cherry picked from commit979d81a)Authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>Co-authored-by: Kira <coldcaption@gmail.com>Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
…ncy key (GH-134310) (#134484)gh-134309: Add ``github.actor`` to the GitHub Actions concurrency key (GH-134310)When inexperienced users create a PR from their default branch, all of the concurrency keyscollide as there is no namespacing. This becomes an issue at events with many new contributors,where workflow runs are cancelled on other pull requests.Disambiguate by adding the username of the relevant 'actor' to the concurrency key.(cherry picked from commit979d81a)Authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>Co-authored-by: Kira <coldcaption@gmail.com>Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Uh oh!
There was an error while loading.Please reload this page.
PerGitHub, "When you define a concurrency key, GitHub Actions ensures that only one workflow or job with that key runs at any given time." Under the current config, concurrency group names for pull requests are made unique (against those of other pull requests) only by the name of the branch, so if pull requests were made for identically named branches around the same time, CI would fail. (This affected commit#134234 )
This PR addresses that by using more unique attributes to name each concurrency group. This should also help with debugging any future issues that concern the name of a concurrency group.