feat: github action to check if PR has requested labels before being merged #19984

gauthierpetetin · 2023-07-12T21:03:23Z

Explanation

The purpose of this new Github action is to check if PRs have the requested labels before they get merged:

A team label, i.e. a label starting with team- string (or external-contributor label)
~~A QA label, i.e. either QA Passed or No QA Needed/E2E Only or Spot Check on the Release Build~~

Furthermore, it forbids merging PRs with the following labels: needs-qa, QA'd but questions, issues-found, need-ux-ds-review, blocked, stale, DO-NOT-MERGE. A consequence is that we can get rid of this github action.

Progresses #12345

Screenshots/Screencaps

Recording for missing team label: https://recordit.co/U38NvBpIIj
Recording for missing QA label: https://recordit.co/jowmkvMkCh

Manual Testing Steps

Add a team label and a QA label on this PR, the job shall turn to green
Remove either team label or QA label from this PR, the job shall turn to red

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

Other

Same PR for Mobile

github-actions · 2023-07-12T21:03:35Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

gauthierpetetin · 2023-07-19T20:58:16Z

@PeterYinusa @danjm as discussed last Friday, I removed the QA label check from this PR, in this commit.

DDDDDanica · 2023-07-20T13:11:59Z

.github/scripts/check-pr-has-required-labels.ts

+  // Retrieve pull request labels
+  const prLabels = await retrievePullRequestLabels(octokit, prRepoOwner, prRepoName, prNumber);
+
+  const preventMergeLabels = ["needs-qa", "QA'd but questions", "issues-found"];


we can also add labels such as need-ux-ds-review (which i see a lot of this one), blocked (provided but maybe not used as many , stale, DO-NOT-MERGE, for now, wdyt?

There is already a CI workflow to prevent merging DO-NOT-MERGE labelled PRs. Though we may want to consolidate it here, that's a good point.

Though... we could move in the other direction as well. The DO-NOT-MERGE workflow is a lot simpler. We can access the necessary information (PR labels) purely using GitHub Actions context, no queries needed.

Making blocked block merging might make sense, if only to force someone to remove it when it's no longer applicable. In theory if a PR is blocked, it must not be mergable, so if it's remaining it's inaccurate. Perhaps we could automatically remove it instead though, not sure.

Not sure about stale. If a PR was marked as stale, but then is ready to merge, then... that sounds good? Maybe I don't understand the workflow though, e.g. maybe we want people to explicitly remove that label? Here it seems reasonable to consider automatic removal as well though.

Thanks for your feedbacks @DDDDDanica and @Gudahtt . We've discussed this with @danjm @sethkfman @cortisiko @PeterYinusa @vpintorico last Friday and agreed on:

adding checks for these labels in this new Github action: need-ux-ds-review, blocked, stale, DO-NOT-MERGE

removing the old DO-NOT-MERGE Github action as duplicate of this new Github action

Here's the corresponding commit.

.github/workflows/check-pr-labels.yml

metamaskbot · 2023-07-27T19:22:51Z

Builds ready [aa6c269]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1601 ± 26 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	119	223	151	28	14
		domContentLoaded	1535	1749	1600	54	26
		load	1536	1749	1601	54	26
		domInteractive	1535	1749	1600	54	26

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

gauthierpetetin added team-extension-client team-extension-platform labels Jul 12, 2023

gauthierpetetin requested a review from a team as a code owner July 12, 2023 21:03

gauthierpetetin self-assigned this Jul 12, 2023

pedronfigueiredo previously approved these changes Jul 13, 2023

View reviewed changes

DDDDDanica previously approved these changes Jul 13, 2023

View reviewed changes

gauthierpetetin dismissed stale reviews from DDDDDanica and pedronfigueiredo via 0eab8d1 July 13, 2023 11:16

DDDDDanica reviewed Jul 20, 2023

View reviewed changes

.github/workflows/check-pr-labels.yml Show resolved Hide resolved

gauthierpetetin mentioned this pull request Jul 25, 2023

feat: github action to check if PR has requested labels before being merged MetaMask/metamask-mobile#6794

Merged

3 tasks

pedronfigueiredo approved these changes Jul 27, 2023

View reviewed changes

DDDDDanica approved these changes Jul 27, 2023

View reviewed changes

gauthierpetetin added 6 commits July 27, 2023 15:46

feat(action): check if pr includes requested labels

3a5a060

fix(action): add missing QA label

8aa689c

fix(action): check if no label prevents merging

4e05165

fix(action): remove QA label check

b0a5a6a

fix(action): add missing reopened condition

897a32e

fix(action): increase list of labels which prevent merges

aa6c269

gauthierpetetin force-pushed the feat/check_pr_labels branch from dc36dc8 to aa6c269 Compare July 27, 2023 18:47

gauthierpetetin merged commit 0c25193 into develop Jul 28, 2023

gauthierpetetin deleted the feat/check_pr_labels branch July 28, 2023 18:15

github-actions bot locked and limited conversation to collaborators Jul 28, 2023

metamaskbot added the release-10.36.0 Issue or pull request that will be included in release 10.36.0 label Jul 28, 2023

Gudahtt added release-11.1.0 Issue or pull request that will be included in release 11.1.0 and removed release-10.36.0 Issue or pull request that will be included in release 10.36.0 labels Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: github action to check if PR has requested labels before being merged #19984

feat: github action to check if PR has requested labels before being merged #19984

gauthierpetetin commented Jul 12, 2023 •

edited

Loading

github-actions bot commented Jul 12, 2023

gauthierpetetin commented Jul 19, 2023

DDDDDanica Jul 20, 2023 •

edited

Loading

Gudahtt Jul 20, 2023

Gudahtt Jul 20, 2023

gauthierpetetin Jul 25, 2023 •

edited

Loading

metamaskbot commented Jul 27, 2023

feat: github action to check if PR has requested labels before being merged #19984

feat: github action to check if PR has requested labels before being merged #19984

Conversation

gauthierpetetin commented Jul 12, 2023 • edited Loading

Explanation

Screenshots/Screencaps

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

Other

github-actions bot commented Jul 12, 2023

gauthierpetetin commented Jul 19, 2023

DDDDDanica Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Gudahtt Jul 20, 2023

Choose a reason for hiding this comment

Gudahtt Jul 20, 2023

Choose a reason for hiding this comment

gauthierpetetin Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

metamaskbot commented Jul 27, 2023

gauthierpetetin commented Jul 12, 2023 •

edited

Loading

DDDDDanica Jul 20, 2023 •

edited

Loading

gauthierpetetin Jul 25, 2023 •

edited

Loading