-
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
Add option tools: linked
for tools
input of init action.
#2281
Conversation
a294209
to
e4ee1da
Compare
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.
Very nice! Could you update the docs for tools
in init/action.yml
too?
src/setup-codeql.ts
Outdated
logger.info( | ||
`Using CodeQL CLI version ${source.toolsVersion} from ${source.sourceType}.`, | ||
); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If latest
is deprecated, we should also create a warning if we see it.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -6,7 +6,7 @@ Note that the only difference between `v2` and `v3` of the CodeQL Action is the | |||
|
|||
## [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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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
|
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.
Nice! Just a couple of final comments.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.info("Using CodeQL CLI from local path $path"); | |
logger.info(`Using CodeQL CLI from local path ${toolsInput}`); |
src/setup-codeql.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -452,12 +474,16 @@ export async function getCodeQLSource( | |||
url = await getCodeQLBundleDownloadURL(tagName!, apiDetails, logger); | |||
} | |||
|
|||
const toolsVersion = cliVersion ?? humanReadableVersion; | |||
logger.info( | |||
`Using CodeQL CLI version ${toolsVersion} downloaded from ${url}.`, |
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.
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 <[email protected]>
See [1]. [1]: github/codeql-action#2281 Signed-off-by: Sebastian Schuberth <[email protected]>
Description
The
init
action supports an inputtools
, which can get a range of values, including a special valuelatest
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 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.ts
Still outstanding
tools
field in theinit
action #1327Merge / deployment checklist