| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
This PR adds experimental support for excluding generated files (marked with linguist-generated=true in .gitattributes) from CodeQL analysis. The feature is controlled by the ignore_generated_files feature flag and is automatically enabled for Copilot Code Reviews (CCR).
Copilot reviewed 18 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file| File | Description |
|---|---|
| src/git-utils.ts | Adds functions to list tracked files and identify files marked as generated via linguist-generated attribute |
| src/git-utils.test.ts | Adds unit tests for the new git utility functions |
| src/feature-flags.ts | Defines the IgnoreGeneratedFiles feature flag with environment variable CODEQL_ACTION_IGNORE_GENERATED_FILES |
| src/config-utils.ts | Integrates generated file detection into config initialization, adding them to paths-ignore when feature is enabled or in CCR |
| src/actions-util.ts | Adds isCCR() detection function and modifies isDefaultSetup() to exclude CCR scenarios |
| src/actions-util.test.ts | Adds tests for the new workflow detection functions |
| lib/*.js | Auto-generated JavaScript code mirroring TypeScript changes (not reviewed per guidelines) |
Sorry, something went wrong.
| } | ||
|
|
||
| /** Determines whether we are running in CCR. */ | ||
| export function isCCR(): boolean { |
There was a problem hiding this comment.
What about introducing an environment variable we set in CCR, rather than relying on the analysis key?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| export async function getGeneratedFiles( | ||
| workingDirectory: string, | ||
| ): Promise<string[]> { | ||
| const files = await listFiles(workingDirectory); |
There was a problem hiding this comment.
This could potentially be a very large number of files, too many to pass on the command line.
If we're mainly interested in CCR, could we filter down to just the diff here?
Alternatively, we could parse globs from the .gitattributes file rather than finding all files that match. That would be more likely to contain one entry for a large directory rather than potentially hundreds.
Or we could add a limit on the number of files on which we'll run check-attr.
Sorry, something went wrong.
There was a problem hiding this comment.
This could potentially be a very large number of files, too many to pass on the command line.
True. The files could be passed in via stdin as well I think, which might be a more robust solution.
If we're mainly interested in CCR, could we filter down to just the diff here?
Potentially. We should first look at whether there is actually enough of a performance hit for large repos for this to make sense though.
Alternatively, we could parse globs from the .gitattributes file rather than finding all files that match. That would be more likely to contain one entry for a large directory rather than potentially hundreds.
I considered this, but I'd like to avoid it if we can get the information from git itself so we don't have to try and duplicate what it does.
Sorry, something went wrong.
| export async function getGeneratedFiles( | ||
| workingDirectory: string, | ||
| ): Promise<string[]> { |
There was a problem hiding this comment.
Have you measured how long this operation takes overall on a large mono-repo?
Sorry, something went wrong.
There was a problem hiding this comment.
I believe it should be relatively quick, since Git caches this information IIRC. That said, I haven't explicitly tested it on a large repo, but I can do that.
Sorry, something went wrong.
The analysis key already tells us this under normal conditions
There was a problem hiding this comment.
Some comments I'd like to see addressed before we roll this out externally, but looks good to start testing.
Have you tried running this on this repo? We could add a PR check as an integration test. We'd need to drop the CCR-only restriction though.
Sorry, something went wrong.
| } | ||
| } | ||
|
|
||
| // If we are in CCR or the corresponding FF is enabled, try to determine |
There was a problem hiding this comment.
Or just check the FF.
| // If we are in CCR or the corresponding FF is enabled, try to determine | |
| // If we are in CCR and the corresponding FF is enabled, try to determine |
Sorry, something went wrong.
@henrymercer I have opened #3408 to add a PR check for a basic CCR-like setup. I don't think we need to drop the CCR-only restriction for us to be able to test this. |
Sorry, something went wrong.
Sure, I was thinking about testing this out in code security mode on our repo, since we check in generated code. But the CCR PR check works too. |
Sorry, something went wrong.
| // Arbitrarily choose the first language. We could also choose all languages, but that | ||
| // increases the risk of misinterpreting the data. |
There was a problem hiding this comment.
This is no longer arbitrary ;)
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This PR adds experimental support for excluding files that are marked as linguist-generated=true in a .gitattributes file from analysis.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
Products:
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist