-
Notifications
You must be signed in to change notification settings - Fork 132
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
Utilize GitHub Actions to check for SEMVER impact label #2470
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2470 +/- ##
=======================================
Coverage 50.98% 50.98%
=======================================
Files 124 124
Lines 5305 5305
Branches 1137 1137
=======================================
Hits 2705 2705
Misses 2311 2311
Partials 289 289 ☔ View full report in Codecov by Sentry. |
It still seems to be on "on PR closed" - any reason why you aren't using "on PR approved" as suggested in the issue by @tlylt ? |
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.
Great - thanks for working on this! (BTW I think your links are linking to the successful job rather than the PRS this is generated, which rlly confused me at first haha)
I think this is ready to merge - @yucheng11122017 want to take another look?
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! Thanks for the work :) Would be very useful
@KevinEyo1 could you check why the check is failing on this PR? |
Hi @yucheng11122017, I will take a look into it |
This PR |
Hi @yucheng11122017, can you try it now? I gave the action permission to write to PRs which allows it to add labels. |
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
I think it still doesn't work @KevinEyo1 |
Hi @yucheng11122017 sorry about that, can you try again? I set |
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.
For testing
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.
test
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.
(Actual approval) Thank you for all the changes @KevinEyo1 and the investigation! LGTM :)
What is the purpose of this pull request?
Overview of changes:
Fixes #2464
Added a new workflow that runs jobs on PR closing to
master
branch (only if its merging in, so won't be run if PR is just closed without merging)Anything you'd like to highlight/discuss:
Figuring how to best test this, due to limitations in regard to local testing of closing PRs
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
GitHub Actions: check impact label
It is easy to forget to label PRs with their SEMVER impact when
merging.
Adding a workflow to automate labelling will help prevent
users from missing labels when merging.
Let's check the PR body description for user selected impact,
and automatically add the label to the PR when merging.
This approach allows the user to not even have to add the label
themselves.
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major
,r.Minor
,r.Patch
.Breaking change release note preparation (if applicable):