From da526345859007ee5ff2d0247025d8d775131974 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 2 Dec 2025 18:28:51 +0100 Subject: [PATCH] build: set up PullApprove for reviews Replaces Github's code review assignments with PullApprove which is a bit more configurable and which can help reduce noise for the team. --- .github/CODEOWNERS | 14 ----- .github/workflows/ci.yml | 4 +- .github/workflows/pr.yml | 4 +- .pullapprove.yml | 107 +++++++++++++++++++++++++++++++++++++++ package.json | 3 +- scripts/ownerslint.mts | 86 ------------------------------- 6 files changed, 112 insertions(+), 106 deletions(-) delete mode 100644 .github/CODEOWNERS create mode 100644 .pullapprove.yml delete mode 100644 scripts/ownerslint.mts diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS deleted file mode 100644 index ae11ddfd8e48..000000000000 --- a/.github/CODEOWNERS +++ /dev/null @@ -1,14 +0,0 @@ -* @angular/components-googlers - -/.circleci/ @angular/dev-infra-components -/.husky/ @angular/dev-infra-components -/.ng-dev/ @angular/dev-infra-components -/.vscode/ @angular/dev-infra-components -/scripts/ @angular/dev-infra-components -/test/ @angular/dev-infra-components - -/.github/ @angular/dev-infra-components -/.github/CODEOWNERS @angular/dev-infra-components @andrewseguin @jelbourn -/.github/ISSUE_TEMPLATE/** @angular/components-googlers - -/tools/ @angular/dev-infra-components diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0bfb3eca1583..0d107ec736f8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,8 +32,8 @@ jobs: run: | bazel build //:entry_points_manifest pnpm check-entry-point-setup $(bazel info bazel-bin)/entry_points_manifest.json - - name: Check OWNERS file - run: pnpm ownerslint + - name: Validate pull approve configuration + run: pnpm ng-dev pullapprove verify - name: Check for component id collisions run: pnpm detect-component-id-collisions - name: Check style lint diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 5d093eab9329..c624ddfebd60 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -30,8 +30,8 @@ jobs: run: | bazel build //:entry_points_manifest pnpm check-entry-point-setup $(bazel info bazel-bin)/entry_points_manifest.json - - name: Check OWNERS file - run: pnpm ownerslint + - name: Validate pull approve configuration + run: pnpm ng-dev pullapprove verify - name: Check for component id collisions run: pnpm detect-component-id-collisions - name: Check style lint diff --git a/.pullapprove.yml b/.pullapprove.yml new file mode 100644 index 000000000000..1d4032127406 --- /dev/null +++ b/.pullapprove.yml @@ -0,0 +1,107 @@ +version: 3 + +# Add users here if they're temporarily unavailable. +#availability: +# users_unavailable: [] + +# Meta field that goes unused by PullApprove to allow for defining aliases to be +# used throughout the config. +meta: + no-groups-above-this-active: &no-groups-above-this-active len(groups.active.exclude("components-team")) == 0 + + defaults: &defaults + reviews: + # Authors provide their approval implicitly, this approval allows for a reviewer + # from a group not to need a review specifically for an area of the repository + # they own. This is coupled with the `required-minimum-review` group which requires + # that all PRs are reviewed by at least one team member who is not the author of + # the PR. + author_value: 1 + +# turn on 'draft' support +# https://docs.pullapprove.com/config/github-api-version/ +# https://developer.github.com/v3/previews/#draft-pull-requests +github_api_version: 'shadow-cat-preview' + +# https://docs.pullapprove.com/config/overrides/ +# Note that overrides are processed in order. +overrides: + # For PRs which are still being worked on, either still in draft mode or indicated through WIP in + # title or label, PullApprove stays in a pending state until its ready for review. + - if: "draft or 'WIP' in title or 'PR state: WIP' in labels" + status: pending + explanation: 'Waiting to send reviews as PR is WIP' + # Disable PullApprove on specific PRs by adding the `PullApprove: disable` label + - if: "'PullApprove: disable' in labels" + status: success + explanation: "PullApprove skipped because of 'PullApprove: disable' label" + # If no file matching based groups are active, report this pull request as failing. Most likely, + # the PR author would need to update the PullApprove config, or create new group. + - if: len(groups.active) == 0 + status: failure + explanation: 'At least one group must match this PR. Please update an existing review group, or create a new group.' + +groups: + components-infra: + <<: *defaults + conditions: + - author not in ["angular-robot"] + - > + contains_any_globs(files, [ + '.bazel_fix_commands.json', + '.bazelignore', + '.bazelrc', + '.bazelversion', + '.firebaserc', + '.git-blame-ignore-revs', + '.gitignore', + '.npmrc', + '.nvmrc', + '.pullapprove.yml', + 'firebase.json', + 'LICENSE', + 'renovate.json', + 'tsconfig.json', + '.github/**/{*,.*}', + '.husky/**/{*,.*}', + '.ng-dev/**/{*,.*}', + '.vscode/**/{*,.*}', + 'scripts/**/{*,.*}', + 'test/**/{*,.*}', + 'tools/**/{*,.*}', + ]) + reviews: + request: 1 # Request at least one review + required: 1 # Require that all PRs have approval from at least one of the users in the group + author_value: 0 # The author of the PR cannot provide an approval for themself + reviewers: + users: + - crisbeto + - devversion + - andrewseguin + - josephperrott + + components-team: + <<: *defaults + conditions: + - *no-groups-above-this-active + - > + contains_any_globs(files, [ + '**/{.*,*}/**', + '**/{.*,*}', + ]) + reviews: + request: 2 # Request at least two reviews + required: 1 # Require that all PRs have approval from at least one of the users in the group + author_value: 0 # The author of the PR cannot provide an approval for themself + reviewers: + users: + - adolgachev + - crisbeto + - mmalerba + - andrewseguin + - wagnermaciel + - tjshiu + - ok7sai + - ~devversion + - ~josephperrott diff --git a/package.json b/package.json index b73445427b48..337b513f699e 100644 --- a/package.json +++ b/package.json @@ -27,7 +27,7 @@ "test-local": "pnpm -s test --local", "test-firefox": "pnpm -s test --firefox", "test-tsec": "pnpm bazelisk test //... --build_tag_filters=tsec --test_tag_filters=tsec", - "lint": "pnpm -s tslint && pnpm -s stylelint && pnpm -s ownerslint && pnpm -s ng-dev format changed --check", + "lint": "pnpm -s tslint && pnpm -s stylelint && pnpm -s ng-dev format changed --check", "e2e": "bazel test //src/... --build_tag_filters=e2e --test_tag_filters=e2e --build_tests_only", "deploy-dev-app": "node ./scripts/deploy-dev-app.js", "breaking-changes": "node --no-warnings=ExperimentalWarning --loader ts-node/esm/transpile-only scripts/breaking-changes.mts", @@ -35,7 +35,6 @@ "check-package-externals": "node --no-warnings=ExperimentalWarning --loader ts-node/esm/transpile-only scripts/check-package-externals.mts", "format": "pnpm -s ng-dev format changed", "cherry-pick-patch": "ts-node --project tools/cherry-pick-patch/tsconfig.json tools/cherry-pick-patch/cherry-pick-patch.ts", - "ownerslint": "node --no-warnings=ExperimentalWarning --loader ts-node/esm/transpile-only scripts/ownerslint.mts", "detect-component-id-collisions": "node --no-warnings=ExperimentalWarning --loader ts-node/esm/transpile-only scripts/detect-component-id-collisions.mts", "tslint": "tslint -c tslint.json --project ./tsconfig.json", "stylelint": "stylelint \"(src|docs)/**/*.+(css|scss)\" --config .stylelintrc.json", diff --git a/scripts/ownerslint.mts b/scripts/ownerslint.mts deleted file mode 100644 index ea4b49634d4a..000000000000 --- a/scripts/ownerslint.mts +++ /dev/null @@ -1,86 +0,0 @@ -import chalk from 'chalk'; -import {readdirSync, readFileSync, statSync} from 'fs'; -import {Minimatch} from 'minimatch'; -import {join} from 'path'; - -/** - * Script that lints the CODEOWNERS file and makes sure that all files have an owner. - */ - -/** Path for the Github owners file. */ -const ownersFilePath = '.github/CODEOWNERS'; - -/** Path for the .gitignore file. */ -const gitIgnorePath = '.gitignore'; - -let errors = 0; -const ownedPaths = readFileSync(ownersFilePath, 'utf8') - .split('\n') - // Trim lines. - .map(line => line.trim()) - // Remove empty lines and comments. - .filter(line => line && !line.startsWith('#')) - // Split off just the path glob. - .map(line => line.split(/\s+/)[0]) - // Turn paths into Minimatch objects. - .map(path => new Minimatch(path, {dot: true, matchBase: true})); - -const ignoredPaths = readFileSync(gitIgnorePath, 'utf8') - .split('\n') - // Trim lines. - .map(line => line.trim()) - // Remove empty lines and comments. - .filter(line => line && !line.startsWith('#')) - // Turn paths into Minimatch objects. - .map(path => new Minimatch(path, {dot: true, matchBase: true})); - -for (let paths = getChildPaths('.'); paths.length; ) { - paths = Array.prototype.concat( - ...paths - // Remove ignored paths - .filter( - path => - !ignoredPaths.reduce( - (isIgnored, ignoredPath) => isIgnored || ignoredPath.match('/' + path), - false, - ), - ) - // Remove paths that match an owned path. - .filter( - path => - !ownedPaths.reduce((isOwned, ownedPath) => isOwned || isOwnedBy(path, ownedPath), false), - ) - // Report an error for any files that didn't match any owned paths. - .filter(path => { - if (statSync(path).isFile()) { - console.log(chalk.red(`No code owner found for "${path}".`)); - errors++; - return false; - } - return true; - }) - // Get the next level of children for any directories. - .map(path => getChildPaths(path)), - ); -} - -if (errors) { - throw Error( - `Found ${errors} files with no owner. Code owners for the files ` + - `should be added in the CODEOWNERS file.`, - ); -} - -/** Check if the given path is owned by the given owned path matcher. */ -function isOwnedBy(path: string, ownedPath: Minimatch) { - // If the owned path ends with `**` its safe to eliminate whole directories. - if (ownedPath.pattern.endsWith('**') || statSync(path).isFile()) { - return ownedPath.match('/' + path); - } - return false; -} - -/** Get the immediate child paths of the given path. */ -function getChildPaths(path: string) { - return readdirSync(path).map(child => join(path, child)); -}