diff --git a/dev-infra/pullapprove/BUILD.bazel b/dev-infra/pullapprove/BUILD.bazel index f73a2aaeca..4cd28756f0 100644 --- a/dev-infra/pullapprove/BUILD.bazel +++ b/dev-infra/pullapprove/BUILD.bazel @@ -1,4 +1,5 @@ load("@npm_bazel_typescript//:index.bzl", "ts_library") +load("//tools:defaults.bzl", "jasmine_node_test") ts_library( name = "pullapprove", @@ -8,6 +9,8 @@ ts_library( "group.ts", "logging.ts", "parse-yaml.ts", + "pullapprove_arrays.ts", + "utils.ts", "verify.ts", ], module_name = "@angular/dev-infra-private/pullapprove", @@ -25,3 +28,24 @@ ts_library( "@npm//yargs", ], ) + +ts_library( + name = "pullapprove_test_lib", + testonly = True, + srcs = glob( + ["*.spec.ts"], + ), + visibility = ["//visibility:private"], + deps = [ + ":pullapprove", + "@npm//@types/jasmine", + "@npm//@types/node", + "@npm//typescript", + ], +) + +jasmine_node_test( + name = "pullapprove_test", + srcs = [":pullapprove_test_lib"], + visibility = ["//visibility:private"], +) diff --git a/dev-infra/pullapprove/condition_evaluator.ts b/dev-infra/pullapprove/condition_evaluator.ts index 340e0b84a2..b64eb7a5a8 100644 --- a/dev-infra/pullapprove/condition_evaluator.ts +++ b/dev-infra/pullapprove/condition_evaluator.ts @@ -6,10 +6,9 @@ * 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(); +import {PullApproveGroup} from './group'; +import {PullApproveGroupArray, PullApproveStringArray} from './pullapprove_arrays'; +import {getOrCreateGlob} from './utils'; /** * Context that is provided to conditions. Conditions can use various helpers @@ -18,30 +17,34 @@ const patternCache = new Map(); */ const conditionContext = { 'len': (value: any[]) => value.length, - 'contains_any_globs': (files: PullApproveArray, patterns: string[]) => { + 'contains_any_globs': (files: PullApproveStringArray, 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), ` +export function convertConditionToFunction(expr: string): ( + files: string[], groups: PullApproveGroup[]) => 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 second parameter is the list of + // PullApproveGroups that are accessible in the condition expressions. The followed parameters + // correspond to other context variables provided by PullApprove for conditions. + const evaluateFn = new Function('files', 'groups', ...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)); + return (files, groups) => { + const result = evaluateFn( + new PullApproveStringArray(...files), new PullApproveGroupArray(...groups), + ...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)) { @@ -59,41 +62,3 @@ export function convertConditionToFunction(expr: string): (files: string[]) => b 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 a27ef876cf..45de9eac23 100644 --- a/dev-infra/pullapprove/group.ts +++ b/dev-infra/pullapprove/group.ts @@ -6,14 +6,15 @@ * found in the LICENSE file at https://angular.io/license */ -import {error} from '../utils/console'; +import {error, warn} from '../utils/console'; import {convertConditionToFunction} from './condition_evaluator'; import {PullApproveGroupConfig} from './parse-yaml'; +import {PullApproveGroupStateDependencyError} from './pullapprove_arrays'; /** A condition for a group. */ interface GroupCondition { expression: string; - checkFn: (files: string[]) => boolean; + checkFn: (files: string[], groups: PullApproveGroup[]) => boolean; matchedFiles: Set; } @@ -39,7 +40,9 @@ export class PullApproveGroup { /** List of conditions for the group. */ conditions: GroupCondition[] = []; - constructor(public groupName: string, config: PullApproveGroupConfig) { + constructor( + public groupName: string, config: PullApproveGroupConfig, + readonly precedingGroups: PullApproveGroup[] = []) { this._captureConditions(config); } @@ -78,18 +81,30 @@ export class PullApproveGroup { testFile(filePath: string): boolean { return this.conditions.every(({matchedFiles, checkFn, expression}) => { try { - const matchesFile = checkFn([filePath]); + const matchesFile = checkFn([filePath], this.precedingGroups); 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`; - error(errMessage); - process.exit(1); + // In the case of a condition that depends on the state of groups we want to just + // warn that the verification can't accurately evaluate the condition and then + // continue processing. Other types of errors fail the verification, as conditions + // should otherwise be able to execute without throwing. + if (e instanceof PullApproveGroupStateDependencyError) { + const errMessage = `Condition could not be evaluated: \n` + + `${e.message}\n` + + `From the [${this.groupName}] group:\n` + + ` - ${expression}`; + warn(errMessage); + } else { + const errMessage = `Condition could not be evaluated: \n\n` + + `From the [${this.groupName}] group:\n` + + ` - ${expression}` + + `\n\n${e.message} ${e.stack}\n\n`; + error(errMessage); + process.exit(1); + } } }); } diff --git a/dev-infra/pullapprove/parse-yaml.ts b/dev-infra/pullapprove/parse-yaml.ts index c0856d1763..fc0289ecce 100644 --- a/dev-infra/pullapprove/parse-yaml.ts +++ b/dev-infra/pullapprove/parse-yaml.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ import {parse as parseYaml} from 'yaml'; +import {PullApproveGroup} from './group'; export interface PullApproveGroupConfig { conditions?: string[]; @@ -33,3 +34,12 @@ export interface PullApproveConfig { export function parsePullApproveYaml(rawYaml: string): PullApproveConfig { return parseYaml(rawYaml, {merge: true}) as PullApproveConfig; } + +/** Parses all of the groups defined in the pullapprove yaml. */ +export function getGroupsFromYaml(pullApproveYamlRaw: string): PullApproveGroup[] { + /** JSON representation of the pullapprove yaml file. */ + const pullApprove = parsePullApproveYaml(pullApproveYamlRaw); + return Object.entries(pullApprove.groups).reduce((groups, [groupName, group]) => { + return groups.concat(new PullApproveGroup(groupName, group, groups)); + }, [] as PullApproveGroup[]); +} diff --git a/dev-infra/pullapprove/pullapprove_arrays.ts b/dev-infra/pullapprove/pullapprove_arrays.ts new file mode 100644 index 0000000000..46cec6d522 --- /dev/null +++ b/dev-infra/pullapprove/pullapprove_arrays.ts @@ -0,0 +1,89 @@ +/** + * @license + * Copyright Google LLC 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 {PullApproveGroup} from './group'; +import {getOrCreateGlob} from './utils'; + +export class PullApproveGroupStateDependencyError extends Error { + constructor(message?: string) { + super(message); + // 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, PullApproveGroupStateDependencyError.prototype); + // Error names are displayed in their stack but can't be set in the constructor. + this.name = PullApproveGroupStateDependencyError.name; + } +} + +/** + * Superset of a native array. The superset provides methods which mimic the + * list data structure used in PullApprove for files in conditions. + */ +export class PullApproveStringArray 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, PullApproveStringArray.prototype); + } + /** Returns a new array which only includes files that match the given pattern. */ + include(pattern: string): PullApproveStringArray { + return new PullApproveStringArray(...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): PullApproveStringArray { + return new PullApproveStringArray(...this.filter(s => !getOrCreateGlob(pattern).match(s))); + } +} + +/** + * Superset of a native array. The superset provides methods which mimic the + * list data structure used in PullApprove for groups in conditions. + */ +export class PullApproveGroupArray extends Array { + constructor(...elements: PullApproveGroup[]) { + 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, PullApproveGroupArray.prototype); + } + + include(pattern: string): PullApproveGroupArray { + return new PullApproveGroupArray(...this.filter(s => s.groupName.match(pattern))); + } + + /** Returns a new array which only includes files that did not match the given pattern. */ + exclude(pattern: string): PullApproveGroupArray { + return new PullApproveGroupArray(...this.filter(s => s.groupName.match(pattern))); + } + + get pending() { + throw new PullApproveGroupStateDependencyError(); + } + + get active() { + throw new PullApproveGroupStateDependencyError(); + } + + get inactive() { + throw new PullApproveGroupStateDependencyError(); + } + + get rejected() { + throw new PullApproveGroupStateDependencyError(); + } + + get names() { + return this.map(g => g.groupName); + } +} diff --git a/dev-infra/pullapprove/utils.ts b/dev-infra/pullapprove/utils.ts new file mode 100644 index 0000000000..8e93f07a9f --- /dev/null +++ b/dev-infra/pullapprove/utils.ts @@ -0,0 +1,24 @@ +/** + * @license + * Copyright Google LLC 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(); + +/** + * Gets a glob for the given pattern. The cached glob will be returned + * if available. Otherwise a new glob will be created and cached. + */ +export 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/verify.spec.ts b/dev-infra/pullapprove/verify.spec.ts new file mode 100644 index 0000000000..f3ddb62088 --- /dev/null +++ b/dev-infra/pullapprove/verify.spec.ts @@ -0,0 +1,88 @@ +/** + * @license + * Copyright Google LLC 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 {PullApproveGroup} from './group'; +import {getGroupsFromYaml} from './parse-yaml'; + +describe('group parsing', () => { + it('gets group name', () => { + const groupName = 'fw-migrations'; + const groups = getGroupsFromYaml(` + groups: + ${groupName}: + type: optional + `); + expect(groups[0].groupName).toBe(groupName); + }); + + it('gets correct number of groups', () => { + const groups = getGroupsFromYaml(` + groups: + fw-migrations: + type: optional + fw-core: + type: optional + `); + expect(groups.length).toBe(2); + }); + + it('gets preceding groups', () => { + const groups = getGroupsFromYaml(` + groups: + fw-migrations: + type: optional + fw-core: + type: optional + dev-infra: + type: optional + `); + const fwMigrations = getGroupByName(groups, 'fw-migrations')!; + const fwCore = getGroupByName(groups, 'fw-core')!; + const devInfra = getGroupByName(groups, 'dev-infra')!; + expect(getGroupNames(fwMigrations.precedingGroups)).toEqual([]); + expect(getGroupNames(fwCore.precedingGroups)).toEqual([fwMigrations.groupName]); + expect(getGroupNames(devInfra.precedingGroups)).toEqual([ + fwMigrations.groupName, fwCore.groupName + ]); + }); + + it('matches file conditions', () => { + const groups = getGroupsFromYaml(` + groups: + fw-core: + conditions: + - contains_any_globs(files, ['packages/core/**']) + `); + const fwCore = getGroupByName(groups, 'fw-core')!; + expect(fwCore.testFile('packages/core/test.ts')).toBe(true); + expect(fwCore.testFile('some/other/location/test.ts')).toBe(false); + }); + + it('allows conditions based on groups', () => { + const groups = getGroupsFromYaml(` + groups: + fw-migrations: + conditions: + - len(groups) > 0 + fw-core: + conditions: + - len(groups.active) > 0 + `); + const fwMigrations = getGroupByName(groups, 'fw-migrations')!; + expect(() => fwMigrations.testFile('any')).not.toThrow(); + const fwCore = getGroupByName(groups, 'fw-core')!; + expect(() => fwCore.testFile('any')).not.toThrow(); + }); +}); + +function getGroupByName(groups: PullApproveGroup[], name: string): PullApproveGroup|undefined { + return groups.find(g => g.groupName === name); +} + +function getGroupNames(groups: PullApproveGroup[]) { + return groups.map(g => g.groupName); +} diff --git a/dev-infra/pullapprove/verify.ts b/dev-infra/pullapprove/verify.ts index f349edeeef..d8302c2caa 100644 --- a/dev-infra/pullapprove/verify.ts +++ b/dev-infra/pullapprove/verify.ts @@ -11,10 +11,8 @@ import {resolve} from 'path'; import {getRepoBaseDir} from '../utils/config'; import {debug, info} from '../utils/console'; import {allFiles} from '../utils/repo-files'; - -import {PullApproveGroup} from './group'; import {logGroup, logHeader} from './logging'; -import {parsePullApproveYaml} from './parse-yaml'; +import {getGroupsFromYaml} from './parse-yaml'; export function verify() { /** Full path to PullApprove config file */ @@ -23,12 +21,8 @@ export function verify() { const REPO_FILES = allFiles(); /** The pull approve config file. */ const pullApproveYamlRaw = readFileSync(PULL_APPROVE_YAML_PATH, 'utf8'); - /** JSON representation of the pullapprove yaml file. */ - const pullApprove = parsePullApproveYaml(pullApproveYamlRaw); /** All of the groups defined in the pullapprove yaml. */ - const groups = Object.entries(pullApprove.groups).map(([groupName, group]) => { - return new PullApproveGroup(groupName, group); - }); + const groups = getGroupsFromYaml(pullApproveYamlRaw); /** * PullApprove groups without conditions. These are skipped in the verification * as those would always be active and cause zero unmatched files. diff --git a/dev-infra/utils/BUILD.bazel b/dev-infra/utils/BUILD.bazel index f1bdc1976d..248aaddacf 100644 --- a/dev-infra/utils/BUILD.bazel +++ b/dev-infra/utils/BUILD.bazel @@ -16,6 +16,7 @@ ts_library( "@npm//@types/node", "@npm//@types/shelljs", "@npm//chalk", + "@npm//inquirer", "@npm//shelljs", "@npm//tslib", "@npm//typed-graphqlify",