Update required-review config #29318
Conversation
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.
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.
anomiex
added
[Type] Enhancement
github-actions
bot
added
the
Actions
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. |
|
I think the list is capitalization sensitive, but afaik, GH user names are unique without regard to capitalization. While you're in there, is that something you could look at? #29320 |
Sure, but not on this PR. |
| # 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
anomiex
removed
the
[Status] Needs Review
|
This will need reconsideration after #29426. Possibly we won't need anything further here, but maybe some of the rearrangement will still make sense. |
Proposed changes:
This makes the following changes:
consumeoption to DRY things out.composer.lockandchangelog/*in all projects, not only plugins.docs/**in the rule withconsumeset an moving "Monorepo itself" after.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No
Testing instructions:
consumeoption #29317 has been released!