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
Show file tree
Hide file tree
Changes from all commits
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
18 changes: 18 additions & 0 deletions .github/qa-teams.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# QA Teams Configuration
mobile_qa:
reviewers_required: 2
members:
- churik
- yevh-berdnyk
- VolodLytvynenko
- pavloburykh
- mariia-skrypnyk
- Horupa-Olena

desktop_qa:
reviewers_required: 2
members:
- anastasiyaig
- virginiabalducci
- glitchminer
- antdanchenko
155 changes: 155 additions & 0 deletions .github/workflows/breaking-change-review.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
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

name: Require QA review
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v2

- 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 random QA reviewers
if: steps.check_label.outputs.result == 'true'
id: set_reviewers
run: |
mobile_qa_required=$(yq eval '.mobile_qa.reviewers_required' .github/qa-teams.yml)
desktop_qa_required=$(yq eval '.desktop_qa.reviewers_required' .github/qa-teams.yml)

mobile_qa_members=$(yq eval '.mobile_qa.members[]' .github/qa-teams.yml | tr '\n' ' ')
desktop_qa_members=$(yq eval '.desktop_qa.members[]' .github/qa-teams.yml | tr '\n' ' ')

mobile_qa=$(echo $mobile_qa_members | tr ' ' '\n' | shuf -n $mobile_qa_required | tr '\n' ',' | sed 's/,$//')
desktop_qa=$(echo $desktop_qa_members | tr ' ' '\n' | shuf -n $desktop_qa_required | tr '\n' ',' | sed 's/,$//')

echo "mobile_qa=$mobile_qa" >> $GITHUB_OUTPUT
echo "desktop_qa=$desktop_qa" >> $GITHUB_OUTPUT

- 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];

console.log('Requesting reviews from:', reviewers);

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';

message += `- ${mobileQAApproved ? '[ ]' : '[x]'} Mobile QA team review\n`
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