Fix tools: linked log message - #3139
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an incorrect CLI version number being displayed in log messages when using tools: linked or tools: latest. The fix involves moving the logging statements to occur after the actual CLI version has been determined from the defaults.
- Move the forceShippedTools logic and logging after CLI version assignment
- Ensure the log message displays the correct version from defaults instead of the parameter value
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/setup-codeql.ts | Moved forceShippedTools logic after CLI version assignment to fix log message |
| lib/*.js | Generated JavaScript files reflecting the TypeScript changes |
Sorry, something went wrong.
mbg
left a comment
There was a problem hiding this comment.
Makes sense - a couple of thoughts on what's going on here, but neither is blocking.
Sorry, something went wrong.
| @@ -344,9 +319,33 @@ export async function getCodeQLSource( | |||
| toolsInput = await getNightlyToolsUrl(logger); | |||
There was a problem hiding this comment.
Nit: Not something introduced by this PR, but I am not super keen on this rewriting of toolsInput from an alias to a URL. It makes it harder than necessary to follow what this function does. I think ideally this would be a function resolveToolsAlias that always returns a URL by either resolving the alias or returning the URL unchanged.
Sorry, something went wrong.
| const forceShippedTools = | ||
| toolsInput && CODEQL_BUNDLE_VERSION_ALIAS.includes(toolsInput); | ||
|
|
||
| if (forceShippedTools) { | ||
| cliVersion = defaults.cliVersion; | ||
| tagName = defaults.bundleVersion; | ||
|
|
||
| logger.info( | ||
| `'tools: ${toolsInput}' was requested, so using CodeQL version ${cliVersion}, the version shipped with the Action.`, | ||
| ); | ||
|
|
||
| if (toolsInput === "latest") { | ||
| logger.warning( | ||
| "`tools: latest` has been renamed to `tools: linked`, but the old name is still supported. No action is required.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
As discussed elsewhere, it was confusing for review purposes that this moved down here, which is because cliVersion was already defined here. I'd probably change this back and reference defaults.cliVersion in the log message, or import them differently. No strong feelings about this though.
Sorry, something went wrong.
The CLI version number was wrong.
Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist