Add option tools: linked for tools input of init action.
#2281
Conversation
a294209 to
e4ee1da
Compare
src/setup-codeql.ts
Outdated
| logger.info( | ||
| `Using CodeQL CLI version ${source.toolsVersion} from ${source.sourceType}.`, | ||
| ); |
There was a problem hiding this comment.
There are a few routes where we don't actually know the CLI version at this point, for example if the tools were specified using a local path or if an old bundle URL was specified like https://github.com/github/codeql-action/releases/download/codeql-bundle-20230317/codeql-bundle-linux64.tar.gz. In both cases toolsVersion is a bit opaque (for the first case it will be local, for the second it will be 0.0.0-20230317), and I think we want to avoid showing this to users to avoid confusion.
What do you think about pushing this message into getCodeQLSource and returning more specific messages in each case, for instance:
- Local could say something like "Using CodeQL CLI from local path $path"
- Old bundle URL could say something like "Using CodeQL CLI from URL $url"
- Cases where we know the CLI version could say what we have here "Using CodeQL CLI version $version from $source."
There was a problem hiding this comment.
Ah, I see - I wasn't aware that we actually can be in a position where we don't have all the information.
I was hoping that by putting the logging into the getCodeQLBundle, after getCodeQLSource has returned, that I could get away with logging at just one point, and at a time where the dust had settled and a decision on what/where to get codeql from had been made, instead of trying to chase the various return paths in getCodeQLSource.
I will revise the approach.
src/setup-codeql.ts
Outdated
| `"tools: linked" or "tools: latest" was requested. The version shipped with the Action is ` + | ||
| `${defaultCliVersion.cliVersion}.`, |
There was a problem hiding this comment.
Couple of suggestions:
- Say something like "Overriding the version of the CodeQL tools by , the version shipped with the Action"
- Use
tools: ${toolsInput}to simplify the message
There was a problem hiding this comment.
If latest is deprecated, we should also create a warning if we see it.
There was a problem hiding this comment.
I wanted to ask actually about the deprecation process for the tool. I'll take we're happy to just log.warn?
There was a problem hiding this comment.
My 2¢: it's fine for customers who have already set up tools: latest to continue using it. I don't think it's worth removing support for this for the forseeable future.
CHANGELOG.md
Outdated
| ## [UNRELEASED] | ||
|
|
||
| No user facing changes. | ||
| - Add `tools: linked` option for input of `init` action. [#2281](https://github.com/github/codeql-action/pull/2281) |
There was a problem hiding this comment.
| - Add `tools: linked` option for input of `init` action. [#2281](https://github.com/github/codeql-action/pull/2281) | |
| - The `tools: latest` input to the `init` Action has been renamed to `tools: linked`. This option specifies that the Action should use the tools shipped at the same time as the Action. The old name will continue to work for backwards compatibility, but we recommend that new workflows use the new name. [#2281](https://github.com/github/codeql-action/pull/2281) |
|
Once this is done, we will also need to update the documentation in https://github.com/github/codeql-action/blob/main/init/action.yml#L17 For now, maybe we just need to update this input description with the new change and remove the link. Then later, we should update the |
Yes, this is my plan before the end of the ticket as a whole. I was originally thinking of doing this as part of a seperate PR that's designed to specifically address the issue in #1327. Will raise this soon - unless there's a preference to address this in this PR? I was thinking that a separate PR will be cleaner, and can afford being more focused on drafting a good general documentation for |
|
It's not currently documented, so I'm happy with a separate PR if you prefer! |
e4ee1da to
5a08657
Compare
|
Apologies, I had to force push a
|
src/setup-codeql.ts
Outdated
| !CODEQL_BUNDLE_VERSION_ALIAS.includes(toolsInput) && | ||
| !toolsInput.startsWith("http") | ||
| ) { | ||
| logger.info("Using CodeQL CLI from local path $path"); |
There was a problem hiding this comment.
| logger.info("Using CodeQL CLI from local path $path"); | |
| logger.info(`Using CodeQL CLI from local path ${toolsInput}`); |
| if (toolsInput === "latest") { | ||
| logger.warning( | ||
| "The 'latest' alias for the CodeQL tools has been deprecated. Please use 'linked' instead.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
See my previous comment — I'm not sure it's worth creating work for customers to change their workflows to use "linked".
There was a problem hiding this comment.
Hmm, thinking out loud here:
Does it make sense to keep the deprecation warning, but soften the wording a bit, to indicate that the preferred option going forward is linked, but that latest remains compatible?
I'm thinking that if we don't have this bit of documentation (via the warning), from the user's perspective there's a chance of looking at the other bits of documentation we have, which indicate linked to be used, compare with their action containing latest, and wonder what happened to latest and why there's a discrepancy between our documentation and their current version, without any warning.
I believe that raising a warning here at least alerts a user to the fact that this has changed, which is better user experience than shadow changes, but I don't have any strong feelings 67E6 on the subject.
Any thoughts?
There was a problem hiding this comment.
That's a good point. We could have an info message like "tools: latest has been renamed to tools: linked, but the previous name is still supported for backwards compatibility. No action is necessary.".
src/setup-codeql.ts
Outdated
| const version = cliVersion ?? humanReadableVersion; | ||
| logger.info( | ||
| `Using CodeQL CLI version ${version} from toolcache at ${codeqlFolder}`, | ||
| ); |
There was a problem hiding this comment.
humanReadableVersion can still contain something like 0.0.0-<bundle version number> (it could probably have a better name as it's mainly for telemetry and debug logs). How about only printing the version ${cliVersion} bit if cliVersion is defined?
src/setup-codeql.ts
Outdated
|
|
||
| const toolsVersion = cliVersion ?? humanReadableVersion; | ||
| logger.info( | ||
| `Using CodeQL CLI version ${toolsVersion} downloaded from ${url}.`, |
There was a problem hiding this comment.
Likewise here. Also we might want to rephrase this slightly so it doesn't sound like the CLI has already been downloaded.
|
Hi @henrymercer, this is now ready for another round of reviews. |
Also ensure that option latest remains compatible, and add tests for the two options.
…gs from some locations
b24e695 to
df4819e
Compare
See [1]. [1]: github/codeql-action#2281 Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
See [1]. [1]: github/codeql-action#2281 Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
Description
The
initaction supports an inputtools, which can get a range of values, including a special valuelatestthat is supposed to force the use of the bundled version of CodeQL for the action.The use of the word
latest, however, might hint to users that this is really fetching the newest version of CodeQL, rather than the one associated with the action when it was released.This PR is adding an option
tools: linkedthat is functionally equivalent totools: latest, but is supposed to guide user expectations as to the version that's actually used.Review Guidance
src/setup-codeql.ts.src/setup-codeql.test.tsStill outstanding
toolsfield in theinitaction #1327Merge / deployment checklist