Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid warning on workflow_call triggers #2274

Merged
merged 1 commit into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Note that the only difference between `v2` and `v3` of the CodeQL Action is the
## [UNRELEASED]

- Update default CodeQL bundle version to 2.17.2. [#2270](https://github.com/github/codeql-action/pull/2270)
- Avoid printing out a warning for a missing `on.push` trigger when the CodeQL Action is triggered via a `workflow_call` event. [#2274](https://github.com/github/codeql-action/pull/2274)

## 3.25.3 - 25 Apr 2024

Expand All @@ -30,7 +31,7 @@ No user facing changes.

- The `setup-python-dependencies` input to the `init` Action
- The `CODEQL_ACTION_DISABLE_PYTHON_DEPENDENCY_INSTALLATION` environment variable

We recommend removing any references to these from your workflows. For more information, see the release notes for CodeQL Action v3.23.0 and v2.23.0.
- Automatically overwrite an existing database if found on the filesystem. [#2229](https://github.com/github/codeql-action/pull/2229)
- Bump the minimum CodeQL bundle version to 2.12.6. [#2232](https://github.com/github/codeql-action/pull/2232)
Expand Down
47 changes: 20 additions & 27 deletions lib/workflow.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/workflow.js.map

Large diffs are not rendered by default.

23 changes: 23 additions & 0 deletions lib/workflow.test.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/workflow.test.js.map

Large diffs are not rendered by default.

38 changes: 38 additions & 0 deletions src/workflow.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,44 @@ test("getWorkflowErrors() should not report an error if PRs are totally unconfig
);
});

test("getWorkflowErrors() should not report a warning if there is a workflow_call trigger", async (t) => {
const errors = await getWorkflowErrors(
yaml.load(`
name: "CodeQL"
on:
workflow_call:
`) as Workflow,
await getCodeQLForTesting(),
);

t.deepEqual(...errorCodes(errors, []));
});

test("getWorkflowErrors() should not report a warning if there is a workflow_call trigger as a string", async (t) => {
const errors = await getWorkflowErrors(
yaml.load(`
name: "CodeQL"
on: workflow_call
`) as Workflow,
await getCodeQLForTesting(),
);

t.deepEqual(...errorCodes(errors, []));
});

test("getWorkflowErrors() should not report a warning if there is a workflow_call trigger as an array", async (t) => {
const errors = await getWorkflowErrors(
yaml.load(`
name: "CodeQL"
on:
- workflow_call
`) as Workflow,
await getCodeQLForTesting(),
);

t.deepEqual(...errorCodes(errors, []));
});

test("getCategoryInputOrThrow returns category for simple workflow with category", (t) => {
process.env["GITHUB_REPOSITORY"] = "github/codeql-action-fake-repository";
t.is(
Expand Down
52 changes: 23 additions & 29 deletions src/workflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@ export interface Workflow {
on?: string | string[] | WorkflowTriggers;
}

function isObject(o: unknown): o is object {
return o !== null && typeof o === "object";
}

const GLOB_PATTERN = new RegExp("(\\*\\*?)");

function escapeRegExp(string) {
Expand Down Expand Up @@ -193,37 +189,35 @@ export async function getWorkflowErrors(
}
}

let missingPush = false;
// If there is no push trigger, we will not be able to analyze the default branch.
// So add a warning to the user to add a push trigger.
// If there is a workflow_call trigger, we don't need a push trigger since we assume
// that the workflow_call trigger is called from a workflow that has a push trigger.
const hasPushTrigger = hasWorkflowTrigger("push", doc);
const hasPullRequestTrigger = hasWorkflowTrigger("pull_request", doc);
const hasWorkflowCallTrigger = hasWorkflowTrigger("workflow_call", doc);

if (doc.on === undefined) {
// this is not a valid config
} else if (typeof doc.on === "string") {
if (doc.on === "pull_request") {
missingPush = true;
}
} else if (Array.isArray(doc.on)) {
const hasPush = doc.on.includes("push");
const hasPullRequest = doc.on.includes("pull_request");
if (hasPullRequest && !hasPush) {
missingPush = true;
}
} else if (isObject(doc.on)) {
const hasPush = Object.prototype.hasOwnProperty.call(doc.on, "push");
const hasPullRequest = Object.prototype.hasOwnProperty.call(
doc.on,
"pull_request",
);
if (hasPullRequestTrigger && !hasPushTrigger && !hasWorkflowCallTrigger) {
errors.push(WorkflowErrors.MissingPushHook);
}

if (!hasPush && hasPullRequest) {
missingPush = true;
}
return errors;
}

function hasWorkflowTrigger(triggerName: string, doc: Workflow): boolean {
if (!doc.on) {
return false;
}

if (missingPush) {
errors.push(WorkflowErrors.MissingPushHook);
if (typeof doc.on === "string") {
return doc.on === triggerName;
}

return errors;
if (Array.isArray(doc.on)) {
return doc.on.includes(triggerName);
}

return Object.prototype.hasOwnProperty.call(doc.on, triggerName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify this:

Suggested change
return Object.prototype.hasOwnProperty.call(doc.on, triggerName);
return Object.hasOwn(doc.on, triggerName);

Copy link
Contributor Author

@aeisenberg aeisenberg May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I didn't know about this function.

It's only available on node 16.9 and later. Are we sure that all supported GHES runners use that version or later?

Also, we need to update our tsconfig, or else we ge tlinter errors. So, I'm going to hold off for now and we can make the tsconfig changes later.

}

export async function validateWorkflow(
Expand Down
Loading