| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Very nice! Could you update the docs for tools in init/action.yml too?
Sorry, something went wrong.
| 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:
Sorry, something went wrong.
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.
Sorry, something went wrong.
| `"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:
Sorry, something went wrong.
There was a problem hiding this comment.
If latest is deprecated, we should also create a warning if we see it.
Sorry, something went wrong.
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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
| ## [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) |
Sorry, something went wrong.
|
Once this is done, we will also need to update the documentation in docs-internal. Though, I took a quick look and I can't find any reference to the tools input. Is this even documented there? The init action file contains a link to some documentation, but that page doesn't mention tools anywhere. 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 docs-internal with a full description of tools. |
Sorry, something went wrong.
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 tools, without being subjected to the review cycle/scope of this PR. Any thoughts? |
Sorry, something went wrong.
|
It's not currently documented, so I'm happy with a separate PR if you prefer! |
Sorry, something went wrong.
|
Apologies, I had to force push a rebase because of some issues with the CHANGELOG. What has changed in 5a08657:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Nice! Just a couple of final comments.
Sorry, something went wrong.
| !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}`); |
Sorry, something went wrong.
| 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".
Sorry, something went wrong.
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 on the subject.
Any thoughts?
Sorry, something went wrong.
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.".
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
|
|
||
| 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.
Sorry, something went wrong.
|
Hi @henrymercer, this is now ready for another round of reviews. |
Sorry, something went wrong.
Also ensure that option latest remains compatible, and add tests for the two options.
…gs from some locations
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>
| Back | FazBrowse Home | New Git URL |
Description
The init action supports an input tools, which can get a range of values, including a special value latest that 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: linked that is functionally equivalent to tools: latest, but is supposed to guide user expectations as to the version that's actually used.
Review Guidance
Still outstanding
Merge / deployment checklist