Fix overzealous warning when PR scanning is not required

github · Jan 13, 2021 · 0853901 · 0853901
1 parent 087e7a3
commit 0853901
Showing 6 changed files with 324 additions and 214 deletions.
diff --git a/lib/actions-util.js b/lib/actions-util.js
diff --git a/lib/actions-util.js.map b/lib/actions-util.js.map
diff --git a/lib/actions-util.test.js b/lib/actions-util.test.js
diff --git a/lib/actions-util.test.js.map b/lib/actions-util.test.js.map
diff --git a/src/actions-util.test.ts b/src/actions-util.test.ts
@@ -5,6 +5,13 @@ import sinon from "sinon";
 import * as actionsutil from "./actions-util";
 import { setupTests } from "./testing-utils";
 
+function errorCodes(
+  actual: actionsutil.CodedError[],
+  expected: actionsutil.CodedError[]
+): [string[], string[]] {
+  return [actual.map(({ code }) => code), expected.map(({ code }) => code)];
+}
+
 setupTests(test);
 
 test("getRef() throws on the empty string", async (t) => {
@@ -83,46 +90,42 @@ test("prepareEnvironment() when a local run", (t) => {
   t.deepEqual(process.env.CODEQL_ACTION_ANALYSIS_KEY, "LOCAL-RUN:UNKNOWN-JOB");
 });
 
-test("validateWorkflow() when on is missing", (t) => {
-  const errors = actionsutil.validateWorkflow({});
-
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.MissingHooks]);
-});
-
 test("validateWorkflow() when on.push is missing", (t) => {
   const errors = actionsutil.validateWorkflow({ on: {} });
 
-  console.log(errors);
-
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.MissingHooks]);
+  t.deepEqual(
+    ...errorCodes(errors, [actionsutil.WorkflowErrors.MissingPushHook])
+  );
 });
 
 test("validateWorkflow() when on.push is an array missing pull_request", (t) => {
   const errors = actionsutil.validateWorkflow({ on: ["push"] });
 
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.MissingPullRequestHook]);
+  t.deepEqual(...errorCodes(errors, []));
 });
 
 test("validateWorkflow() when on.push is an array missing push", (t) => {
   const errors = actionsutil.validateWorkflow({ on: ["pull_request"] });
 
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.MissingPushHook]);
+  t.deepEqual(
+    ...errorCodes(errors, [actionsutil.WorkflowErrors.MissingPushHook])
+  );
 });
 
 test("validateWorkflow() when on.push is valid", (t) => {
   const errors = actionsutil.validateWorkflow({
     on: ["push", "pull_request"],
   });
 
-  t.deepEqual(errors, []);
+  t.deepEqual(...errorCodes(errors, []));
 });
 
 test("validateWorkflow() when on.push is a valid superset", (t) => {
   const errors = actionsutil.validateWorkflow({
     on: ["push", "pull_request", "schedule"],
   });
 
-  t.deepEqual(errors, []);
+  t.deepEqual(...errorCodes(errors, []));
 });
 
 test("validateWorkflow() when on.push should not have a path", (t) => {
@@ -133,39 +136,47 @@ test("validateWorkflow() when on.push should not have a path", (t) => {
     },
   });
 
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.PathsSpecified]);
+  t.deepEqual(
+    ...errorCodes(errors, [actionsutil.WorkflowErrors.PathsSpecified])
+  );
 });
 
 test("validateWorkflow() when on.push is a correct object", (t) => {
   const errors = actionsutil.validateWorkflow({
     on: { push: { branches: ["main"] }, pull_request: { branches: ["main"] } },
   });
 
-  t.deepEqual(errors, []);
+  t.deepEqual(...errorCodes(errors, []));
 });
 
 test("validateWorkflow() when on.pull_requests is a string", (t) => {
   const errors = actionsutil.validateWorkflow({
     on: { push: { branches: ["main"] }, pull_request: { branches: "*" } },
   });
 
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.MismatchedBranches]);
+  t.deepEqual(
+    ...errorCodes(errors, [actionsutil.WorkflowErrors.MismatchedBranches])
+  );
 });
 
 test("validateWorkflow() when on.pull_requests is a string and correct", (t) => {
   const errors = actionsutil.validateWorkflow({
     on: { push: { branches: "*" }, pull_request: { branches: "*" } },
   });
 
-  t.deepEqual(errors, []);
+  t.deepEqual(...errorCodes(errors, []));
 });
 
 test("validateWorkflow() when on.push is correct with empty objects", (t) => {
-  const errors = actionsutil.validateWorkflow({
-    on: { push: undefined, pull_request: undefined },
-  });
+  const errors = actionsutil.validateWorkflow(
+    yaml.safeLoad(`
+on:
+  push:
+  pull_request:
+`)
+  );
 
-  t.deepEqual(errors, []);
+  t.deepEqual(...errorCodes(errors, []));
 });
 
 test("validateWorkflow() when on.push is mismatched", (t) => {
@@ -176,7 +187,9 @@ test("validateWorkflow() when on.push is mismatched", (t) => {
     },
   });
 
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.MismatchedBranches]);
+  t.deepEqual(
+    ...errorCodes(errors, [actionsutil.WorkflowErrors.MismatchedBranches])
+  );
 });
 
 test("validateWorkflow() when on.push is not mismatched", (t) => {
@@ -187,7 +200,7 @@ test("validateWorkflow() when on.push is not mismatched", (t) => {
     },
   });
 
-  t.deepEqual(errors, []);
+  t.deepEqual(...errorCodes(errors, []));
 });
 
 test("validateWorkflow() when on.push is mismatched for pull_request", (t) => {
@@ -198,119 +211,146 @@ test("validateWorkflow() when on.push is mismatched for pull_request", (t) => {
     },
   });
 
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.MismatchedBranches]);
+  t.deepEqual(
+    ...errorCodes(errors, [actionsutil.WorkflowErrors.MismatchedBranches])
+  );
 });
 
 test("validateWorkflow() for a range of malformed workflows", (t) => {
   t.deepEqual(
-    actionsutil.validateWorkflow({
-      on: {
-        push: 1,
-        pull_request: 1,
-      },
-    } as any),
-    []
+    ...errorCodes(
+      actionsutil.validateWorkflow({
+        on: {
+          push: 1,
+          pull_request: 1,
+        },
+      } as any),
+      []
+    )
   );
 
   t.deepEqual(
-    actionsutil.validateWorkflow({
-      on: 1,
-    } as any),
-    [actionsutil.WorkflowErrors.MissingHooks]
+    ...errorCodes(
+      actionsutil.validateWorkflow({
+        on: 1,
+      } as any),
+      [actionsutil.WorkflowErrors.MissingHooks]
+    )
   );
 
   t.deepEqual(
-    actionsutil.validateWorkflow({
-      on: 1,
-      jobs: 1,
-    } as any),
-    [actionsutil.WorkflowErrors.MissingHooks]
+    ...errorCodes(
+      actionsutil.validateWorkflow({
+        on: 1,
+        jobs: 1,
+      } as any),
+      [actionsutil.WorkflowErrors.MissingHooks]
+    )
   );
 
   t.deepEqual(
-    actionsutil.validateWorkflow({
-      on: 1,
-      jobs: [1],
-    } as any),
-    [actionsutil.WorkflowErrors.MissingHooks]
+    ...errorCodes(
+      actionsutil.validateWorkflow({
+        on: 1,
+        jobs: [1],
+      } as any),
+      [actionsutil.WorkflowErrors.MissingHooks]
+    )
   );
 
   t.deepEqual(
-    actionsutil.validateWorkflow({
-      on: 1,
-      jobs: { 1: 1 },
-    } as any),
-    [actionsutil.WorkflowErrors.MissingHooks]
+    ...errorCodes(
+      actionsutil.validateWorkflow({
+        on: 1,
+        jobs: { 1: 1 },
+      } as any),
+      [actionsutil.WorkflowErrors.MissingHooks]
+    )
   );
 
   t.deepEqual(
-    actionsutil.validateWorkflow({
-      on: 1,
-      jobs: { test: 1 },
-    } as any),
-    [actionsutil.WorkflowErrors.MissingHooks]
+    ...errorCodes(
+      actionsutil.validateWorkflow({
+        on: 1,
+        jobs: { test: 1 },
+      } as any),
+      [actionsutil.WorkflowErrors.MissingHooks]
+    )
   );
 
   t.deepEqual(
-    actionsutil.validateWorkflow({
-      on: 1,
-      jobs: { test: [1] },
-    } as any),
-    [actionsutil.WorkflowErrors.MissingHooks]
+    ...errorCodes(
+      actionsutil.validateWorkflow({
+        on: 1,
+        jobs: { test: [1] },
+      } as any),
+      [actionsutil.WorkflowErrors.MissingHooks]
+    )
   );
 
   t.deepEqual(
-    actionsutil.validateWorkflow({
-      on: 1,
-      jobs: { test: { steps: 1 } },
-    } as any),
-    [actionsutil.WorkflowErrors.MissingHooks]
+    ...errorCodes(
+      actionsutil.validateWorkflow({
+        on: 1,
+        jobs: { test: { steps: 1 } },
+      } as any),
+      [actionsutil.WorkflowErrors.MissingHooks]
+    )
   );
 
   t.deepEqual(
-    actionsutil.validateWorkflow({
-      on: 1,
-      jobs: { test: { steps: [{ notrun: "git checkout HEAD^2" }] } },
-    } as any),
-    [actionsutil.WorkflowErrors.MissingHooks]
+    ...errorCodes(
+      actionsutil.validateWorkflow({
+        on: 1,
+        jobs: { test: { steps: [{ notrun: "git checkout HEAD^2" }] } },
+      } as any),
+      [actionsutil.WorkflowErrors.MissingHooks]
+    )
   );
 
   t.deepEqual(
-    actionsutil.validateWorkflow({
-      on: 1,
-      jobs: { test: [undefined] },
-    } as any),
-    [actionsutil.WorkflowErrors.MissingHooks]
+    ...errorCodes(
+      actionsutil.validateWorkflow({
+        on: 1,
+        jobs: { test: [undefined] },
+      } as any),
+      [actionsutil.WorkflowErrors.MissingHooks]
+    )
   );
 
-  t.deepEqual(actionsutil.validateWorkflow(1 as any), [
-    actionsutil.WorkflowErrors.MissingHooks,
-  ]);
+  t.deepEqual(...errorCodes(actionsutil.validateWorkflow(1 as any), []));
 
   t.deepEqual(
-    actionsutil.validateWorkflow({
-      on: {
-        push: {
-          branches: 1,
+    ...errorCodes(
+      actionsutil.validateWorkflow({
+        on: {
+          push: {
+            branches: 1,
+          },
+          pull_request: {
+            branches: 1,
+          },
         },
-        pull_request: {
-          branches: 1,
-        },
-      },
-    } as any),
-    []
+      } as any),
+      []
+    )
   );
 });
 
 test("validateWorkflow() when on.pull_request for every branch but push specifies branches", (t) => {
-  const errors = actionsutil.validateWorkflow({
-    on: {
-      push: { branches: ["main"] },
-      pull_request: null,
-    },
-  });
+  const errors = actionsutil.validateWorkflow(
+    yaml.safeLoad(`
+name: "CodeQL"
+on:
+  push:
+    branches: ["main"]
+  pull_request:
+`)
+  );
 
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.MismatchedBranches]);
+  t.deepEqual(
+    ...errorCodes(errors, [actionsutil.WorkflowErrors.MismatchedBranches])
+  );
 });
 
 test("validateWorkflow() when on.pull_request for wildcard branches", (t) => {
@@ -321,7 +361,7 @@ test("validateWorkflow() when on.pull_request for wildcard branches", (t) => {
     },
   });
 
-  t.deepEqual(errors, []);
+  t.deepEqual(...errorCodes(errors, []));
 });
 
 test("validateWorkflow() when on.pull_request for mismatched wildcard branches", (t) => {
@@ -332,7 +372,9 @@ test("validateWorkflow() when on.pull_request for mismatched wildcard branches",
     },
   });
 
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.MismatchedBranches]);
+  t.deepEqual(
+    ...errorCodes(errors, [actionsutil.WorkflowErrors.MismatchedBranches])
+  );
 });
 
 test("validateWorkflow() when HEAD^2 is checked out", (t) => {
@@ -343,7 +385,9 @@ test("validateWorkflow() when HEAD^2 is checked out", (t) => {
     jobs: { test: { steps: [{ run: "git checkout HEAD^2" }] } },
   });
 
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.CheckoutWrongHead]);
+  t.deepEqual(
+    ...errorCodes(errors, [actionsutil.WorkflowErrors.CheckoutWrongHead])
+  );
 });
 
 test("formatWorkflowErrors() when there is one error", (t) => {
@@ -416,7 +460,7 @@ test("validateWorkflow() when branches contain dots", (t) => {
 `)
   );
 
-  t.deepEqual(errors, []);
+  t.deepEqual(...errorCodes(errors, []));
 });
 
 test("validateWorkflow() when on.push has a trailing comma", (t) => {
@@ -432,7 +476,7 @@ on:
 `)
   );
 
-  t.deepEqual(errors, []);
+  t.deepEqual(...errorCodes(errors, []));
 });
 
 test("validateWorkflow() should only report the current job's CheckoutWrongHead", (t) => {
@@ -461,7 +505,9 @@ jobs:
 `)
   );
 
-  t.deepEqual(errors, [actionsutil.WorkflowErrors.CheckoutWrongHead]);
+  t.deepEqual(
+    ...errorCodes(errors, [actionsutil.WorkflowErrors.CheckoutWrongHead])
+  );
 });
 
 test("validateWorkflow() should not report a different job's CheckoutWrongHead", (t) => {
@@ -490,5 +536,43 @@ jobs:
 `)
   );
 
-  t.deepEqual(errors, []);
+  t.deepEqual(...errorCodes(errors, []));
+});
+
+test("validateWorkflow() when on is missing", (t) => {
+  const errors = actionsutil.validateWorkflow(
+    yaml.safeLoad(`
+name: "CodeQL"
+`)
+  );
+
+  t.deepEqual(...errorCodes(errors, []));
+});
+
+test("validateWorkflow() should not report an error if PRs are totally unconfigured", (t) => {
+  t.deepEqual(
+    ...errorCodes(
+      actionsutil.validateWorkflow(
+        yaml.safeLoad(`
+name: "CodeQL"
+on:
+  push:
+    branches: [master]
+`)
+      ),
+      []
+    )
+  );
+
+  t.deepEqual(
+    ...errorCodes(
+      actionsutil.validateWorkflow(
+        yaml.safeLoad(`
+name: "CodeQL"
+on: ["push"]
+`)
+      ),
+      []
+    )
+  );
 });
diff --git a/src/actions-util.ts b/src/actions-util.ts
@@ -184,7 +184,7 @@ enum MissingTriggers {
   PullRequest = 2,
 }
 
-interface CodedError {
+export interface CodedError {
   message: string;
   code: string;
 }
@@ -236,11 +236,11 @@ export function validateWorkflow(doc: Workflow): CodedError[] {
   let missing = MissingTriggers.None;
 
   if (doc.on === undefined) {
-    missing = MissingTriggers.Push | MissingTriggers.PullRequest;
+    // codeql will scan the default branch
   } else if (typeof doc.on === "string") {
     switch (doc.on) {
       case "push":
-        missing = MissingTriggers.PullRequest;
+        // valid configuration
         break;
       case "pull_request":
         missing = MissingTriggers.Push;
@@ -250,19 +250,22 @@ export function validateWorkflow(doc: Workflow): CodedError[] {
         break;
     }
   } else if (Array.isArray(doc.on)) {
-    if (!doc.on.includes("push")) {
+    const hasPush = doc.on.includes("push");
+    const hasPullRequest = doc.on.includes("pull_request");
+    if (hasPullRequest && !hasPush) {
       missing = missing | MissingTriggers.Push;
     }
-    if (!doc.on.includes("pull_request")) {
-      missing = missing | MissingTriggers.PullRequest;
-    }
   } else if (isObject(doc.on)) {
-    if (!Object.prototype.hasOwnProperty.call(doc.on, "pull_request")) {
-      missing = missing | MissingTriggers.PullRequest;
-    }
-    if (!Object.prototype.hasOwnProperty.call(doc.on, "push")) {
+    const hasPush = Object.prototype.hasOwnProperty.call(doc.on, "push");
+    const hasPullRequest = Object.prototype.hasOwnProperty.call(
+      doc.on,
+      "pull_request"
+    );
+
+    if (!hasPush) {
       missing = missing | MissingTriggers.Push;
-    } else {
+    }
+    if (hasPush && hasPullRequest) {
       const paths = doc.on.push?.paths;
       // if you specify paths or paths-ignore you can end up with commits that have no baseline
       // if they didn't change any files
@@ -276,24 +279,28 @@ export function validateWorkflow(doc: Workflow): CodedError[] {
       }
     }
 
-    const push = branchesToArray(doc.on.push?.branches);
+    // check the user is scanning PRs right now
+    // if not the warning does not apply
+    if (doc.on.pull_request !== undefined) {
+      const push = branchesToArray(doc.on.push?.branches);
 
-    if (push !== "**") {
-      const pull_request = branchesToArray(doc.on.pull_request?.branches);
+      if (push !== "**") {
+        const pull_request = branchesToArray(doc.on.pull_request?.branches);
 
-      if (pull_request !== "**") {
-        const difference = pull_request.filter(
-          (value) => !push.some((o) => patternIsSuperset(o, value))
-        );
-        if (difference.length > 0) {
-          // there are branches in pull_request that may not have a baseline
-          // because we are not building them on push
+        if (pull_request !== "**") {
+          const difference = pull_request.filter(
+            (value) => !push.some((o) => patternIsSuperset(o, value))
+          );
+          if (difference.length > 0) {
+            // there are branches in pull_request that may not have a baseline
+            // because we are not building them on push
+            errors.push(WorkflowErrors.MismatchedBranches);
+          }
+        } else if (push.length > 0) {
+          // push is set up to run on a subset of branches
+          // and you could open a PR against a branch with no baseline
           errors.push(WorkflowErrors.MismatchedBranches);
         }
-      } else if (push.length > 0) {
-        // push is set up to run on a subset of branches
-        // and you could open a PR against a branch with no baseline
-        errors.push(WorkflowErrors.MismatchedBranches);
       }
     }
   } else {