-
Notifications
You must be signed in to change notification settings - Fork 310
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
Use cache-cleanup command line option #2450
Conversation
The --mode command line option to has been renamed to --cache-cleanup
CI is failing, I think, because some of the tests use an older version of the CodeQL CLI. So maybe this change can't be made after all :/ |
I see, it looks like this was deprecated as of CLI v 2.17.1 (2024-04-24). Thanks for looking out in our logs! We do have a codeql-action/src/tools-features.ts Lines 21 to 26 in 7233ec5
|
Thanks for the reply @angelapwen 🙂. I'll make the change to |
I think that would work, but it would mean we'd still get the warning for older CLIs. So it might be worth doing the version check the old way in this case, so we won't get warnings for older CLIs that wouldn't contain the feature flag in What do you both think? |
Given that the CLI is already released without the feature exposed and that checking the version is rather simple, it seems like an easy way to correctly handle to the change. But I don't have a strong preference and I can also see the argument that the feature flags is a cleaner and more decoupled way to handle changes in the CLI. |
Yeah, that would work as well! I had thought we wanted to deprecate the 'old' way where we specify the minimum version eventually, and thought emitting this warning was ~alright for old CLIs given we're still supporting it for now. Though this case might be a good example of why we shouldn't deprecate the minimum version way completely, given the likelihood that CLI changes affecting the Action may be merged without the feature flag command 🤔 |
Agreed! IIRC, we don't use much code for the mechanism of comparing version numbers, so I'd be in favour of keeping it around. |
Okay! I'm sold 😸 |
I've updated the PR with a version check that guards usage of the new CLI option name. CI seems to be happy now 😄 I tried following the existing pattern for version checks, but let me now if something is not looking quite right. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just one optional comment. Thanks!
const codeqlArgs = [ | ||
"database", | ||
"cleanup", | ||
databasePath, | ||
`--mode=${cleanupLevel}`, | ||
`${cacheCleanupFlag}=${cleanupLevel}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleanup modes have also been renamed as of 2.17.1, though the old ones still work. Do you think we should use the new cleanup mode names? It's slightly more obvious what they do, though this will probably not impact many users.
--cache-cleanup=<mode> Select how aggressively to trim the cache. Choices
include:
clear: Remove the entire cache, trimming down to
the state of a freshly extracted dataset
trim (default): Trim everything except explicitly
"cached" predicates.
fit: Simply make sure the defined size limits for
the disk cache are observed, deleting as many
intermediates as necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. That was done earlier in 2.15.1
.
I think it makes sense to use the new one. Especially in the documentation where "brutal" is mentioned but does not show up in the linked docs.
From a quick look it seems that the only mode that's directly mentioned in code is "brutal". I can add a check and use the new one for that as well? Should I create a new constant or just reuse the one I've already added? Is the "brutal"
in action.yml strictly documentation? Because it will not be possible to change that one conditionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah great point. As you say we'll have to keep the default value in action.yml
as brutal
until we deprecate all CLIs before 2.15.1.
With regard to the code, actionsUtil.getOptionalInput("cleanup-level")
will be brutal
unless someone has explicitly specified an empty input like cleanup-level: null
. This is pretty unlikely. So changing the code would affect a very small number of users, possibly zero.
I think given this we should probably merge this as is, and add a work item to update the constant in action.yml
in about 4 months when we will have deprecated all CLIs before 2.15.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds very reasonable to me. The other option would be to update the default and map it back to "brutal"
for old CLIs. But given that there is a 4 month path to deprecation that seems not worth it.
I think this is ready to merge 😄. Does this look good to you @angelapwen? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me @paldepind 😸! Feel free to open an internal issue to help remind us to update the cleanup modes as well!
Hello :)
It seems that the
--mode
command line option forcodeql database cleanup
has been renamed to--cache-cleanup
.When the analyze action is run I see the following in the job log
and the documentation for
cleanup-level
refers to an option that is not present in the linked documentation.Merge / deployment checklist