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.

Sign up for GitHub

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

Jump to bottom

Update required-review config #29318

Closed
wants to merge 2 commits into from
Closed

Update required-review config #29318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6936eb8
required-review: Add `consume` option
anomiex Mar 6, 2023
aafeb65
Update required-review config
anomiex Mar 6, 2023
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
86 changes: 30 additions & 56 deletions .github/files/required-review.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,12 @@
# Jetpack Approvers need to review changes to the monorepo itself.
- name: Monorepo itself
paths:
- '!projects/**'
- '!pnpm-lock.yaml'
teams:
- jetpack-approvers

# Everyone who can approve anything can merge pnpm.lock and docs.
- name: pnpm lockfile, composer files
- name: lockfiles, plugin composer.json, changelogs, docs
paths:
- 'docs/**'
- 'pnpm-lock.yaml'
- 'projects/plugins/*/composer.lock'
- 'projects/*/*/composer.lock'
- 'projects/plugins/*/composer.json'
- 'projects/plugins/*/changelog/*'
- 'projects/*/*/changelog/*'
consume: true
teams:
# Unfortunately this list need to be maintaned manually...
# yamato-backup-and-security is the group that consists of both yamato-scan and yamato-backup teams.
Expand All @@ -25,38 +18,43 @@
- red
- yamato-backup-and-security

# Jetpack Approvers review the Jetpack plugin and all packages, except for those with specific team ownership.
- name: Jetpack and packages
# Jetpack Approvers need to review changes to the monorepo itself.
- name: Monorepo itself
paths:
- '!projects/**'
teams:
- jetpack-approvers

# Jetpack Approvers review the Jetpack plugin.
- name: Jetpack-the-plugin
paths:
- 'projects/packages/**'
- 'projects/plugins/jetpack/**'
# Exclude packages managed by other teams, which are covered by other blocks below.
- '!projects/js-packages/image-guide/**'
- '!projects/js-packages/svelte-data-sync-client/**'
- '!projects/packages/wp-js-data-sync/**'
- '!projects/packages/backup/**'
- '!projects/packages/import/**'
- '!projects/packages/forms/**'
- '!projects/packages/search/**'
- '!projects/packages/stats/**'
- '!projects/packages/stats-admin/**'
- '!projects/packages/publicize/**'
- '!projects/packages/waf/**'
- '!projects/plugins/*/composer.lock'
- '!projects/plugins/*/composer.json'
- '!projects/plugins/*/changelog/*'
teams:
- jetpack-approvers

# Packages owned by Ground Control.
- name: Ground Control packages
paths:
- 'projects/js-packages/babel-plugin-replace-textdomain/**'
- 'projects/js-packages/eslint-changed/**'
- 'projects/js-packages/eslint-config-target-es/**'
- 'projects/js-packages/i18n-check-webpack-plugin/**'
- 'projects/js-packages/i18n-loader-webpack-plugin/**'
- 'projects/js-packages/remove-asset-webpack-plugin/**'
- 'projects/js-packages/webpack-config/**'
- 'projects/packages/autoloader/**'
- 'projects/packages/changelogger/**'
- 'projects/packages/codesniffer/**'
- 'projects/packages/ignorefile/**'
- 'projects/packages/phpcs-filter/**'
teams:
- jetpack-approvers
Comment on lines +28 to +51
Copy link
Member

Choose a reason for hiding this comment

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

I do not fully understand that part. Given that the Approvers group already is the fallback, why do we need to add those specific use-cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need to, at least not now. OTOH, this clearly documents that these packages are specifically intended to be owned, and keeps them that way should we decide to change the default for some reason.


# The Avengers team reviews changes to the CRM plugin,
# and can add dependencies to the monorepo's lock file.
- name: CRM
paths:
- 'projects/plugins/crm/**'
- '!projects/plugins/*/composer.lock'
- '!projects/plugins/*/composer.json'
- '!projects/plugins/*/changelog/*'
teams:
- avengers
- jetpack-approvers
Expand All @@ -67,9 +65,6 @@
paths:
- 'projects/packages/import/**'
- 'projects/plugins/migration/**'
- '!projects/plugins/*/composer.lock'
- '!projects/plugins/*/composer.json'
- '!projects/plugins/*/changelog/*'
teams:
- caribou
- jetpack-approvers
Expand All @@ -81,9 +76,6 @@
- 'projects/packages/backup/**'
- 'projects/plugins/vaultpress/**'
- 'projects/plugins/backup/**'
- '!projects/plugins/*/composer.lock'
- '!projects/plugins/*/composer.json'
- '!projects/plugins/*/changelog/*'
teams:
- yamato-backup
- jetpack-approvers
Expand All @@ -94,9 +86,6 @@
paths:
- 'projects/packages/waf/**'
- 'projects/plugins/protect/**'
- '!projects/plugins/*/composer.lock'
- '!projects/plugins/*/composer.json'
- '!projects/plugins/*/changelog/*'
teams:
- yamato-scan
- jetpack-approvers
Expand All @@ -110,9 +99,6 @@
- 'projects/js-packages/image-guide/**'
- 'projects/js-packages/svelte-data-sync-client/**'
- 'projects/packages/wp-js-data-sync/**'
- '!projects/plugins/*/composer.lock'
- '!projects/plugins/*/composer.json'
- '!projects/plugins/*/changelog/*'
teams:
- heart-of-gold
- jetpack-approvers
Expand All @@ -123,9 +109,6 @@
paths:
- 'projects/plugins/search/**'
- 'projects/packages/search/**'
- '!projects/plugins/*/composer.lock'
- '!projects/plugins/*/composer.json'
- '!projects/plugins/*/changelog/*'
teams:
- red
- jetpack-approvers
Expand All @@ -135,9 +118,6 @@
paths:
- 'projects/packages/stats/**'
- 'projects/packages/stats-admin/**'
- '!projects/plugins/*/composer.lock'
- '!projects/plugins/*/composer.json'
- '!projects/plugins/*/changelog/*'
teams:
- red
- jetpack-approvers
Expand All @@ -148,9 +128,6 @@
- 'projects/plugins/social/**'
- 'projects/packages/publicize/**'
- 'projects/js-packages/publicize-components/**'
- '!projects/plugins/*/composer.lock'
- '!projects/plugins/*/composer.json'
- '!projects/plugins/*/changelog/*'
teams:
- jetpack-reach
- jetpack-approvers
Expand All @@ -159,9 +136,6 @@
- name: Forms
paths:
- 'projects/packages/forms/**'
- '!projects/plugins/*/composer.lock'
- '!projects/plugins/*/composer.json'
- '!projects/plugins/*/changelog/*'
teams:
- '@cgastrell'
- '@ice9js'
Expand Down
23 changes: 23 additions & 0 deletions projects/github-actions/required-review/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ The requirements consist of an array of requirement objects. A requirement objec
* `paths` is an array of path patterns, or the string "unmatched". If an array, the reviewers
specified will be checked if any path in the array matches any path in the PR. If the string
"unmatched", the reviewers are checked if any file in the PR has not been matched yet.
* `consume` is a boolean, defaulting to false. If set, any paths that match this rule will be ignored
for all following rules.

This is intended for things like lockfiles or changelogs that you might want to allow everyone
to edit in any package in a monorepo, to avoid having to manually exclude these files in every
requirement for each different team owning some package.
* `teams` is an array of strings that are GitHub team slugs in the organization or repository. A
review is required from a member of any of these teams.

Expand Down Expand Up @@ -108,6 +114,23 @@ the "Docs" and "Front end" review requirements. If you wanted to avoid that, you
teams:
- documentation

# Everyone can update lockfiles, even if later rules might otherwise match.
- name: Lockfiles
paths:
- 'packages/*/composer.lock'
- '**.css'
consume: true
teams:
- everyone

# The "Some package" team must approve anything in `packages/some-package/`.
# Except for changes to `packages/some-package/composer.lock`, because the previous requirement consumed that path.
- name: Some package
paths:
- 'packages/some-package/**'
teams:
- some-package-team

# Any CSS and React .jsx files must be reviewed by a front-end developer AND by a designer,
# OR by a member of the maintenance team.
- name: Front end
Expand Down
4 changes: 4 additions & 0 deletions projects/github-actions/required-review/changelog/add-required-review-consume-option
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: added

Added `consume` option for requirements.
8 changes: 5 additions & 3 deletions projects/github-actions/required-review/src/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,17 +63,19 @@ async function main() {
reviewers.forEach( r => core.info( r ) );
core.endGroup();

const paths = await require( './paths.js' )();
let paths = await require( './paths.js' )();
core.startGroup( `PR affects ${ paths.length } file(s)` );
paths.forEach( p => core.info( p ) );
core.endGroup();

const matchedPaths = [];
let matchedPaths = [];
let ok = true;
for ( let i = 0; i < requirements.length; i++ ) {
const r = requirements[ i ];
core.startGroup( `Checking requirement "${ r.name }"...` );
if ( ! r.appliesToPaths( paths, matchedPaths ) ) {
let applies;
( { applies, matchedPaths, paths } = r.appliesToPaths( paths, matchedPaths ) );
if ( ! applies ) {
core.endGroup();
core.info( `Requirement "${ r.name }" does not apply to any files in this PR.` );
} else if ( await r.isSatisfied( reviewers ) ) {
Expand Down
28 changes: 22 additions & 6 deletions projects/github-actions/required-review/src/requirement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ class Requirement {
* @param {object} config - Object config
* @param {string[]|string} config.paths - Paths this requirement applies to. Either an array of picomatch globs, or the string "unmatched".
* @param {Array} config.teams - Team reviews requirements.
* @param {boolean} config.consume - Whether matched paths should be ignored by later rules.
*/
constructor( config ) {
this.name = config.name || 'Unnamed requirement';
Expand Down Expand Up @@ -163,14 +164,19 @@ class Requirement {
}

this.reviewerFilter = buildReviewerFilter( config, { 'any-of': config.teams }, ' ' );
this.consume = !! config.consume;
}

// eslint-disable-next-line jsdoc/require-returns, jsdoc/require-returns-check -- Doesn't support documentation of object structure.
/**
* Test whether this requirement applies to the passed paths.
*
* @param {string[]} paths - Paths to test against.
* @param {string[]} matchedPaths - Paths that have already been matched. Will be modified if true is returned.
* @returns {boolean} Whether the requirement applies.
* @param {string[]} matchedPaths - Paths that have already been matched.
* @returns {object} _ Results object.
* @returns {boolean} _.applies Whether the requirement applies.
* @returns {string[]} _.matchedPaths New value for `matchedPaths`.
* @returns {string[]} _.paths New value for `paths`.
*/
appliesToPaths( paths, matchedPaths ) {
let matches;
Expand All @@ -183,14 +189,24 @@ class Requirement {
}
}

if ( matches.length !== 0 ) {
const ret = {
applies: matches.length !== 0,
matchedPaths,
paths,
};

if ( ret.applies ) {
core.info( 'Matches the following files:' );
matches.forEach( m => core.info( ` - ${ m }` ) );
matchedPaths.push( ...matches.filter( p => ! matchedPaths.includes( p ) ) );
matchedPaths.sort();
ret.matchedPaths = [ ...new Set( [ ...matchedPaths, ...matches ] ) ].sort();

if ( this.consume ) {
core.info( 'Consuming matched files!' );
ret.paths = ret.paths.filter( p => ! matches.includes( p ) );
}
}

return matches.length !== 0;
return ret;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion projects/github-actions/required-review/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function addenv {
eval "$(
# Read defaults from action.yml
node -e 'console.log( JSON.stringify( require( "js-yaml" ).load( require( "fs" ).readFileSync( process.argv[1] ) ) ) );' "$BASE/projects/github-actions/required-review/action.yml" |
jq -r '.inputs | to_entries | .[] | select( .key != "token" and .value.default ) | [ "addenv INPUT_", ( .key | ascii_upcase | sub( " "; "_" ) ), " ", @sh "\(.value.default)" ] | join( "" )';
jq -r '.inputs | to_entries | .[] | select( .key != "token" and ( .value | has( "default" ) ) ) | [ "addenv INPUT_", ( .key | ascii_upcase | sub( " "; "_" ) ), " ", @sh "\(.value.default)" ] | join( "" )';

# Read values from .github/workflows/required-review.yml
node -e 'console.log( JSON.stringify( require( "js-yaml" ).load( require( "fs" ).readFileSync( process.argv[1] ) ) ) );' "$BASE/.github/workflows/required-review.yml" |
Expand Down