Skip to content

Support ignoring generated files #3318

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.

Sign up for GitHub

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

Open
mbg wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Support ignoring generated files #3318

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1512f40
Add function to query `git` for all generated files
mbg Nov 19, 2025
3eaf000
Add `isCCR` helper, and update `isDefaultSetup`
mbg Nov 19, 2025
846f859
Add `IgnoreGeneratedFiles` FF
mbg Nov 19, 2025
b4db382
Add generated files to `paths-ignore`, if FF is enabled
mbg Nov 19, 2025
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
Prev Previous commit
Next Next commit
Add isCCR helper, and update isDefaultSetup
  • Loading branch information
@mbg
mbg committed Nov 19, 2025
commit 3eaf00092ba5fd106e6444a9fb14fc4795bbb2c1
25 changes: 25 additions & 0 deletions src/actions-util.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import {
fixCodeQualityCategory,
getPullRequestBranches,
isAnalyzingPullRequest,
isCCR,
isDefaultSetup,
isDynamicWorkflow,
} from "./actions-util";
import { computeAutomationID } from "./api-client";
import { EnvVar } from "./environment";
Expand Down Expand Up @@ -246,3 +249,25 @@ test("fixCodeQualityCategory", (t) => {
},
);
});

test("isDynamicWorkflow() returns true if event name is `dynamic`", (t) => {
process.env.GITHUB_EVENT_NAME = "dynamic";
t.assert(isDynamicWorkflow());
process.env.GITHUB_EVENT_NAME = "push";
t.false(isDynamicWorkflow());
Comment on lines +254 to +257
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

This test directly modifies process.env without proper cleanup, which could cause test pollution. Consider using withMockedEnv (defined at line 30) to ensure environment variables are restored after the test, similar to how it's done in other tests in this file (e.g., lines 206-251).

Suggested change
process.env.GITHUB_EVENT_NAME = "dynamic";
t.assert(isDynamicWorkflow());
process.env.GITHUB_EVENT_NAME = "push";
t.false(isDynamicWorkflow());
withMockedEnv(
{ GITHUB_EVENT_NAME: "dynamic" },
() => {
t.assert(isDynamicWorkflow());
},
);
withMockedEnv(
{ GITHUB_EVENT_NAME: "push" },
() => {
t.false(isDynamicWorkflow());
},
);

Copilot uses AI. Check for mistakes.
});

test("isCCR() returns true when expected", (t) => {
process.env.GITHUB_EVENT_NAME = "dynamic";
process.env.CODEQL_ACTION_ANALYSIS_KEY =
"dynamic/copilot-pull-request-reviewer";
t.assert(isCCR());
t.false(isDefaultSetup());
});

test("isDefaultSetup() returns true when expected", (t) => {
process.env.GITHUB_EVENT_NAME = "dynamic";
process.env.CODEQL_ACTION_ANALYSIS_KEY = "dynamic/github-code-scanning";
t.assert(isDefaultSetup());
t.false(isCCR());
Comment on lines +254 to +272
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

This test directly modifies process.env without proper cleanup, which could cause test pollution. Consider using withMockedEnv (defined at line 30) to ensure environment variables are restored after the test, similar to how it's done in other tests in this file (e.g., lines 206-251).

Suggested change
process.env.GITHUB_EVENT_NAME = "dynamic";
t.assert(isDynamicWorkflow());
process.env.GITHUB_EVENT_NAME = "push";
t.false(isDynamicWorkflow());
});
test("isCCR() returns true when expected", (t) => {
process.env.GITHUB_EVENT_NAME = "dynamic";
process.env.CODEQL_ACTION_ANALYSIS_KEY =
"dynamic/copilot-pull-request-reviewer";
t.assert(isCCR());
t.false(isDefaultSetup());
});
test("isDefaultSetup() returns true when expected", (t) => {
process.env.GITHUB_EVENT_NAME = "dynamic";
process.env.CODEQL_ACTION_ANALYSIS_KEY = "dynamic/github-code-scanning";
t.assert(isDefaultSetup());
t.false(isCCR());
withMockedEnv(
{ GITHUB_EVENT_NAME: "dynamic" },
() => {
t.assert(isDynamicWorkflow());
process.env.GITHUB_EVENT_NAME = "push";
t.false(isDynamicWorkflow());
},
);
});
test("isCCR() returns true when expected", (t) => {
withMockedEnv(
{
GITHUB_EVENT_NAME: "dynamic",
CODEQL_ACTION_ANALYSIS_KEY: "dynamic/copilot-pull-request-reviewer",
},
() => {
t.assert(isCCR());
t.false(isDefaultSetup());
},
);
});
test("isDefaultSetup() returns true when expected", (t) => {
withMockedEnv(
{
GITHUB_EVENT_NAME: "dynamic",
CODEQL_ACTION_ANALYSIS_KEY: "dynamic/github-code-scanning",
},
() => {
t.assert(isDefaultSetup());
t.false(isCCR());
},
);

Copilot uses AI. Check for mistakes.
Comment on lines +254 to +272
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

This test directly modifies process.env without proper cleanup, which could cause test pollution. Consider using withMockedEnv (defined at line 30) to ensure environment variables are restored after the test, similar to how it's done in other tests in this file (e.g., lines 206-251).

Suggested change
process.env.GITHUB_EVENT_NAME = "dynamic";
t.assert(isDynamicWorkflow());
process.env.GITHUB_EVENT_NAME = "push";
t.false(isDynamicWorkflow());
});
test("isCCR() returns true when expected", (t) => {
process.env.GITHUB_EVENT_NAME = "dynamic";
process.env.CODEQL_ACTION_ANALYSIS_KEY =
"dynamic/copilot-pull-request-reviewer";
t.assert(isCCR());
t.false(isDefaultSetup());
});
test("isDefaultSetup() returns true when expected", (t) => {
process.env.GITHUB_EVENT_NAME = "dynamic";
process.env.CODEQL_ACTION_ANALYSIS_KEY = "dynamic/github-code-scanning";
t.assert(isDefaultSetup());
t.false(isCCR());
withMockedEnv(
{ GITHUB_EVENT_NAME: "dynamic" },
() => {
t.assert(isDynamicWorkflow());
},
);
withMockedEnv(
{ GITHUB_EVENT_NAME: "push" },
() => {
t.false(isDynamicWorkflow());
},
);
});
test("isCCR() returns true when expected", (t) => {
withMockedEnv(
{
GITHUB_EVENT_NAME: "dynamic",
CODEQL_ACTION_ANALYSIS_KEY: "dynamic/copilot-pull-request-reviewer",
},
() => {
t.assert(isCCR());
t.false(isDefaultSetup());
},
);
});
test("isDefaultSetup() returns true when expected", (t) => {
withMockedEnv(
{
GITHUB_EVENT_NAME: "dynamic",
CODEQL_ACTION_ANALYSIS_KEY: "dynamic/github-code-scanning",
},
() => {
t.assert(isDefaultSetup());
t.false(isCCR());
},
);

Copilot uses AI. Check for mistakes.
});
13 changes: 12 additions & 1 deletion src/actions-util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,18 @@ export function isDynamicWorkflow(): boolean {

/** Determines whether we are running in default setup. */
export function isDefaultSetup(): boolean {
return isDynamicWorkflow();
return isDynamicWorkflow() && !isCCR();
}

/** Determines whether we are running in CCR. */
export function isCCR(): boolean {
Copy link
Contributor

@henrymercer henrymercer Nov 20, 2025

Choose a reason for hiding this comment

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

What about introducing an environment variable we set in CCR, rather than relying on the analysis key?

Copy link
Member Author

@mbg mbg Nov 26, 2025

Choose a reason for hiding this comment

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

As discussed elsewhere, that is sort of what we are doing here already (and we decide on whether are in CCR in other places in the same way currently).

A better solution would be to add a new analysis kind (or similar) for CCR, but that would be more work and currently the same as code-quality in essentially everything but name.

I'd suggest we should stick with this for the moment (since we also use the same approach elsewhere) and look at improving it longer-term.

return (
(isDynamicWorkflow() &&
process.env["CODEQL_ACTION_ANALYSIS_KEY"]?.startsWith(
"dynamic/copilot-pull-request-reviewer",
)) ||
false
);
Comment on lines +262 to +268
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The logic can be simplified. The || false is needed to convert undefined from optional chaining to false, but the outer parentheses are unnecessary. Consider: return isDynamicWorkflow() && (process.env["CODEQL_ACTION_ANALYSIS_KEY"]?.startsWith("dynamic/copilot-pull-request-reviewer") ?? false); This is clearer and uses the nullish coalescing operator which is more explicit about handling undefined.

Suggested change
return (
(isDynamicWorkflow() &&
process.env["CODEQL_ACTION_ANALYSIS_KEY"]?.startsWith(
"dynamic/copilot-pull-request-reviewer",
)) ||
false
);
return isDynamicWorkflow() &&
(process.env["CODEQL_ACTION_ANALYSIS_KEY"]?.startsWith("dynamic/copilot-pull-request-reviewer") ?? false);

Copilot uses AI. Check for mistakes.
}

export function prettyPrintInvocation(cmd: string, args: string[]): string {
Expand Down