From 83e4a76afa273f9af8f75947ad0c99f98e04c83a Mon Sep 17 00:00:00 2001 From: Joey Perrott Date: Fri, 20 Mar 2020 06:59:35 -0700 Subject: [PATCH] feat(dev-infra): handle excluding files via globs in pullapprove (#36162) Updates the pullapprove verification script to handle cases of excluding globs from groups. PR Close #36162 --- dev-infra/pullapprove/BUILD.bazel | 4 + dev-infra/pullapprove/group.ts | 158 ++++++++++++++++ dev-infra/pullapprove/logging.ts | 42 +++++ dev-infra/pullapprove/parse-yaml.ts | 33 ++++ dev-infra/pullapprove/verify.ts | 274 +++++++++------------------- 5 files changed, 326 insertions(+), 185 deletions(-) create mode 100644 dev-infra/pullapprove/group.ts create mode 100644 dev-infra/pullapprove/logging.ts create mode 100644 dev-infra/pullapprove/parse-yaml.ts diff --git a/dev-infra/pullapprove/BUILD.bazel b/dev-infra/pullapprove/BUILD.bazel index 5989f338e6..9d1ffb6d7d 100644 --- a/dev-infra/pullapprove/BUILD.bazel +++ b/dev-infra/pullapprove/BUILD.bazel @@ -3,11 +3,15 @@ load("@npm_bazel_typescript//:index.bzl", "ts_library") ts_library( name = "pullapprove", srcs = [ + "group.ts", + "logging.ts", + "parse-yaml.ts", "verify.ts", ], module_name = "@angular/dev-infra-private/pullapprove", visibility = ["//dev-infra:__subpackages__"], deps = [ + "//dev-infra/utils:config", "@npm//@types/minimatch", "@npm//@types/node", "@npm//@types/shelljs", diff --git a/dev-infra/pullapprove/group.ts b/dev-infra/pullapprove/group.ts new file mode 100644 index 0000000000..4a4630ad21 --- /dev/null +++ b/dev-infra/pullapprove/group.ts @@ -0,0 +1,158 @@ +/** + * @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, match} from 'minimatch'; + +import {PullApproveGroupConfig} from './parse-yaml'; + +/** A condition for a group. */ +interface GroupCondition { + glob: string; + matcher: IMinimatch; + matchedFiles: Set; +} + +/** Result of testing files against the group. */ +export interface PullApproveGroupResult { + groupName: string; + matchedIncludes: GroupCondition[]; + matchedExcludes: GroupCondition[]; + matchedCount: number; + unmatchedIncludes: GroupCondition[]; + unmatchedExcludes: GroupCondition[]; + unmatchedCount: number; +} + +// Regex Matcher for contains_any_globs conditions +const CONTAINS_ANY_GLOBS_REGEX = /^'([^']+)',?$/; + +const CONDITION_TYPES = { + INCLUDE_GLOBS: /^contains_any_globs/, + EXCLUDE_GLOBS: /^not contains_any_globs/, + ATTR_LENGTH: /^len\(.*\)/, +}; + +/** 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; + + constructor(public groupName: string, group: PullApproveGroupConfig) { + for (let condition of group.conditions) { + condition = condition.trim(); + + 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 { + 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); + 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; + } + } + }); + return matched; + } +} + +/** + * 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 new file mode 100644 index 0000000000..af0f649094 --- /dev/null +++ b/dev-infra/pullapprove/logging.ts @@ -0,0 +1,42 @@ +/** + * @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 {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; + console.groupCollapsed(`[${group.groupName}]`); + if (includeConditions.length) { + console.group('includes'); + includeConditions.forEach( + matcher => console.info(`${matcher.glob} - ${matcher.matchedFiles.size}`)); + 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. */ +export function logHeader(...params: string[]) { + const totalWidth = 80; + const fillWidth = totalWidth - 2; + const headerText = params.join(' ').substr(0, fillWidth); + const leftSpace = Math.ceil((fillWidth - headerText.length) / 2); + const rightSpace = fillWidth - leftSpace - headerText.length; + const fill = (count: number, content: string) => content.repeat(count); + + 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 new file mode 100644 index 0000000000..a74eb3a0cb --- /dev/null +++ b/dev-infra/pullapprove/parse-yaml.ts @@ -0,0 +1,33 @@ +/** + * @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 {parse as parseYaml} from 'yaml'; + +export interface PullApproveGroupConfig { + conditions: string; + reviewers: { + users: string[], + teams: string[], + }; +} + +export interface PullApproveConfig { + version: number; + github_api_version?: string; + pullapprove_conditions?: { + condition: string, + unmet_status: string, + explanation: string, + }[]; + groups: { + [key: string]: PullApproveGroupConfig, + }; +} + +export function parsePullApproveYaml(rawYaml: string): PullApproveConfig { + return parseYaml(rawYaml) as PullApproveConfig; +} \ No newline at end of file diff --git a/dev-infra/pullapprove/verify.ts b/dev-infra/pullapprove/verify.ts index e43c4f8287..5566e78734 100644 --- a/dev-infra/pullapprove/verify.ts +++ b/dev-infra/pullapprove/verify.ts @@ -6,210 +6,114 @@ * found in the LICENSE file at https://angular.io/license */ import {readFileSync} from 'fs'; -import {IMinimatch, Minimatch} from 'minimatch'; import * as path from 'path'; import {cd, exec, set} from 'shelljs'; -import {parse as parseYaml} from 'yaml'; -interface GlobMatcher { - group: string; - glob: string; - matcher: IMinimatch; - matchCount: number; -} +import {PullApproveGroup} from './group'; +import {logGroup, logHeader} from './logging'; +import {parsePullApproveYaml} from './parse-yaml'; +import {getRepoBaseDir} from '../utils/config'; export function verify() { // Exit early on shelljs errors set('-e'); - - // Regex Matcher for contains_any_globs conditions - const CONTAINS_ANY_GLOBS_REGEX = /^'([^']+)',?$/; - // Full path of the angular project directory - const ANGULAR_PROJECT_DIR = process.cwd(); - // Change to the Angular project directory - cd(ANGULAR_PROJECT_DIR); - // 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 + cd(PROJECT_DIR); // Full path to PullApprove config file - const PULL_APPROVE_YAML_PATH = path.resolve(ANGULAR_PROJECT_DIR, '.pullapprove.yml'); + const PULL_APPROVE_YAML_PATH = path.resolve(PROJECT_DIR, '.pullapprove.yml'); // All relative path file names in the git repo, this is retrieved using git rather // that a glob so that we only get files that are checked in, ignoring things like // node_modules, .bazelrc.user, etc - const ALL_FILES = exec('git ls-tree --full-tree -r --name-only HEAD', {silent: true}) - .trim() - .split('\n') - .filter((_: string) => !!_); - if (!ALL_FILES.length) { - console.error( - `No files were found to be in the git tree, did you run this command from \n` + - `inside the angular repository?`); + const REPO_FILES = + exec('git ls-files', {silent: true}).trim().split('\n').filter((_: string) => !!_); + // 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); + }); + // 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); } + // Files which are matched by at least one group. + const matchedFiles: string[] = []; + // Files which are not matched by at least one group. + const unmatchedFiles: string[] = []; - /** Gets the glob matching information from each group's condition. */ - function getGlobMatchersFromCondition( - groupName: string, condition: string): [GlobMatcher[], string[]] { - const trimmedCondition = condition.trim(); - const globMatchers: GlobMatcher[] = []; - const badConditionLines: string[] = []; - - // If the condition starts with contains_any_globs, evaluate all of the globs - if (trimmedCondition.startsWith('contains_any_globs')) { - trimmedCondition.split('\n') - .slice(1, -1) - .map(glob => { - const trimmedGlob = glob.trim(); - const match = trimmedGlob.match(CONTAINS_ANY_GLOBS_REGEX); - if (!match) { - badConditionLines.push(trimmedGlob); - return ''; - } - return match[1]; - }) - .filter(globString => !!globString) - .forEach(globString => globMatchers.push({ - group: groupName, - glob: globString, - matcher: new Minimatch(globString, {dot: true}), - matchCount: 0, - })); - } - return [globMatchers, badConditionLines]; - } - - /** Create logs for each review group. */ - function logGroups(groups: Map>) { - Array.from(groups.entries()).sort().forEach(([groupName, globs]) => { - console.groupCollapsed(groupName); - Array.from(globs.values()) - .sort((a, b) => b.matchCount - a.matchCount) - .forEach(glob => console.info(`${glob.glob} - ${glob.matchCount}`)); - console.groupEnd(); - }); - } - - /** Logs a header within a text drawn box. */ - function logHeader(...params: string[]) { - const totalWidth = 80; - const fillWidth = totalWidth - 2; - const headerText = params.join(' ').substr(0, fillWidth); - const leftSpace = Math.ceil((fillWidth - headerText.length) / 2); - const rightSpace = fillWidth - leftSpace - headerText.length; - const fill = (count: number, content: string) => content.repeat(count); - - console.info(`┌${fill(fillWidth, '─')}┐`); - console.info(`│${fill(leftSpace, ' ')}${headerText}${fill(rightSpace, ' ')}│`); - console.info(`└${fill(fillWidth, '─')}┘`); - } - - /** Runs the pull approve verification check on provided files. */ - function runVerification(files: string[]) { - // All of the globs created for each group's conditions. - const allGlobs: GlobMatcher[] = []; - // The pull approve config file. - const pullApprove = readFileSync(PULL_APPROVE_YAML_PATH, {encoding: 'utf8'}); - // All of the PullApprove groups, parsed from the PullApprove yaml file. - const parsedPullApproveGroups = - parseYaml(pullApprove).groups as{[key: string]: {conditions: string}}; - // All files which were found to match a condition in PullApprove. - const matchedFiles = new Set(); - // All files which were not found to match a condition in PullApprove. - const unmatchedFiles = new Set(); - // All PullApprove groups which matched at least one file. - const matchedGroups = new Map>(); - // All PullApprove groups which did not match at least one file. - const unmatchedGroups = new Map>(); - // All condition lines which were not able to be correctly parsed, by group. - const badConditionLinesByGroup = new Map(); - // Total number of condition lines which were not able to be correctly parsed. - let badConditionLineCount = 0; - - // Get all of the globs from the PullApprove group conditions. - Object.entries(parsedPullApproveGroups).forEach(([groupName, group]) => { - for (const condition of group.conditions) { - const [matchers, badConditions] = getGlobMatchersFromCondition(groupName, condition); - if (badConditions.length) { - badConditionLinesByGroup.set(groupName, badConditions); - badConditionLineCount += badConditions.length; - } - allGlobs.push(...matchers); - } - }); - - if (badConditionLineCount) { - console.info(`Discovered ${badConditionLineCount} parsing errors in PullApprove conditions`); - console.info(`Attempted parsing using: ${CONTAINS_ANY_GLOBS_REGEX}`); - console.info(); - console.info(`Unable to properly parse the following line(s) by group:`); - badConditionLinesByGroup.forEach((badConditionLines, groupName) => { - console.info(`- ${groupName}:`); - badConditionLines.forEach(line => console.info(` ${line}`)); - }); - console.info(); - console.info( - `Please correct the invalid conditions, before PullApprove verification can be completed`); - process.exit(1); - } - - // Check each file for if it is matched by a PullApprove condition. - for (let file of files) { - const matched = allGlobs.filter(glob => glob.matcher.match(file)); - matched.length ? matchedFiles.add(file) : unmatchedFiles.add(file); - matched.forEach(glob => glob.matchCount++); - } - - // Add each glob for each group to a map either matched or unmatched. - allGlobs.forEach(glob => { - const groups = glob.matchCount ? matchedGroups : unmatchedGroups; - const globs = groups.get(glob.group) || new Map(); - // Set the globs map in the groups map - groups.set(glob.group, globs); - // Set the glob in the globs map - globs.set(glob.glob, glob); - }); - - // PullApprove is considered verified if no files or groups are found to be unsed. - const verificationSucceeded = !(unmatchedFiles.size || unmatchedGroups.size); - - /** - * Overall result - */ - logHeader('Result'); - if (verificationSucceeded) { - console.info('PullApprove verification succeeded!'); + // 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) { + matchedFiles.push(file); } else { - console.info(`PullApprove verification failed.\n`); - console.info(`Please update '.pullapprove.yml' to ensure that all necessary`); - console.info(`files/directories have owners and all patterns that appear in`); - console.info(`the file correspond to actual files/directories in the repo.`); + unmatchedFiles.push(file); } - /** - * File by file Summary - */ - logHeader('PullApprove file match results'); - console.groupCollapsed(`Matched Files (${matchedFiles.size} files)`); - VERBOSE_MODE && matchedFiles.forEach(file => console.info(file)); - console.groupEnd(); - console.groupCollapsed(`Unmatched Files (${unmatchedFiles.size} files)`); - unmatchedFiles.forEach(file => console.info(file)); - console.groupEnd(); + }); + // Results for each group + const resultsByGroup = groupsWithMatchers.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 = + resultsByGroup.every(r => !r.unmatchedCount) && !unmatchedFiles.length; - /** - * Group by group Summary - */ - logHeader('PullApprove group matches'); - console.groupCollapsed(`Matched Groups (${matchedGroups.size} groups)`); - VERBOSE_MODE && logGroups(matchedGroups); - console.groupEnd(); - console.groupCollapsed(`Unmatched Groups (${unmatchedGroups.size} groups)`); - logGroups(unmatchedGroups); - console.groupEnd(); - - // Provide correct exit code based on verification success. - process.exit(verificationSucceeded ? 0 : 1); + /** + * Overall result + */ + logHeader('Overall Result'); + if (verificationSucceeded) { + console.info('PullApprove verification succeeded!'); + } else { + console.info(`PullApprove verification failed.\n`); + console.info(`Please update '.pullapprove.yml' to ensure that all necessary`); + console.info(`files/directories have owners and all patterns that appear in`); + console.info(`the file correspond to actual files/directories in the repo.`); } + /** + * File by file Summary + */ + logHeader('PullApprove results by file'); + console.groupCollapsed(`Matched Files (${matchedFiles.length} files)`); + VERBOSE_MODE && matchedFiles.forEach(file => console.info(file)); + console.groupEnd(); + console.groupCollapsed(`Unmatched Files (${unmatchedFiles.length} files)`); + unmatchedFiles.forEach(file => console.info(file)); + console.groupEnd(); + /** + * 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.groupEnd(); + const matchedGroups = resultsByGroup.filter(group => !group.unmatchedCount); + console.groupCollapsed(`Matched conditions by Group (${matchedGroups.length} groups)`); + VERBOSE_MODE && matchedGroups.forEach(group => logGroup(group)); + console.groupEnd(); + const unmatchedGroups = resultsByGroup.filter(group => group.unmatchedCount); + console.groupCollapsed(`Unmatched conditions by Group (${unmatchedGroups.length} groups)`); + unmatchedGroups.forEach(group => logGroup(group, false)); + console.groupEnd(); - - runVerification(ALL_FILES); + // Provide correct exit code based on verification success. + process.exit(verificationSucceeded ? 0 : 1); }