Use cache-cleanup command line option by paldepind · Pull Request #2450 · github/codeql-action · GitHub

Hello :)

It seems that the --mode command line option for codeql database cleanup has been renamed to --cache-cleanup.

When the analyze action is run I see the following in the job log

The --mode option has been renamed to --cache-cleanup. Please update your invocations accordingly. The old option name will be removed in a future version of CodeQL (earliest 2.18.0).

and the documentation for cleanup-level refers to an option that is not present in the linked documentation.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.
Confirm the changelog has been updated if necessary.

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 Features class in the CLI source code, defined in VersionCommand.java that helps us gate against which CLI version includes which commands/options. It looks like this changeover hasn't been added to that class yet though, so we'll want to add it there first. After that, we'll be able to use isSupportedToolsFeature

codeql-action/src/tools-features.ts

Lines 21 to 26 in 7233ec5

    
           export function isSupportedToolsFeature( 
        
             versionInfo: VersionInfo, 
        
             feature: ToolsFeature, 
        
           ): boolean { 
        
             return !!versionInfo.features && versionInfo.features[feature]; 
        
           }

in the Action source code to make sure that when we are running off of older CLI versions, the older option name is being called.

Thanks for the reply @angelapwen 🙂. I'll make the change to VersionCommand.java. And then I guess we'll have to wait for a new release of the CLI before we proceed with this PR?

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 VersionCommand.java (specifically CodeQL v2.17.1 to v2.18.x). Specifically, we would add a constant to the Action for the minimum version supporting the new cache cleanup option, and then pass the new flag in the Action when the CLI version is at least that constant.

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 🤔

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.

Looks great, just one optional comment. Thanks!

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.

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.

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.

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?

Looks good to me @paldepind 😸! Feel free to open an internal issue to help remind us to update the cleanup modes as well!

Use cache-cleanup command line option

e4525ac

The --mode command line option to has been renamed to --cache-cleanup

paldepind requested a review from a team as a code owner August 28, 2024 14:54

paldepind added 2 commits September 2, 2024 08:32

Guard usage of cache-cleanup option by version check

9a16e92

Merge branch 'main' into use-cache-cleanup-flag

878047b

henrymercer approved these changes Sep 2, 2024

View reviewed changes

angelapwen approved these changes Sep 4, 2024

View reviewed changes

paldepind merged commit 4ac5f37 into github:main Sep 4, 2024

paldepind deleted the use-cache-cleanup-flag branch September 10, 2024 06:37

github-actions Bot mentioned this pull request Sep 13, 2024

Merge main into releases/v3 #2478

Merged

8 tasks

dbartol mentioned this pull request Sep 13, 2024

Merge releases/v3 into releases/v2 #2480

Merged

8 tasks

                       "cleanup",
                       databasePath,
-                      `--mode=${cleanupLevel}`,
+                      `${cacheCleanupFlag}=${cleanupLevel}`,

Use cache-cleanup command line option by paldepind · Pull Request #2450 · github/codeql-action · GitHub

Use cache-cleanup command line option - #2450

Use cache-cleanup command line option#2450paldepind merged 3 commits intogithub:maingithub/codeql-action:mainfrom paldepind:use-cache-cleanup-flagpaldepind/codeql-action:use-cache-cleanup-flagCopy head branch name to clipboard

Conversation

paldepind commented Aug 28, 2024

Merge / deployment checklist

Uh oh!

paldepind commented Aug 28, 2024

Uh oh!

angelapwen commented Aug 28, 2024

Uh oh!

paldepind commented Aug 29, 2024

Uh oh!

henrymercer commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paldepind commented Aug 29, 2024

Uh oh!

angelapwen commented Aug 29, 2024

Uh oh!

henrymercer commented Aug 29, 2024

Uh oh!

angelapwen commented Aug 29, 2024

Uh oh!

paldepind commented Sep 2, 2024

Uh oh!

henrymercer left a comment

Choose a reason for hiding this comment

Uh oh!

henrymercer Sep 2, 2024

Choose a reason for hiding this comment

Uh oh!

paldepind Sep 2, 2024

Choose a reason for hiding this comment

Uh oh!

henrymercer Sep 2, 2024

Choose a reason for hiding this comment

Uh oh!

paldepind Sep 2, 2024

Choose a reason for hiding this comment

Uh oh!

paldepind commented Sep 4, 2024

Uh oh!

angelapwen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Use cache-cleanup command line option#2450
paldepind merged 3 commits into
github:mainfrom
paldepind:use-cache-cleanup-flag

henrymercer commented Aug 29, 2024 •
edited
Loading