-
Notifications
You must be signed in to change notification settings - Fork 334
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
Remove the lua tracer feature flag check from the codeql-action. #1244
Conversation
Always defer to the CLI on the Lua tracer state from now on.
CODEQL_VERSION_LUA_TRACING_GO_WINDOWS_FIXED | ||
))) | ||
) { | ||
extraArgs.push("--internal-use-lua-tracing"); |
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.
Since we are no longer explicitly setting --internal-use-lua-tracing
, does it mean that the modified action would work only with CLI versions that defaults to Lua tracing?
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.
No, the action works with all CLI versions, but on some CLI versions the action will now use the legacy tracer instead.
As both tracers are 100% compatible, this is not an issue.
On later CLI versions, where the Lua tracer has gained additional capabilities, we also use it by default.
This test is broken, as it first sets environment variables, and then immediately unsets it again. This only worked by chance with the legacy tracer, and breaks the Lua tracer.
I suggest we defer merging this to after we've cut the release of the CodeQL Action that'll go into GHES 3.7. |
Now that you mention it, I think we need to do the opposite and ensure this PR makes it into GHES, otherwise C# tracing will be broken on GHES: Without the PR, our logic will detect that we're running on GHES, which disables all feature flags. (I had not thought that through before you mentioned GHES here 😬 ) |
Ah, I wasn't aware we'd deleted the CLR tracer in |
Always defer to the CLI on the Lua tracer state from now on.
This will turn off Lua tracing when combined with a CLI that has Lua tracing disabled by default, but as Lua tracing and legacy tracing on these CLIs are 100% compatible, that does not pose a problem.
Furthermore, the reduction of complexity in our code is worth turning off Lua tracing for those few users.
Querying the feature flag is becoming soon pointless, as dotcom will always return
true
anyways.Merge / deployment checklist