diff --git a/.pullapprove.yml b/.pullapprove.yml index 3250e6006a..808700e31b 100644 --- a/.pullapprove.yml +++ b/.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: diff --git a/dev-infra/pullapprove/BUILD.bazel b/dev-infra/pullapprove/BUILD.bazel index 6b341553ae..36cfb2aab2 100644 --- a/dev-infra/pullapprove/BUILD.bazel +++ b/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", diff --git a/dev-infra/pullapprove/cli.ts b/dev-infra/pullapprove/cli.ts index d1036e644e..0557fcdfdd 100644 --- a/dev-infra/pullapprove/cli.ts +++ b/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( - 'verify', 'Verify the pullapprove config', {}, () => verify()); + return localYargs.help() + .strict() + .option('verbose', {alias: ['v'], description: 'Enable verbose logging'}) + .demandCommand() + .command('verify', 'Verify the pullapprove config', {}, ({verbose}) => verify(verbose)); } if (require.main === module) { diff --git a/dev-infra/pullapprove/condition_evaluator.ts b/dev-infra/pullapprove/condition_evaluator.ts new file mode 100644 index 0000000000..65939261e9 --- /dev/null +++ b/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(); + +/** + * 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 { + 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; +} diff --git a/dev-infra/pullapprove/group.ts b/dev-infra/pullapprove/group.ts index d901284497..1468f43031 100644 --- a/dev-infra/pullapprove/group.ts +++ b/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; - matcher: IMinimatch; + expression: string; + checkFn: (files: string[]) => boolean; matchedFiles: Set; } /** Result of testing files against the group. */ export interface PullApproveGroupResult { groupName: string; - matchedIncludes: GroupCondition[]; - matchedExcludes: GroupCondition[]; + matchedConditions: GroupCondition[]; matchedCount: number; - unmatchedIncludes: GroupCondition[]; - unmatchedExcludes: GroupCondition[]; + unmatchedConditions: GroupCondition[]; unmatchedCount: number; } -// Regex Matcher for contains_any_globs conditions -const CONTAINS_ANY_GLOBS_REGEX = /^'([^']+)',?$/; +// Regular expression that matches conditions for the global approval. +const GLOBAL_APPROVAL_CONDITION_REGEX = /^"global-(docs-)?approvers" not in groups.approved$/; -const CONDITION_TYPES = { - INCLUDE_GLOBS: /^contains_any_globs/, - EXCLUDE_GLOBS: /^not contains_any_globs/, - ATTR_LENGTH: /^len\(.*\)/, - GLOBAL_APPROVAL: /^"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 { - // Lines which were not able to be parsed as expected. - private misconfiguredLines: string[] = []; - // 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; + /** List of conditions for the group. */ + conditions: GroupCondition[] = []; - constructor(public groupName: string, group: PullApproveGroupConfig) { - if (group.conditions) { - for (let condition of group.conditions) { - condition = condition.trim(); + constructor(public groupName: string, config: PullApproveGroupConfig) { + this._captureConditions(config); + } - if (condition.match(CONDITION_TYPES.INCLUDE_GLOBS)) { - const [conditions, misconfiguredLines] = getLinesForContainsAnyGlobs(condition); - conditions.forEach(globString => this.includeConditions.push({ - glob: globString, - matcher: new Minimatch(globString, {dot: true}), - matchedFiles: new Set(), - })); - 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(), - })); - 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)) { + private _captureConditions(config: PullApproveGroupConfig) { + if (config.conditions && this.groupName !== FALLBACK_GROUP_NAME) { + return config.conditions.forEach(condition => { + const expression = condition.trim(); + + if (expression.match(GLOBAL_APPROVAL_CONDITION_REGEX)) { // Currently a noop as we don't take any action for global approval conditions. - } else { - const errMessage = - `Unrecognized condition found, unable to parse the following condition: \n\n` + - `From the [${groupName}] group:\n` + - ` - ${condition}` + - `\n\n` + - `Known condition regexs:\n` + - `${Object.entries(CONDITION_TYPES).map(([k, v]) => ` ${k} - ${v}`).join('\n')}` + - `\n\n`; - console.error(errMessage); + return; + } + + try { + this.conditions.push({ + expression, + checkFn: convertConditionToFunction(expression), + matchedFiles: new Set(), + }); + } 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) { - let matched = false; - this.includeConditions.forEach((includeCondition: GroupCondition) => { - if (includeCondition.matcher.match(file)) { - let matchedExclude = false; - this.excludeConditions.forEach((excludeCondition: GroupCondition) => { - if (excludeCondition.matcher.match(file)) { - // Add file as a discovered exclude as it is negating a matched - // include condition. - excludeCondition.matchedFiles.add(file); - matchedExclude = true; - } - }); - // 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; + testFile(filePath: string): boolean { + return this.conditions.every(({matchedFiles, checkFn, expression}) => { + try { + const matchesFile = checkFn([filePath]); + if (matchesFile) { + matchedFiles.add(filePath); } + return matchesFile; + } catch (e) { + const errMessage = `Condition could not be evaluated: \n\n` + + `From the [${this.groupName}] group:\n` + + ` - ${expression}` + + `\n\n${e.message} ${e.stack}\n\n`; + console.error(errMessage); + process.exit(1); } }); - return matched; + } + + /** Retrieve the results for the Group, all matched and unmatched conditions. */ + getResults(): PullApproveGroupResult { + const matchedConditions = this.conditions.filter(c => !!c.matchedFiles.size); + const unmatchedConditions = this.conditions.filter(c => !c.matchedFiles.size); + return { + matchedConditions, + matchedCount: matchedConditions.length, + unmatchedConditions, + unmatchedCount: unmatchedConditions.length, + groupName: this.groupName, + }; } } - -/** - * Extract all of the individual globs from a group condition, - * providing both the valid and invalid lines. - */ -function getLinesForContainsAnyGlobs(lines: string) { - const invalidLines: string[] = []; - const validLines = lines.split('\n') - .slice(1, -1) - .map((glob: string) => { - const trimmedGlob = glob.trim(); - const match = trimmedGlob.match(CONTAINS_ANY_GLOBS_REGEX); - if (!match) { - invalidLines.push(trimmedGlob); - return ''; - } - return match[1]; - }) - .filter(globString => !!globString); - return [validLines, invalidLines]; -} diff --git a/dev-infra/pullapprove/logging.ts b/dev-infra/pullapprove/logging.ts index af0f649094..fdaa6f5a16 100644 --- a/dev-infra/pullapprove/logging.ts +++ b/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 excludeConditions = matched ? group.matchedExcludes : group.unmatchedExcludes; + const conditions = matched ? group.matchedConditions : group.unmatchedConditions; console.groupCollapsed(`[${group.groupName}]`); - if (includeConditions.length) { - console.group('includes'); - includeConditions.forEach( - matcher => console.info(`${matcher.glob} - ${matcher.matchedFiles.size}`)); + if (conditions.length) { + conditions.forEach(matcher => { + const count = 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. */ @@ -39,4 +33,4 @@ export function logHeader(...params: string[]) { console.info(`┌${fill(fillWidth, '─')}┐`); console.info(`│${fill(leftSpace, ' ')}${headerText}${fill(rightSpace, ' ')}│`); console.info(`└${fill(fillWidth, '─')}┘`); -} \ No newline at end of file +} diff --git a/dev-infra/pullapprove/parse-yaml.ts b/dev-infra/pullapprove/parse-yaml.ts index 5b3d89d47e..9bc609366c 100644 --- a/dev-infra/pullapprove/parse-yaml.ts +++ b/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[], diff --git a/dev-infra/pullapprove/verify.ts b/dev-infra/pullapprove/verify.ts index f92fd073a3..02827765dd 100644 --- a/dev-infra/pullapprove/verify.ts +++ b/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. - const groupsWithoutMatchers = groups.filter(group => !group.hasMatchers); - // PullApprove groups with matchers. - const groupsWithMatchers = groups.filter(group => group.hasMatchers); - // All lines from group conditions which are not parsable. - 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); - } + // PullApprove groups without conditions. These are skipped in the verification + // as those would always be active and cause zero unmatched files. + const groupsSkipped = groups.filter(group => !group.conditions.length); + // PullApprove groups with conditions. + const groupsWithConditions = groups.filter(group => !!group.conditions.length); // 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)`); - VERBOSE_MODE && groupsWithoutMatchers.forEach(group => console.info(`${group.groupName}`)); + console.groupCollapsed(`Groups skipped (${groupsSkipped.length} groups)`); + 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)`);