From 4d86ea64710f5d71c829e2435b8cd429eef81e27 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 19 Jul 2021 18:18:41 +0200 Subject: [PATCH] ci: no longer use deprecated `pullapprove_conditions` option (#42895) PullApprove deprecated the `pullapprove_conditions` config option and introduced the `overrides` option. This commit migrates to the new option, while also eliminating the `fallback` group with a simple override (as per recommendation from the pull approve docs). PR Close #42895 --- .pullapprove.yml | 74 ++++++++-------------------------- dev-infra/ng-dev.js | 6 +-- dev-infra/pullapprove/group.ts | 7 +--- 3 files changed, 18 insertions(+), 69 deletions(-) diff --git a/.pullapprove.yml b/.pullapprove.yml index 66e19b8ddc..870eb067a8 100644 --- a/.pullapprove.yml +++ b/.pullapprove.yml @@ -79,7 +79,10 @@ meta: # one are those that appear above it. no-groups-above-this-pending: &no-groups-above-this-pending len(groups.active.pending.exclude("required-minimum-review").exclude("global-approvers").exclude("global-docs-approvers")) == 0 no-groups-above-this-rejected: &no-groups-above-this-rejected len(groups.active.rejected.exclude("required-minimum-review").exclude("global-approvers").exclude("global-docs-approvers")) == 0 - no-groups-above-this-active: &no-groups-above-this-active len(groups.active.exclude("required-minimum-review").exclude("global-approvers").exclude("global-docs-approvers")) == 0 + + # Yaml anchor that represents a PullApprove expression that evaluates to `true` whenever + # there are no actual review groups active. Groups which are always active are excluded. + no-review-groups-active: &no-review-groups-active len(groups.active.exclude("required-minimum-review").exclude("global-approvers").exclude("global-docs-approvers")) == 0 can-be-global-approved: &can-be-global-approved '"global-approvers" not in groups.approved' can-be-global-docs-approved: &can-be-global-docs-approved '"global-docs-approvers" not in groups.approved' @@ -97,22 +100,23 @@ meta: # https://developer.github.com/v3/previews/#draft-pull-requests github_api_version: 'shadow-cat-preview' -pullapprove_conditions: +# 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. - - condition: "'WIP' not in title" - unmet_status: pending + - if: "draft or 'WIP' in title or 'PR state: WIP' in labels" + status: pending explanation: 'Waiting to send reviews as PR is WIP' - - condition: "'PR state: WIP' not in labels" - unmet_status: pending - explanation: 'Waiting to send reviews as PR is WIP' - - condition: 'not draft' - unmet_status: pending - explanation: 'Waiting to send reviews as PR is in draft' # Disable PullApprove on specific PRs by adding the `PullApprove: disable` label - - condition: "'PullApprove: disable' not in labels" - unmet_status: success + - if: "'PullApprove: disable' in labels" + status: success explanation: "PullApprove skipped because of 'PullApprove: disable' label" + # If no 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: *no-review-groups-active + status: failure + explanation: 'At least one group must match this PR. Please update an existing review group, or create a new group.' groups: # ========================================================= @@ -1305,49 +1309,3 @@ groups: - atscott - jelbourn - josephperrott - - # ==================================================== - # Catch all for if no groups match the code change - # ==================================================== - fallback: - <<: *defaults - # A group is considered to be `active` for a PR if at least one of group's - # conditions matches the PR. - # - # The PullApprove CI check should fail if a PR has no `active` groups, as - # this indicates the PR is modifying a file that has no owner. - # - # This is enforced through the pullapprove verification check done - # as part of the CircleCI lint job. Failures in this lint job should be - # fixed as part of the PR. This can be done by updating the - # `.pullapprove.yml` file cover the unmatched path. - # The pullapprove verification script is part of the ng-dev tool and can be - # run locally with the command: `yarn -s ng-dev pullapprove verify` - # - # For cases in which the verification check fails to ensure coverage, this - # group will be active. The expectation is that this should be remedied - # before merging the PR as described above. In an emergency situation - # `global-approvers` can still approve PRs that match this `fallback` rule, - # but that should be an exception and not an expectation. - conditions: - - *no-groups-above-this-active - # When any of the `global-*` groups is approved, they cause other groups to deactivate. - # In those cases, the condition above would evaluate to `true` while in reality, only a global - # approval has been provided. To ensure we don't activate the fallback group in such cases, - # ensure that no explicit global approval has been provided. - - *can-be-global-approved - - *can-be-global-docs-approved - # PullApprove uses a combination of users defined in the pullapprove configuration and the - # number of users who have performed reviews on Github in the recent past if the configuration - # does not specify it. Because, as an open source project, anyone on Github can perform a - # review we need to ensure that all groups, including the fallback group, have at least one user - # or group defined as reviewers. - reviewers: - users: - - josephperrott - reviews: - request: 0 - required: 1 - # Reviewed-for is required on fallback as it should not ever actually be reviewed, requiring - # Reviewed-for helps insure an accidental approval doesn't occur on the fallback group. - reviewed_for: required diff --git a/dev-infra/ng-dev.js b/dev-infra/ng-dev.js index ff363be7b1..cc6db24e76 100755 --- a/dev-infra/ng-dev.js +++ b/dev-infra/ng-dev.js @@ -5180,10 +5180,6 @@ function transformExpressionToJs(expression) { */ // Regular expression that matches conditions for the global approval. const GLOBAL_APPROVAL_CONDITION_REGEX = /^"global-(docs-)?approvers" not in groups.approved$/; -// Name of the PullApprove group that serves as fallback. This group should never capture -// any conditions as it would always match specified files. This is not desired as we want -// to figure out as part of this tool, whether there actually are unmatched files. -const FALLBACK_GROUP_NAME = 'fallback'; /** A PullApprove group to be able to test files against. */ class PullApproveGroup { constructor(groupName, config, precedingGroups = []) { @@ -5196,7 +5192,7 @@ class PullApproveGroup { this.reviewers = (_a = config.reviewers) !== null && _a !== void 0 ? _a : { users: [], teams: [] }; } _captureConditions(config) { - if (config.conditions && this.groupName !== FALLBACK_GROUP_NAME) { + if (config.conditions) { return config.conditions.forEach(condition => { const expression = condition.trim(); if (expression.match(GLOBAL_APPROVAL_CONDITION_REGEX)) { diff --git a/dev-infra/pullapprove/group.ts b/dev-infra/pullapprove/group.ts index ec135d52a9..2c1b266906 100644 --- a/dev-infra/pullapprove/group.ts +++ b/dev-infra/pullapprove/group.ts @@ -37,11 +37,6 @@ export interface PullApproveGroupResult { // Regular expression that matches conditions for the global approval. const GLOBAL_APPROVAL_CONDITION_REGEX = /^"global-(docs-)?approvers" not in groups.approved$/; -// Name of the PullApprove group that serves as fallback. This group should never capture -// any conditions as it would always match specified files. This is not desired as we want -// to figure out as part of this tool, whether there actually are unmatched files. -const FALLBACK_GROUP_NAME = 'fallback'; - /** A PullApprove group to be able to test files against. */ export class PullApproveGroup { /** List of conditions for the group. */ @@ -57,7 +52,7 @@ export class PullApproveGroup { } private _captureConditions(config: PullApproveGroupConfig) { - if (config.conditions && this.groupName !== FALLBACK_GROUP_NAME) { + if (config.conditions) { return config.conditions.forEach(condition => { const expression = condition.trim();