feat(dev-infra): pullapprove verify should handle `files` in conditions (#36661)

Currently, when verifying our pullapprove configuration, we don't
respect modifications to the set of files in a condition.

e.g. It's not possible to do the following:

```
contains_any_globs(files.exclude(...), [
```

This prevents us from having codeowner groups which match a directory,
but want to filter out specific sub directories. For example, `fw-core`
matches all files in the core package. We want to exclude the schematics
from that glob. Usually we do this by another exclude condition.

This has a *significant* downside though. It means that fw-core will not
be requested if a PR changes schematic code, _and_ actual fw-core code.

To support these conditions, the pullapprove verification tool is
refactored, so that it no longer uses Regular expressions for parsing,
but rather evaluates the code through a dynamic function. This is
possible since the conditions are written in simple Python that can
be run in NodeJS too (with small modifications/transformations).

PR Close #36661

.pullapprove.yml

@@ -849,7 +849,7 @@ groups:
           'aio/content/images/guide/deployment/**',
           'aio/content/guide/file-structure.md',
           'aio/content/guide/ivy.md',
-          'aio/content/guide/web-worker.md'
+          'aio/content/guide/web-worker.md',
           'aio/content/guide/workspace-config.md',
           ])
     reviewers:

dev-infra/pullapprove/BUILD.bazel

@@ -4,6 +4,7 @@ ts_library(
     name = "pullapprove",
     srcs = [
         "cli.ts",
+        "condition_evaluator.ts",
         "group.ts",
         "logging.ts",
         "parse-yaml.ts",

dev-infra/pullapprove/cli.ts

@@ -10,8 +10,11 @@ import {verify} from './verify';
 
 /** Build the parser for the pullapprove commands. */
 export function buildPullapproveParser(localYargs: yargs.Argv) {
-  return localYargs.help().strict().demandCommand().command(
+  return localYargs.help()
-      'verify', 'Verify the pullapprove config', {}, () => verify());
+      .strict()
+      .option('verbose', {alias: ['v'], description: 'Enable verbose logging'})
+      .demandCommand()
+      .command('verify', 'Verify the pullapprove config', {}, ({verbose}) => verify(verbose));
 }
 
 if (require.main === module) {

dev-infra/pullapprove/condition_evaluator.ts

@@ -0,0 +1,99 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {IMinimatch, Minimatch} from 'minimatch';
+
+/** Map that holds patterns and their corresponding Minimatch globs. */
+const patternCache = new Map<string, IMinimatch>();
+
+/**
+ * Context that is provided to conditions. Conditions can use various helpers
+ * that PullApprove provides. We try to mock them here. Consult the official
+ * docs for more details: https://docs.pullapprove.com/config/conditions.
+ */
+const conditionContext = {
+  'len': (value: any[]) => value.length,
+  'contains_any_globs': (files: PullApproveArray, patterns: string[]) => {
+    // Note: Do not always create globs for the same pattern again. This method
+    // could be called for each source file. Creating glob's is expensive.
+    return files.some(f => patterns.some(pattern => getOrCreateGlob(pattern).match(f)));
+  }
+};
+
+/**
+ * Converts a given condition to a function that accepts a set of files. The returned
+ * function can be called to check if the set of files matches the condition.
+ */
+export function convertConditionToFunction(expr: string): (files: string[]) => boolean {
+  // Creates a dynamic function with the specified expression. The first parameter will
+  // be `files` as that corresponds to the supported `files` variable that can be accessed
+  // in PullApprove condition expressions. The followed parameters correspond to other
+  // context variables provided by PullApprove for conditions.
+  const evaluateFn = new Function('files', ...Object.keys(conditionContext), `
+    return (${transformExpressionToJs(expr)});
+  `);
+
+  // Create a function that calls the dynamically constructed function which mimics
+  // the condition expression that is usually evaluated with Python in PullApprove.
+  return files => {
+    const result = evaluateFn(new PullApproveArray(...files), ...Object.values(conditionContext));
+    // If an array is returned, we consider the condition as active if the array is not
+    // empty. This matches PullApprove's condition evaluation that is based on Python.
+    if (Array.isArray(result)) {
+      return result.length !== 0;
+    }
+    return !!result;
+  };
+}
+
+/**
+ * Transforms a condition expression from PullApprove that is based on python
+ * so that it can be run inside JavaScript. Current transformations:
+ *   1. `not <..>` -> `!<..>`
+ */
+function transformExpressionToJs(expression: string): string {
+  return expression.replace(/not\s+/g, '!');
+}
+
+/**
+ * Superset of a native array. The superset provides methods which mimic the
+ * list data structure used in PullApprove for files in conditions.
+ */
+class PullApproveArray extends Array<string> {
+  constructor(...elements: string[]) {
+    super(...elements);
+
+    // Set the prototype explicitly because in ES5, the prototype is accidentally
+    // lost due to a limitation in down-leveling.
+    // https://github.com/Microsoft/TypeScript/wiki/FAQ#why-doesnt-extending-built-ins-like-error-array-and-map-work.
+    Object.setPrototypeOf(this, PullApproveArray.prototype);
+  }
+
+  /** Returns a new array which only includes files that match the given pattern. */
+  include(pattern: string): PullApproveArray {
+    return new PullApproveArray(...this.filter(s => getOrCreateGlob(pattern).match(s)));
+  }
+
+  /** Returns a new array which only includes files that did not match the given pattern. */
+  exclude(pattern: string): PullApproveArray {
+    return new PullApproveArray(...this.filter(s => !getOrCreateGlob(pattern).match(s)));
+  }
+}
+
+/**
+ * Gets a glob for the given pattern. The cached glob will be returned
+ * if available. Otherwise a new glob will be created and cached.
+ */
+function getOrCreateGlob(pattern: string) {
+  if (patternCache.has(pattern)) {
+    return patternCache.get(pattern)!;
+  }
+  const glob = new Minimatch(pattern, {dot: true});
+  patternCache.set(pattern, glob);
+  return glob;
+}

dev-infra/pullapprove/group.ts

@@ -5,162 +5,101 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
-import {IMinimatch, Minimatch} from 'minimatch';
+import {convertConditionToFunction} from './condition_evaluator';
-
 import {PullApproveGroupConfig} from './parse-yaml';
 
 /** A condition for a group. */
 interface GroupCondition {
-  glob: string;
+  expression: string;
-  matcher: IMinimatch;
+  checkFn: (files: string[]) => boolean;
   matchedFiles: Set<string>;
 }
 
 /** Result of testing files against the group. */
 export interface PullApproveGroupResult {
   groupName: string;
-  matchedIncludes: GroupCondition[];
+  matchedConditions: GroupCondition[];
-  matchedExcludes: GroupCondition[];
   matchedCount: number;
-  unmatchedIncludes: GroupCondition[];
+  unmatchedConditions: GroupCondition[];
-  unmatchedExcludes: GroupCondition[];
   unmatchedCount: number;
 }
 
-// Regex Matcher for contains_any_globs conditions
+// Regular expression that matches conditions for the global approval.
-const CONTAINS_ANY_GLOBS_REGEX = /^'([^']+)',?$/;
+const GLOBAL_APPROVAL_CONDITION_REGEX = /^"global-(docs-)?approvers" not in groups.approved$/;
 
-const CONDITION_TYPES = {
+// Name of the PullApprove group that serves as fallback. This group should never capture
-  INCLUDE_GLOBS: /^contains_any_globs/,
+// any conditions as it would always match specified files. This is not desired as we want
-  EXCLUDE_GLOBS: /^not contains_any_globs/,
+// to figure out as part of this tool, whether there actually are unmatched files.
-  ATTR_LENGTH: /^len\(.*\)/,
+const FALLBACK_GROUP_NAME = 'fallback';
-  GLOBAL_APPROVAL: /^"global-(docs-)?approvers" not in groups.approved$/,
-};
 
 /** A PullApprove group to be able to test files against. */
 export class PullApproveGroup {
-  // Lines which were not able to be parsed as expected.
+  /** List of conditions for the group. */
-  private misconfiguredLines: string[] = [];
+  conditions: GroupCondition[] = [];
-  // Conditions for the group for including files.
-  private includeConditions: GroupCondition[] = [];
-  // Conditions for the group for excluding files.
-  private excludeConditions: GroupCondition[] = [];
-  // Whether the group has file matchers.
-  public hasMatchers = false;
 
-  constructor(public groupName: string, group: PullApproveGroupConfig) {
+  constructor(public groupName: string, config: PullApproveGroupConfig) {
-    if (group.conditions) {
+    this._captureConditions(config);
-      for (let condition of group.conditions) {
+  }
-        condition = condition.trim();
 
-        if (condition.match(CONDITION_TYPES.INCLUDE_GLOBS)) {
+  private _captureConditions(config: PullApproveGroupConfig) {
-          const [conditions, misconfiguredLines] = getLinesForContainsAnyGlobs(condition);
+    if (config.conditions && this.groupName !== FALLBACK_GROUP_NAME) {
-          conditions.forEach(globString => this.includeConditions.push({
+      return config.conditions.forEach(condition => {
-            glob: globString,
+        const expression = condition.trim();
-            matcher: new Minimatch(globString, {dot: true}),
+
-            matchedFiles: new Set<string>(),
+        if (expression.match(GLOBAL_APPROVAL_CONDITION_REGEX)) {
-          }));
-          this.misconfiguredLines.push(...misconfiguredLines);
-          this.hasMatchers = true;
-        } else if (condition.match(CONDITION_TYPES.EXCLUDE_GLOBS)) {
-          const [conditions, misconfiguredLines] = getLinesForContainsAnyGlobs(condition);
-          conditions.forEach(globString => this.excludeConditions.push({
-            glob: globString,
-            matcher: new Minimatch(globString, {dot: true}),
-            matchedFiles: new Set<string>(),
-          }));
-          this.misconfiguredLines.push(...misconfiguredLines);
-          this.hasMatchers = true;
-        } else if (condition.match(CONDITION_TYPES.ATTR_LENGTH)) {
-          // Currently a noop as we do not take any action on this condition type.
-        } else if (condition.match(CONDITION_TYPES.GLOBAL_APPROVAL)) {
           // Currently a noop as we don't take any action for global approval conditions.
-        } else {
+          return;
-          const errMessage =
+        }
-              `Unrecognized condition found, unable to parse the following condition: \n\n` +
+
-              `From the [${groupName}] group:\n` +
+        try {
-              ` - ${condition}` +
+          this.conditions.push({
-              `\n\n` +
+            expression,
-              `Known condition regexs:\n` +
+            checkFn: convertConditionToFunction(expression),
-              `${Object.entries(CONDITION_TYPES).map(([k, v]) => ` ${k} - ${v}`).join('\n')}` +
+            matchedFiles: new Set(),
-              `\n\n`;
+          });
-          console.error(errMessage);
+        } catch (e) {
+          console.error(`Could not parse condition in group: ${this.groupName}`);
+          console.error(` - ${expression}`);
+          console.error(`Error:`, e.message, e.stack);
           process.exit(1);
         }
+      });
     }
   }
-  }
-
-  /** Retrieve all of the lines which were not able to be parsed. */
-  getBadLines(): string[] {
-    return this.misconfiguredLines;
-  }
-
-  /** Retrieve the results for the Group, all matched and unmatched conditions. */
-  getResults(): PullApproveGroupResult {
-    const matchedIncludes = this.includeConditions.filter(c => !!c.matchedFiles.size);
-    const matchedExcludes = this.excludeConditions.filter(c => !!c.matchedFiles.size);
-    const unmatchedIncludes = this.includeConditions.filter(c => !c.matchedFiles.size);
-    const unmatchedExcludes = this.excludeConditions.filter(c => !c.matchedFiles.size);
-    const unmatchedCount = unmatchedIncludes.length + unmatchedExcludes.length;
-    const matchedCount = matchedIncludes.length + matchedExcludes.length;
-    return {
-      matchedIncludes,
-      matchedExcludes,
-      matchedCount,
-      unmatchedIncludes,
-      unmatchedExcludes,
-      unmatchedCount,
-      groupName: this.groupName,
-    };
-  }
 
   /**
    * Tests a provided file path to determine if it would be considered matched by
    * the pull approve group's conditions.
    */
-  testFile(file: string) {
+  testFile(filePath: string): boolean {
-    let matched = false;
+    return this.conditions.every(({matchedFiles, checkFn, expression}) => {
-    this.includeConditions.forEach((includeCondition: GroupCondition) => {
+      try {
-      if (includeCondition.matcher.match(file)) {
+        const matchesFile = checkFn([filePath]);
-        let matchedExclude = false;
+        if (matchesFile) {
-        this.excludeConditions.forEach((excludeCondition: GroupCondition) => {
+          matchedFiles.add(filePath);
-          if (excludeCondition.matcher.match(file)) {
+        }
-            // Add file as a discovered exclude as it is negating a matched
+        return matchesFile;
-            // include condition.
+      } catch (e) {
-            excludeCondition.matchedFiles.add(file);
+        const errMessage = `Condition could not be evaluated: \n\n` +
-            matchedExclude = true;
+            `From the [${this.groupName}] group:\n` +
+            ` - ${expression}` +
+            `\n\n${e.message} ${e.stack}\n\n`;
+        console.error(errMessage);
+        process.exit(1);
       }
     });
-        // An include condition is only considered matched if no exclude
-        // conditions are found to matched the file.
-        if (!matchedExclude) {
-          includeCondition.matchedFiles.add(file);
-          matched = true;
   }
-      }
-    });
-    return matched;
-  }
-}
 
-/**
+  /** Retrieve the results for the Group, all matched and unmatched conditions. */
- * Extract all of the individual globs from a group condition,
+  getResults(): PullApproveGroupResult {
- * providing both the valid and invalid lines.
+    const matchedConditions = this.conditions.filter(c => !!c.matchedFiles.size);
- */
+    const unmatchedConditions = this.conditions.filter(c => !c.matchedFiles.size);
-function getLinesForContainsAnyGlobs(lines: string) {
+    return {
-  const invalidLines: string[] = [];
+      matchedConditions,
-  const validLines = lines.split('\n')
+      matchedCount: matchedConditions.length,
-                         .slice(1, -1)
+      unmatchedConditions,
-                         .map((glob: string) => {
+      unmatchedCount: unmatchedConditions.length,
-                           const trimmedGlob = glob.trim();
+      groupName: this.groupName,
-                           const match = trimmedGlob.match(CONTAINS_ANY_GLOBS_REGEX);
+    };
-                           if (!match) {
-                             invalidLines.push(trimmedGlob);
-                             return '';
   }
-                           return match[1];
-                         })
-                         .filter(globString => !!globString);
-  return [validLines, invalidLines];
 }

dev-infra/pullapprove/logging.ts

@@ -5,26 +5,20 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
+
 import {PullApproveGroupResult} from './group';
 
 /** Create logs for each pullapprove group result. */
 export function logGroup(group: PullApproveGroupResult, matched = true) {
-  const includeConditions = matched ? group.matchedIncludes : group.unmatchedIncludes;
+  const conditions = matched ? group.matchedConditions : group.unmatchedConditions;
-  const excludeConditions = matched ? group.matchedExcludes : group.unmatchedExcludes;
   console.groupCollapsed(`[${group.groupName}]`);
-  if (includeConditions.length) {
+  if (conditions.length) {
-    console.group('includes');
+    conditions.forEach(matcher => {
-    includeConditions.forEach(
+      const count = matcher.matchedFiles.size;
-        matcher => console.info(`${matcher.glob} - ${matcher.matchedFiles.size}`));
+      console.info(`${count} ${count === 1 ? 'match' : 'matches'} - ${matcher.expression}`)
+    });
     console.groupEnd();
   }
-  if (excludeConditions.length) {
-    console.group('excludes');
-    excludeConditions.forEach(
-        matcher => console.info(`${matcher.glob} - ${matcher.matchedFiles.size}`));
-    console.groupEnd();
-  }
-  console.groupEnd();
 }
 
 /** Logs a header within a text drawn box. */

dev-infra/pullapprove/parse-yaml.ts

@@ -8,7 +8,7 @@
 import {parse as parseYaml} from 'yaml';
 
 export interface PullApproveGroupConfig {
-  conditions?: string;
+  conditions?: string[];
   reviewers: {
     users: string[],
     teams?: string[],

dev-infra/pullapprove/verify.ts

@@ -15,11 +15,9 @@ import {PullApproveGroup} from './group';
 import {logGroup, logHeader} from './logging';
 import {parsePullApproveYaml} from './parse-yaml';
 
-export function verify() {
+export function verify(verbose = false) {
   // Exit early on shelljs errors
   set('-e');
-  // Whether to log verbosely
-  const VERBOSE_MODE = process.argv.includes('-v');
   // Full path of the angular project directory
   const PROJECT_DIR = getRepoBaseDir();
   // Change to the Angular project directory
@@ -39,24 +37,11 @@ export function verify() {
   const groups = Object.entries(pullApprove.groups).map(([groupName, group]) => {
     return new PullApproveGroup(groupName, group);
   });
-  // PullApprove groups without matchers.
+  // PullApprove groups without conditions. These are skipped in the verification
-  const groupsWithoutMatchers = groups.filter(group => !group.hasMatchers);
+  // as those would always be active and cause zero unmatched files.
-  // PullApprove groups with matchers.
+  const groupsSkipped = groups.filter(group => !group.conditions.length);
-  const groupsWithMatchers = groups.filter(group => group.hasMatchers);
+  // PullApprove groups with conditions.
-  // All lines from group conditions which are not parsable.
+  const groupsWithConditions = groups.filter(group => !!group.conditions.length);
-  const groupsWithBadLines = groups.filter(g => !!g.getBadLines().length);
-  // If any groups contains bad lines, log bad lines and exit failing.
-  if (groupsWithBadLines.length) {
-    logHeader('PullApprove config file parsing failure');
-    console.info(`Discovered errors in ${groupsWithBadLines.length} groups`);
-    groupsWithBadLines.forEach(group => {
-      console.info(` - [${group.groupName}]`);
-      group.getBadLines().forEach(line => console.info(`    ${line}`));
-    });
-    console.info(
-        `Correct the invalid conditions, before PullApprove verification can be completed`);
-    process.exit(1);
-  }
   // Files which are matched by at least one group.
   const matchedFiles: string[] = [];
   // Files which are not matched by at least one group.
@@ -64,14 +49,14 @@ export function verify() {
 
   // Test each file in the repo against each group for being matched.
   REPO_FILES.forEach((file: string) => {
-    if (groupsWithMatchers.filter(group => group.testFile(file)).length) {
+    if (groupsWithConditions.filter(group => group.testFile(file)).length) {
       matchedFiles.push(file);
     } else {
       unmatchedFiles.push(file);
     }
   });
   // Results for each group
-  const resultsByGroup = groupsWithMatchers.map(group => group.getResults());
+  const resultsByGroup = groupsWithConditions.map(group => group.getResults());
   // Whether all group condition lines match at least one file and all files
   // are matched by at least one group.
   const verificationSucceeded =
@@ -94,7 +79,7 @@ export function verify() {
    */
   logHeader('PullApprove results by file');
   console.groupCollapsed(`Matched Files (${matchedFiles.length} files)`);
-  VERBOSE_MODE && matchedFiles.forEach(file => console.info(file));
+  verbose && matchedFiles.forEach(file => console.info(file));
   console.groupEnd();
   console.groupCollapsed(`Unmatched Files (${unmatchedFiles.length} files)`);
   unmatchedFiles.forEach(file => console.info(file));
@@ -103,12 +88,12 @@ export function verify() {
    * Group by group Summary
    */
   logHeader('PullApprove results by group');
-  console.groupCollapsed(`Groups without matchers (${groupsWithoutMatchers.length} groups)`);
+  console.groupCollapsed(`Groups skipped (${groupsSkipped.length} groups)`);
-  VERBOSE_MODE && groupsWithoutMatchers.forEach(group => console.info(`${group.groupName}`));
+  verbose && groupsSkipped.forEach(group => console.info(`${group.groupName}`));
   console.groupEnd();
   const matchedGroups = resultsByGroup.filter(group => !group.unmatchedCount);
   console.groupCollapsed(`Matched conditions by Group (${matchedGroups.length} groups)`);
-  VERBOSE_MODE && matchedGroups.forEach(group => logGroup(group));
+  verbose && matchedGroups.forEach(group => logGroup(group));
   console.groupEnd();
   const unmatchedGroups = resultsByGroup.filter(group => group.unmatchedCount);
   console.groupCollapsed(`Unmatched conditions by Group (${unmatchedGroups.length} groups)`);