From 6936eb867623d3867f55e3bc82aac808102e75b6 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Mon, 6 Mar 2023 13:17:46 -0500 Subject: [PATCH 1/2] required-review: Add `consume` option In the monorepo we have a bunch of projects owned by different teams. Everyone should be able to update lock files and changelog entries, which means that the rules for each team's projects has to exclude those files so only the "anyone can update these files" rule applies. This adds a new rule option, `consume`. When set on a rule, any paths matching the rule will be ignored for all later rules in the file. This will allow us to not have to repeat these exclusions everywhere. --- .../github-actions/required-review/README.md | 23 +++++++++++++++ .../add-required-review-consume-option | 4 +++ .../required-review/src/main.js | 8 ++++-- .../required-review/src/requirement.js | 28 +++++++++++++++---- .../github-actions/required-review/test.sh | 2 +- 5 files changed, 55 insertions(+), 10 deletions(-) create mode 100644 projects/github-actions/required-review/changelog/add-required-review-consume-option diff --git a/projects/github-actions/required-review/README.md b/projects/github-actions/required-review/README.md index a14164870426b..8d43ac727ed5d 100644 --- a/projects/github-actions/required-review/README.md +++ b/projects/github-actions/required-review/README.md @@ -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. @@ -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 diff --git a/projects/github-actions/required-review/changelog/add-required-review-consume-option b/projects/github-actions/required-review/changelog/add-required-review-consume-option new file mode 100644 index 0000000000000..a715828e4ba5c --- /dev/null +++ b/projects/github-actions/required-review/changelog/add-required-review-consume-option @@ -0,0 +1,4 @@ +Significance: minor +Type: added + +Added `consume` option for requirements. diff --git a/projects/github-actions/required-review/src/main.js b/projects/github-actions/required-review/src/main.js index a6bfb5eb0c407..296ddfd9db44f 100644 --- a/projects/github-actions/required-review/src/main.js +++ b/projects/github-actions/required-review/src/main.js @@ -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 ) ) { diff --git a/projects/github-actions/required-review/src/requirement.js b/projects/github-actions/required-review/src/requirement.js index 50e3d2f1f8bfd..4d6fd5d735363 100644 --- a/projects/github-actions/required-review/src/requirement.js +++ b/projects/github-actions/required-review/src/requirement.js @@ -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'; @@ -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; @@ -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; } /** diff --git a/projects/github-actions/required-review/test.sh b/projects/github-actions/required-review/test.sh index 1ffd5a6884e54..f8960de04c87f 100755 --- a/projects/github-actions/required-review/test.sh +++ b/projects/github-actions/required-review/test.sh @@ -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" | From aafeb65d6b96d0004f31b8589cf6b36c2b96fad8 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Mon, 6 Mar 2023 13:21:07 -0500 Subject: [PATCH 2/2] Update required-review config This makes the following changes: * Taking advantage of the new `consume` option to DRY things out. * Allow everyone to update `composer.lock` and `changelog/*` in all projects, not only plugins. * Fix #28359, as written the "Monorepo itself" rule still applied to docs so only jetpack-approvers were actually allowed. This is done by leaving `docs/**` in the rule with `consume` set an moving "Monorepo itself" after. * The "Jetpack and packages" rule now only covers Jetpack. A second rule covers those packages (and now js-packages too) that are maintained by Ground Control. All other packages are left to team rules or the catch-all at the end, so we don't have to exclude them anymore. --- .github/files/required-review.yaml | 86 +++++++++++------------------- 1 file changed, 30 insertions(+), 56 deletions(-) diff --git a/.github/files/required-review.yaml b/.github/files/required-review.yaml index 36263ba3962a4..77e56f9fe8850 100644 --- a/.github/files/required-review.yaml +++ b/.github/files/required-review.yaml @@ -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. @@ -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 # 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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -159,9 +136,6 @@ - name: Forms paths: - 'projects/packages/forms/**' - - '!projects/plugins/*/composer.lock' - - '!projects/plugins/*/composer.json' - - '!projects/plugins/*/changelog/*' teams: - '@cgastrell' - '@ice9js'