-
Notifications
You must be signed in to change notification settings - Fork 799
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
Adds feature where if review requirement not met, then request #30653
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. |
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.
First of all, thank you for the contribution!
I like that addition, I can see how useful it can be. However, since it can generate additional noise for maintainers since it will generate notifications and / or emails when they get added as reviewers, I think this option should be opt-in. I would consequently recommend that this only be available when specifying it in the action configuration.
What do you think of that?
## [3.1.0] - 2023-05-11 | ||
### Added | ||
- Added feature where if a review requirement is not met, then request the review. | ||
|
||
### Changed | ||
- Updates misspelling |
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.
Good news, you do not need to fill this in. This changelog will be automatically updated when we next release a new version of this action, based off the different changelog entries in the changelog/
directory (where you've added your own changelog entry already, thank you).
## [3.1.0] - 2023-05-11 | |
### Added | |
- Added feature where if a review requirement is not met, then request the review. | |
### Changed | |
- Updates misspelling |
return res.data.login | ||
} catch ( error ) { | ||
throw new WError( | ||
// prettier-ignore |
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 see you've ignored Prettier here and across the file in general. Could you remove the exception and run it, so the style remains consistent with the other files in the action?
// Handle @singleuser virtual teams. Fetch the correct username case from GitHub | ||
// to avoid having to worry about edge cases and Unicode versions and such. | ||
try { | ||
const login = await getUsername(team); |
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 like that you extracted that here, that's clean. 👍
I wonder if we should make further use of that extraction in the existing logic in projects/github-actions/required-review/src/team-members.js
?
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.
yup, I can pull this out. Is there a file or location that is preferred for this? ie a helper file, or I can call it get-username.js
and reference it in both files.
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.
or I can call it get-username.js and reference it in both files
This seems like a good approach to me 👍
Thanks @jeherve, I'll look into making this feature opt-in |
2b5bbbb
to
a48acc5
Compare
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've pushed a commit to address some linting errors, hopefully that helps a bit.
I think we're still missing a check for the new request-reviews
action parameter you added, before we try to request reviews.
@jeherve Thanks for addressing those linting errors. Regarding the |
I'll let @anomiex weigh in on that, since he worked on this part of the code in #29322 |
Can I get more clarification on this, I'm not sure where I should be looking. thx |
Sure thing. Here, you add a review request when needed: Here, you added a new Action parameter, But you do not check for that parameter later on, so that parameter is useless right now. I would recommend that you check if that parameter is set before to call |
garr - missed it in my commit :D - It's there now |
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.
Regarding the
// prettier-ignore
comment, is there anything you would like me to do with it? I copied that from the team-members file so not sure if this needs an update.I'll let @anomiex weigh in on that, since he worked on this part of the code in #29322
I don't like when prettier decides to wrap a backquoted string over multiple lines, in large part because vim doesn't handle it very well. So I tend to prettier-ignore
when that happens.
if ( reviewSatisfied.length === 0 && request == 'true' ) { | ||
await requestReview( team ); | ||
} | ||
return printSet( `${ indent }Members of ${ team }:`, reviewSatisfied ); |
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.
This is not the right place to add the requesting. If the configuration is requiring a review from "teamA or teamB", and someone from teamB already reviewed, there's no need to request a review from teamA. Also putting it here may result in redundant API calls if teamA is listed in multiple requirement blocks, and will almost certainly result in more API calls than are needed.
IMO the best thing to do would be, somewhere in
jetpack/projects/github-actions/required-review/src/main.js
Lines 84 to 88 in 8602a93
} else { | |
ok = false; | |
core.endGroup(); | |
core.error( `Requirement "${ r.name }" is not satisfied by the existing reviews.` ); | |
} |
add the list of needed teams for
r
to a Set
. You'll need to figure out how to get just the needed teams, of course. For example, if review from both teamX and ( teamY or userBob ) is required, and there's already a review from teamY, you'd ideally only want to request teamX, not teamY again and not userBob either.
Then, just after
jetpack/projects/github-actions/required-review/src/main.js
Lines 93 to 96 in 8602a93
await reporter.status( | |
core.getBooleanInput( 'fail' ) ? reporter.STATE_FAILURE : reporter.STATE_PENDING, | |
reviewers.length ? 'Awaiting more reviews...' : 'Awaiting reviews...' | |
); |
if the new input is true and the set isn't empty, print a message along the lines of
Requesting reviews from ${ members of the set }
then pass the whole set to requestReviewer. Since the request reviewers API endpoint accepts multiple reviewers in one call, only one call should need to be made.
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 for the review @anomiex After going through the code I went this route due to how reviewers seemed to be grabbed first and then it looked like it worked back to the teams. This route seemed the most direct w/o making major changes to the rest of the code. With that said, I agree 💯 with limiting unneeded API calls and will look into reducing these as you're suggesting.
I'm wondering if it might be reasonable to say that review requests only occur on the first run? On that initial run I think it's safe to say that there would be no reviews therefor all required reviewers should be requested. Then as reviews are received, there shouldn't be the further need to request them again, as you point out. There could be a case where a review was removed (new commit push, etc), but thinking of how codeowners files works, it only requests reviews the first time iirc, which is the functionality I'm looking to introduce. Not sure if this would make the above any more straight forward.
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.
If someone pushes a commit to a PR that touches additional files, that might also trigger a need for more reviewers.
For example, if someone from teamX says "oh, you also need to change this other thing over here" in their review, and that other thing happens to require review from teamZ.
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.
yup - great point. 👍🏻
* | ||
* @param {string} user - @ followed by a GitHub user name. | ||
*/ | ||
async function getUsername( user ) { |
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.
Add caching in here please. If called twice for the same user
, it shouldn't hit the API again.
// Handle @singleuser virtual teams. Fetch the correct username case from GitHub | ||
// to avoid having to worry about edge cases and Unicode versions and such. |
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.
Is that a concern here? We do it in the other place because we're comparing the name from the config with the name returned on the review. But here I'd hope submitting "someusername" instead of "someUserName" would just do the right thing.
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.
Tested, and yes someUserName
works just as well as someusername
. So guess there's really no reason to factor out this code as here we can just use const login = team.slice( 1 );
and not touch team-members.js
.
If you agree, I'll just put that code back so I'm touching fewer files.
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.
Sounds good to me!
Please let me know what you think of this approach. The main issue that I'm running into is accounting for 1) a nested Currently I'm thinking the most optimal solution would be to add the I'm open to other suggestions of course, I started running in circles so thought best to reach out. This is my first time working with javascript so I may not be familiar with some other tricks that exist in this language for handing a situation like this. |
@@ -166,7 +178,7 @@ class Requirement { | |||
); | |||
} | |||
|
|||
this.reviewerFilter = buildReviewerFilter( config, { 'any-of': config.teams }, ' ' ); | |||
this.reviewerFilter = buildReviewerFilter( config, 'any-of', { 'any-of': config.teams }, ' ' ); |
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.
This is suggestion, if y'all agree with this solution I would look to update { 'any-of': config.teams }
to just be config.teams and rework the related code.
Please let me know what you think of this approach. The main issue that I'm running into is accounting for 1) a nested Currently I'm thinking the most optimal solution would be to add the I'm open to other suggestions of course, I started running in circles so thought best to reach out. This is my first time working with javascript so I may not be familiar with some other tricks that exist in this language for handing a situation like this. |
I don't like the globals inside The functions returned from
Then I'd replace } else {
const neededForRequirement = await r.needsReviewsFrom( reviewers );
core.endGroup();
if ( neededForRequirement.size === 0 ) {
core.info( `Requirement "${ r.name }" is satisfied by the existing reviews.` );
} else {
core.error( `Requirement "${ r.name }" is not satisfied by the existing reviews.` );
// Merge into `neededTeams` set for possible use after the loop.
neededForRequirement.forEach( neededTeams.add, neededTeams );
}
} Instead of |
a9d73d4
to
f8dc253
Compare
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.
Looks pretty good. I left a few comments inline, and I'll kick off the CI jobs where the linter will have some additional comments.
projects/github-actions/required-review/changelog/request-review-if-not-satisfied
Outdated
Show resolved
Hide resolved
…ew-if-not-satisfied Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
@anomiex Addressed comments and committed suggestions - I didn't get the lint results before committing suggested changes so I lost those runs if you could please retrigger those I'll make any needed updates. |
@jeherve Linting errors should be fixed and ready for a final 🤞🏻 run. |
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
Thanks @anomiex I committed your suggestion, also pushed up final linting error corrections. |
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.
If CI is happy, let's go for it. Thanks!
Fixes #
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
.github/required-review.yaml
similar to the following:Tested this by deploying fork to my local github env and using settings above.