Persist CodeQL version output to file rather than environment - #4081
Conversation
There was a problem hiding this comment.
Pull request overview
Moves persisted CodeQL CLI version metadata from an environment variable to a temporary JSON file.
Changes:
- Writes and reads version metadata from
$CODEQL_ACTION_TEMP/version.json. - Removes the obsolete environment variable.
- Updates persistence tests.
Show a summary per file
| File | Description |
|---|---|
src/util.ts |
Implements file-based version caching. |
src/util.test.ts |
Updates cache-reading tests. |
src/environment.ts |
Removes the old cache environment variable. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
This is particularly important for the first time that `getCachedCodeQlVersion` is invoked, as this cache file will not yet exist.
a1f3399 to
9183a7b
Compare
There was a problem hiding this comment.
Thanks for picking this back up! I have added a few inline (mostly style) comments.
Some high-level points:
- Also added as an in-line comment, but do you plan to have a separate file for each command? If so, why? On the same topic, if not, then should the file be more structured to allow for relevant metadata and other command output down the line?
- Have you had any more thoughts about either checking that the cached file is intended for the current analysis (e.g. by checking the generated
jobUuid) or deleting cache files on "startup"? - Add a unit test for the case where the file cannot be read (e.g. because it doesn't exist).
- The linter CI is failing. Remember to run
npm lintlocally before committing / pushing changes.
Much more minor, but I spotted this in the PR description:
This is fine, for now, but, as we begin to cache more of the CLI, we should persist that output to a file instead.
The reasoning here ("as we begin to cache more of the CLI") doesn't really justify the conclusion ("we should persist that output to a file instead). Could you edit that to actually highlight why files are preferable to environment variables?
Co-authored-by: Michael B. Gale <mbg@github.com>
This brings them out of the crowded all-purpose `util.ts` and into `cli/output-cache.ts` where they are exclusively used.
139c544 to
11569df
Compare
b11737e to
40f80a8
Compare
This will ensure it works nicely with `JSON.stringify`. And, then we can validate the type before marshaling.
mbg
left a comment
There was a problem hiding this comment.
LGTM, aside from addressing the minor point about the logging the error and resolving the merge conflict.
Currently, the output of
codeql versionis cached and persisted between steps as an environment variable. This is fine, for now, but, as we begin to cache more of the CLI, we should persist that output to a file instead because:execve(2)).To that end, and as a precaution, this PR replaces
$CODEQL_ACTION_CLI_VERSION_INFOwith$CODEQL_ACTION_TEMP/version.json, a file-backed cache forcodeql version.Changes
util.ts(which used environment variables) into a new modulesrc/cli/output-cache.ts, which now uses a temporary on-disk file (codeql-action-command-cache.json) for cross-step persistence. This includes new functions for caching, retrieving, and resetting the cached version, as well as improved validation of persisted data.VersionInfotype definition fromcodeql.tsto a new shared filesrc/cli/types.ts, and updated all references to use the new location. This centralizes type definitions to avoid cyclic dependencies.CODEQL_VERSION_INFOenvironment variable fromEnvVarinenvironment.ts.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).pr-checks).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