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

Update concurrency group for community label workflow #47355

Merged
merged 4 commits into from May 13, 2024

Conversation

adimoldovan
Copy link
Contributor

@adimoldovan adimoldovan commented May 10, 2024

Submission Review Guidelines:

Changes proposed in this Pull Request:

Closes #47179

Updated the concurrency group for the community-label.yml workflow to use an unique group. The github.ref is always returning refs/heads/trunk incorrectly cancelling some runs.

I also checked for other workflows using issues or pull_request_target triggers but no other was using concurrency.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

After merge, open two issues at the same time. The Add Community Label, Assign Reviewers should run for both, the first run should not be cancelled by the second one.
Repeat the same test with two pull requests.

Copy link
Contributor

github-actions bot commented May 10, 2024

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

@adimoldovan adimoldovan marked this pull request as ready for review May 10, 2024 12:20
@woocommercebot woocommercebot requested a review from a team May 10, 2024 12:21
@adimoldovan adimoldovan enabled auto-merge (squash) May 10, 2024 12:37
Copy link
Contributor

@rrennick rrennick left a comment

Choose a reason for hiding this comment

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

I just had the one question about simplifying the change.

@@ -7,7 +7,9 @@ on:
types: [opened]

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
Copy link
Contributor

Choose a reason for hiding this comment

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

@adimoldovan Could github.ref be replaced with github.head_ref to get the PR branch instead of trunk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would help if we only had the pull_request_target event, but we also have the issues event for which github.head_ref is undefined.
On the other hand the group could be simpler in our case, because we only have the opened action for both events. I went with a group that would cover more actions in case they would be added later.
I guess we could go with the shorter community-label-${{ github.event_name == 'pull_request_target' && github.head_ref || github.run_id }}.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Looks good as is.

@adimoldovan adimoldovan requested a review from rrennick May 13, 2024 09:16
@adimoldovan adimoldovan disabled auto-merge May 13, 2024 09:16
Copy link
Contributor

Hi @rrennick,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

Copy link
Contributor

@rrennick rrennick left a comment

Choose a reason for hiding this comment

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

Looks good.

@adimoldovan adimoldovan merged commit 4ba770c into trunk May 13, 2024
15 checks passed
@adimoldovan adimoldovan deleted the ci/fix-concurrency-group-pull_request_target branch May 13, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Concurrency for community-label.yml causing failed workflow runs
2 participants