Have GitHub Actions check the new reviewer checklist#12429
Have GitHub Actions check the new reviewer checklist#12429AndrewGable merged 30 commits intomainfrom
Conversation
| }); | ||
| } | ||
|
|
||
| getNumberOfItemsFromAuthorChecklist() |
There was a problem hiding this comment.
NAB you could use Promise.all to download the template and the PR body in parallel and then compare them when Promise.all returns.
| /** | ||
| * @returns {Promise} | ||
| */ | ||
| function getPullRequestBody() { |
There was a problem hiding this comment.
NAB could put this function in GitHubUtils directly
| /** | ||
| * @returns {Promise} | ||
| */ | ||
| function getNumberOfItemsFromReviewerChecklist() { |
There was a problem hiding this comment.
Could DRY this up by making it take the filepath and then putting it in .github/libs
There was a problem hiding this comment.
Since there is custom logic for the author checklist to parse out the checklist, I don't think it makes DRYing this up very valuable.
| /** | ||
| * @returns {Promise} | ||
| */ | ||
| function getAllReviewComments() { |
There was a problem hiding this comment.
NAB could put these in GitHubUtils as well
| function checkIssueForCompletedChecklist(numberOfChecklistItems) { | ||
| getAllReviewComments() | ||
| .then(reviewComments => combinedComments.push(...reviewComments)) | ||
| .then(() => getAllComments()) |
There was a problem hiding this comment.
Can use Promise.all here again to get all the comments in parallel:
Promise.all([
getAllComments(),
getAllReviewComments(),
])
.then((...combinedComments) => _.flatten(combinedComments))
.then((combinedComments) => {
// Do stuff
})
dangrous
dismissed stale reviews from roryabraham and sobitneupane
via
a8b6b46
|
Okay it looks like that worked but MAN that diff is weird - it looks exactly the same... |
|
Awesome, thanks for doing that! Bumping @roryabraham for a final approval and merge |
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
Overall looks good 👍🏼 approving now but will wait until I'm not on mobile to merge - can't monitor the actions console on mobile. If anyone else beats me to it feel free to merge
Just out of curiosity, why is PHP looking specifically for the checklist job?
| @@ -0,0 +1,9 @@ | |||
| name: 'Author Checklist Test' | |||
| description: 'Verifies that the author checklist is filled out' | |||
There was a problem hiding this comment.
NAB at all, but we're not very consistent in our Yaml style (i.e: single quotes, double quotes, or none)
| - name: reviewerChecklist.js | ||
| uses: Expensify/App/.github/actions/javascript/reviewerChecklist@main | ||
| with: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
NAB but you can also use github.token instead of SECRETS.GITHUB_TOKEN. I don't know if it makes any difference but we should probably strive for consistency
|
PR Author Checklist is not shown in checks. Is it expected? Screen.Recording.2022-11-10.at.20.46.11.mov |
AndrewGable
changed the title
|
This is off hold, looking into the test failures. |
grgia
left a comment
grgia
left a comment
There was a problem hiding this comment.
Should this be labeled with ProductionQA?
|
Good question - I see the tests are failing because they reference |
|
Ok we can internal QA it once it's merged, ready for review and merge I believe! |
AndrewGable
left a comment
AndrewGable
left a comment
There was a problem hiding this comment.
Looks good, lets test this!
|
Triggered auto assignment to @arosiclair ( |
This comment was marked as resolved.
This comment was marked as resolved.
|
@arosiclair - Please ping here with QA results, thanks! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @AndrewGable in version: 1.2.28-0 🚀
|
|
QA'ed with #12756 - Author logic worked fine, but reviewer test passed even when there were non-checked off boxes in yuwen's comment. Thoughts? Based on the GH Actions console, it doesn't look like the code ever actually ran - there are no console.log statements. Maybe it errored out at some point prior to that? |
|
🚀 Deployed to staging by @AndrewGable in version: 1.2.28-0 🚀
|
|
🚀 Deployed to production by @roryabraham in version: 1.2.28-2 🚀
|
8 participants
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/239866
$ https://github.com/Expensify/Expensify/issues/231992
$ https://github.com/Expensify/Expensify/issues/233287
Tests
Can only really be tested in production after this is merged. I tested it by temporarily pointing the actions to checkout code from my branch. You can see a successful run of the author checklist here and the review checklist here.
QA Steps
(feel free to ping me or any internal engineer to perform this QA if you don't feel comfortable doing it yourself)
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)Screenshots
No screenshots necessary since this code doesn't touch the application