feat_: implement PR review workflow #5877
Open
+173
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (134)
|
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
from
bfe0b66
to
8a90758
Compare
|
Looks like you have BREAKING CHANGES in your PR. Check-list
|
jakubgs
approved these changes
Sep 25, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## develop #5877 +/- ##
===========================================
+ Coverage 45.42% 46.09% +0.67%
===========================================
Files 891 891
Lines 158084 158082 -2
===========================================
+ Hits 71808 72869 +1061
+ Misses 78201 76848 -1353
- Partials 8075 8365 +290
Flags with carried forward coverage won't be shown. Click here to find out more. |
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
from
da41420
to
54ff2e6
Compare
|
Hmmm So it seems we will have to make the relevant teams inside status-go repository first. |
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
2 times, most recently
from
49a9364
to
0b606e4
Compare
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
from
0b606e4
to
6b552c1
Compare
|
@siddarthkay is it time to review this? Or are you still testing? |
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
2 times, most recently
from
1fb7f36
to
03dc775
Compare
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
from
03dc775
to
a3ef631
Compare
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
from
a3ef631
to
3af26d0
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
siddarthkay
commented
Oct 1, 2024
| name: 'QA Approval Check', | ||
| head_sha: context.payload.pull_request.head.sha, | ||
| status: 'completed', | ||
| conclusion: 'failure', |
There was a problem hiding this comment.
@igor-sirotin : added a failing check as discussed, the github-actions bot will no longer request changes, It will just add a comment with an appropriate message.
There was a problem hiding this comment.
Does this create a separate status check? Maybe we could just use this check itself?
Something like this should be enough I think 🤔
status-go/.github/workflows/commit-check.yml
Line 100 in a1c6d7f
The reason is not to bloat the checks list. It's already 12 😅
And the comment can be implemented as "sticky comment", I did it in the commit-check. Can be a single comment in the PR that will be edited when reviews received/lost.
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
from
455c661
to
79e8afc
Compare
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
2 times, most recently
from
0d81b86
to
a677c76
Compare
|
added a check to prevent the bot from approving the PR multiple times. |
igor-sirotin
reviewed
Oct 1, 2024
| @@ -0,0 +1,145 @@ | |||
| name: PR Review Workflow for Breaking Changes | |||
There was a problem hiding this comment.
Shorter name would be better I think
Suggested change
| name: PR Review Workflow for Breaking Changes | |
| name: PR Review Workflow |
| - synchronize | ||
|
|
||
| jobs: | ||
| check_breaking_change: |
There was a problem hiding this comment.
A human-readable name would be nice as well:
Suggested change
| check_breaking_change: | |
| check_breaking_change: | |
| name: Require QA review |
This is what we have for conventional commits:
| id: set_reviewers | ||
| run: | | ||
| echo "mobile_qa=churik,yevh-berdnyk,VolodLytvynenko,pavloburykh,mariia-skrypnyk,Horupa-Olena" >> $GITHUB_OUTPUT | ||
| echo "desktop_qa=anastasiyaig,virginiabalducci,glitchminer,antdanchenko" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
This works ofc, if it's simpler 👍 We can improve in future.
Though I'm worried about pinging all QAs, we end up with the list like this:
Imagine what will happen when we add devs reviews as well 🥲
So I think the "randomise" logic is required before merging this. Otherwise we'll die in tons of notifications.
| const desktopQA = '${{ steps.set_reviewers.outputs.desktop_qa }}'.split(','); | ||
| const reviewers = [...mobileQA, ...desktopQA]; | ||
|
|
||
| await github.rest.pulls.requestReviewers({ |
There was a problem hiding this comment.
Might this re-request reviews, if already approved? I think we don't want such behaviour?
| } | ||
| }); | ||
|
|
||
| - name: Allow PR merge if conditions are met |
There was a problem hiding this comment.
Do we need the bot approval at all, if we have the status check already? It's kinda duplication 🤔
| name: 'QA Approval Check', | ||
| head_sha: context.payload.pull_request.head.sha, | ||
| status: 'completed', | ||
| conclusion: 'failure', |
There was a problem hiding this comment.
Does this create a separate status check? Maybe we could just use this check itself?
Something like this should be enough I think 🤔
status-go/.github/workflows/commit-check.yml
Line 100 in a1c6d7f
The reason is not to bloat the checks list. It's already 12 😅
And the comment can be implemented as "sticky comment", I did it in the commit-check. Can be a single comment in the PR that will be edited when reviews received/lost.
Comment on lines
97
to
103
| if (!mobileQAApproved && !desktopQAApproved) { | ||
| message += 'Both mobile-qa and desktop-qa teams have not approved this PR yet.'; | ||
| } else if (!mobileQAApproved) { | ||
| message += 'The mobile-qa team has not approved this PR yet.'; | ||
| } else if (!desktopQAApproved) { | ||
| message += 'The desktop-qa team has not approved this PR yet.'; | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (!mobileQAApproved && !desktopQAApproved) { | |
| message += 'Both mobile-qa and desktop-qa teams have not approved this PR yet.'; | |
| } else if (!mobileQAApproved) { | |
| message += 'The mobile-qa team has not approved this PR yet.'; | |
| } else if (!desktopQAApproved) { | |
| message += 'The desktop-qa team has not approved this PR yet.'; | |
| } | |
| message += `- ${mobileQAApproved ? '[ ]' : '[x]'} Mobile QA team review` | |
| message += `- ${desktopQAApproved ? '[ ]' : '[x]'} Desktop QA team review` |
churik
approved these changes
Oct 1, 2024
This commit Implements a PR review workflow like this : - This action will check if `breaking change` label exists on PR. - if it does then `@status-im/desktop-qa` and `@status-im/mobile-qa` are asked for review on this PR. - Unless 1 person from `@status-im/desktop-qa` and `@status-im/mobile-qa` approve that PR the Github action will block the PR. - Only after these conditions match the Github action will allow merging of this PR.
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
from
a677c76
to
b56e69d
Compare
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
from
23a73a7
to
f2438bb
Compare
siddarthkay
force-pushed
the
pr-review-workflow-automation
branch
from
f2438bb
to
fc73ab2
Compare
siddarthkay
removed request for
anastasiyaig,
antdanchenko,
glitchminer,
Horupa-Olena,
mariia-skrypnyk,
pavloburykh,
virginiabalducci,
VolodLytvynenko,
yevh-berdnyk and
churik
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR Implements a PR review workflow like this :
breaking changelabel exists on PR.@status-im/desktop-qaand@status-im/mobile-qaare asked for review on this PR.@status-im/desktop-qaand@status-im/mobile-qaapprove that PR the Github action will block the PR.