Skip to content

Commit

Permalink
fix(policy-check): Make check policy continue after errors (#12373)
Browse files Browse the repository at this point in the history
The `check policy` command was not working properly. I think the primary
problem was calls to `this.exit(1)` inside routeToHandlers, which made
the process exit instead of continuing to process other handlers.

This change fixes the problem.
  • Loading branch information
tylerbutler authored Oct 11, 2022
1 parent 77ceb12 commit edb1d84
Showing 1 changed file with 39 additions and 41 deletions.
80 changes: 39 additions & 41 deletions build-tools/packages/build-cli/src/commands/check/policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,19 +94,19 @@ export class CheckPolicy extends BaseCommand<typeof CheckPolicy.flags> {
: new RegExp(this.processedFlags.path, "i");

if (this.processedFlags.handler !== undefined) {
this.log(`Filtering handlers by regex: ${handlerRegex}`);
this.info(`Filtering handlers by regex: ${handlerRegex}`);
}

if (this.processedFlags.path !== undefined) {
this.log(`Filtering file paths by regex: ${pathRegex}`);
this.info(`Filtering file paths by regex: ${pathRegex}`);
}

if (this.processedFlags.fix) {
this.log("Resolving errors if possible.");
this.info("Resolving errors if possible.");
}

if (this.processedFlags.exclusions === undefined) {
this.error("ERROR: No exclusions file provided.");
this.error("No exclusions file provided.");
}

let exclusionsFile: string[];
Expand Down Expand Up @@ -174,43 +174,41 @@ export class CheckPolicy extends BaseCommand<typeof CheckPolicy.flags> {

// route files to their handlers by regex testing their full paths
// synchronize output, exit code, and resolve decision for all handlers
routeToHandlers(file: string, handlerRegex: RegExp) {
const filteredHandlers = policyHandlers.filter(
(handler) => handler.match.test(file) && handlerRegex.test(handler.name),
);

for (const handler of filteredHandlers) {
const result = runWithPerf(handler.name, "handle", () =>
handler.handler(file, CheckPolicy.pathToGitRoot),
);
const resolver = handler.resolver;

if (result === undefined) {
return;
}

if (!this.processedFlags.fix || resolver === undefined) {
return this.exit(1);
}

let output = `${newline}file failed policy check: ${file}${newline}${result}`;

output += `${newline}attempting to resolve: ${file}`;
const resolveResult = runWithPerf(handler.name, "resolve", () =>
resolver(file, CheckPolicy.pathToGitRoot),
);

// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions
if (resolveResult.message) {
output += newline + resolveResult.message;
}

if (!resolveResult.resolved) {
return this.exit(1);
}
routeToHandlers(file: string, handlerRegex: RegExp): void {
policyHandlers
.filter((handler) => handler.match.test(file) && handlerRegex.test(handler.name))
// eslint-disable-next-line unicorn/no-array-for-each
.forEach((handler) => {
const result = runWithPerf(handler.name, "handle", () =>
handler.handler(file, CheckPolicy.pathToGitRoot),
);
if (result !== undefined && result !== "") {
let output = `${newline}file failed policy check: ${file}${newline}${result}`;
const resolver = handler.resolver;
if (this.processedFlags.fix && resolver) {
output += `${newline}attempting to resolve: ${file}`;
const resolveResult = runWithPerf(handler.name, "resolve", () =>
resolver(file, CheckPolicy.pathToGitRoot),
);

this.log(output);
}
if (resolveResult?.message !== undefined) {
output += newline + resolveResult.message;
}

if (!resolveResult.resolved) {
process.exitCode = 1;
}
} else {
process.exitCode = 1;
}

if (process.exitCode === 1) {
this.warning(output);
} else {
this.info(output);
}
}
});
}

logStats() {
Expand All @@ -236,7 +234,7 @@ export class CheckPolicy extends BaseCommand<typeof CheckPolicy.flags> {

CheckPolicy.count++;
if (!exclusions.every((value) => !value.test(line))) {
this.log(`Excluded: ${line}`);
this.verbose(`Excluded: ${line}`);
return;
}

Expand Down

0 comments on commit edb1d84

Please sign in to comment.