From ed3c54906306be8db4951f2c6f42284a530721d7 Mon Sep 17 00:00:00 2001
From: Joey Perrott <josephperrott@gmail.com>
Date: Mon, 6 Apr 2020 11:03:40 -0700
Subject: [PATCH] ci: remove IgorMinar from reviewers list for pullapprove
 fallback group (#36456)

Historically we have had a pullapprove group `fallback` which acted as
a catch all for files which did not match any other groups.  This
group assigned reviews to IgorMinar, however it was not apparent that
this group was assigned.  This change removes this assignment.  This
group as active should always coincide with failures of the pullapprove
verification script. We continue to have this group as a secondary test
ensuring all files in the repo are captured by the pullapprove config.

PR Close #36456
---
 .pullapprove.yml                    | 34 +++++++++++++++++++++++------
 dev-infra/pullapprove/parse-yaml.ts |  2 +-
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/.pullapprove.yml b/.pullapprove.yml
index dfd8f3183a..40cc9f84f2 100644
--- a/.pullapprove.yml
+++ b/.pullapprove.yml
@@ -1126,12 +1126,32 @@ groups:
   #  Catch all for if no groups match the code change
   # ====================================================
   fallback:
+    # 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:
       - *can-be-global-approved
-      # Groups which are found to have matching conditions are `active`
-      # according to PullApprove. If no groups are matched and considered
-      # active, we still want to have a review occur.
-      - len(groups.active) == 0
-    reviewers:
-      users:
-        - IgorMinar
+      # The following groups have no conditions and will be `active` on all PRs
+      # - `global-approvers`
+      # - `global-docs-approvers`
+      #
+      # Since this means the minimum number of active groups a PR can have is 2, this
+      # `fallback` group should be matched anytime the number of active groups is at or
+      # below this minimum.  This work as a protection to ensure that pullapprove does
+      # not incidently mark a PR as passing without meeting the review criteria.
+      - len(groups.active) <= 2
diff --git a/dev-infra/pullapprove/parse-yaml.ts b/dev-infra/pullapprove/parse-yaml.ts
index d733ab3ca4..9e63ce6a98 100644
--- a/dev-infra/pullapprove/parse-yaml.ts
+++ b/dev-infra/pullapprove/parse-yaml.ts
@@ -9,7 +9,7 @@ import {parse as parseYaml} from 'yaml';
 
 export interface PullApproveGroupConfig {
   conditions?: string[];
-  reviewers: {
+  reviewers?: {
     users: string[],
     teams?: string[],
   }|{