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

feat_: implement PR review workflow #5877

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 145 additions & 0 deletions .github/workflows/breaking-change-review.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
name: PR Review Workflow for Breaking Changes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shorter name would be better I think

Suggested change
name: PR Review Workflow for Breaking Changes
name: PR Review Workflow


on:
pull_request:
types:
- opened
- reopened
- labeled
- unlabeled
- synchronize

jobs:
check_breaking_change:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

image

runs-on: ubuntu-latest
steps:
- name: Check for breaking change label
id: check_label
uses: actions/github-script@v6
with:
github-token: ${{secrets.GITHUB_TOKEN}}
script: |
const { data: labels } = await github.rest.issues.listLabelsOnIssue({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
});
const hasBreakingChange = labels.some(label => label.name === 'breaking change');
console.log(`Has breaking change label: ${hasBreakingChange}`);
return hasBreakingChange;

- name: Set QA reviewers
if: steps.check_label.outputs.result == 'true'
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the time being I'm hardcoding desktop and mobile QA members here.
To fetch QA Teams from status-im github ORG I'll need another token that has ORG level permissions and that can also be subject to rate limiting.
so for simplicity sake this was easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@igor-sirotin : we can also write some logic to randomise who gets requested for review since we now decide who to request a review ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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:
image

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.

Copy link
Member

Choose a reason for hiding this comment

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

Then can't we just put that in a separate file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

separate file indeed looks a bit easier to manage.


- name: Request QA reviews
if: steps.check_label.outputs.result == 'true'
uses: actions/github-script@v6
with:
github-token: ${{secrets.GITHUB_TOKEN}}
script: |
const mobileQA = '${{ steps.set_reviewers.outputs.mobile_qa }}'.split(',');
const desktopQA = '${{ steps.set_reviewers.outputs.desktop_qa }}'.split(',');
const reviewers = [...mobileQA, ...desktopQA];

await github.rest.pulls.requestReviewers({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might this re-request reviews, if already approved? I think we don't want such behaviour?

owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number,
reviewers: reviewers
});

- name: Check QA approvals
if: steps.check_label.outputs.result == 'true'
id: check_approvals
uses: actions/github-script@v6
with:
github-token: ${{secrets.GITHUB_TOKEN}}
script: |
const { data: reviews } = await github.rest.pulls.listReviews({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number,
});

const mobileQA = '${{ steps.set_reviewers.outputs.mobile_qa }}'.split(',');
const desktopQA = '${{ steps.set_reviewers.outputs.desktop_qa }}'.split(',');

const mobileQAApproved = reviews.some(review =>
review.state === 'APPROVED' && mobileQA.includes(review.user.login)
);
const desktopQAApproved = reviews.some(review =>
review.state === 'APPROVED' && desktopQA.includes(review.user.login)
);

console.log(`Mobile QA approved: ${mobileQAApproved}`);
console.log(`Desktop QA approved: ${desktopQAApproved}`);

core.setOutput('mobile-qa-approved', mobileQAApproved);
core.setOutput('desktop-qa-approved', desktopQAApproved);

return mobileQAApproved && desktopQAApproved;

- name: Block PR if conditions not met
if: steps.check_label.outputs.result == 'true' && steps.check_approvals.outputs.result != 'true'
uses: actions/github-script@v6
with:
github-token: ${{secrets.GITHUB_TOKEN}}
script: |
const mobileQAApproved = ${{ steps.check_approvals.outputs.mobile-qa-approved }};
const desktopQAApproved = ${{ steps.check_approvals.outputs.desktop-qa-approved }};

let message = 'This PR has the breaking change label and requires approval from both mobile-qa and desktop-qa teams before it can be merged.\n\n';

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.';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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`


await github.rest.checks.create({
owner: context.repo.owner,
repo: context.repo.repo,
name: 'QA Approval Check',
head_sha: context.payload.pull_request.head.sha,
status: 'completed',
conclusion: 'failure',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this create a separate status check? Maybe we could just use this check itself?
Something like this should be enough I think 🤔

core.setFailed("Some commit messages are ill-formed")

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.

output: {
title: 'QA Approval Required',
summary: message
}
});

- name: Allow PR merge if conditions are met
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the bot approval at all, if we have the status check already? It's kinda duplication 🤔

if: steps.check_label.outputs.result == 'true' && steps.check_approvals.outputs.result == 'true'
uses: actions/github-script@v6
with:
github-token: ${{secrets.GITHUB_TOKEN}}
script: |

const { data: reviews } = await github.rest.pulls.listReviews({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number,
});

const botApprovalExists = reviews.some(review =>
review.user.login === 'github-actions[bot]' && review.state === 'APPROVED'
);

if (!botApprovalExists) {
await github.rest.pulls.createReview({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number,
event: 'APPROVE',
body: 'Breaking changes have been approved by Mobile and Desktop QA. This PR can now be merged.'
});
} else {
console.log('Bot approval already exists. No action taken.');
}
Loading