Skip to content
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

Merged
merged 2 commits into from Sep 13, 2022

Conversation

criemen
Copy link
Contributor

@criemen criemen commented Sep 13, 2022

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

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

Always defer to the CLI on the Lua tracer state from now on.
@criemen criemen requested a review from a team as a code owner September 13, 2022 11:25
CODEQL_VERSION_LUA_TRACING_GO_WINDOWS_FIXED
)))
) {
extraArgs.push("--internal-use-lua-tracing");
Copy link
Contributor

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?

Copy link
Contributor Author

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.
@henrymercer
Copy link
Contributor

I suggest we defer merging this to after we've cut the release of the CodeQL Action that'll go into GHES 3.7.

@criemen
Copy link
Contributor Author

criemen commented Sep 13, 2022

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.
Therefore, we will pass in --no-internal-use-lua-tracing.
On rc/3.7, unlike main, that flag still has an effect, and disable Lua tracing.
However, we have already deleted the CLR tracer, so the legacy tracer for C# is in a bad shape. Furthermore, we really want to ship the C# improvements also to our GHES customers.

(I had not thought that through before you mentioned GHES here 😬 )

@henrymercer
Copy link
Contributor

Ah, I wasn't aware we'd deleted the CLR tracer in rc/3.7. In that case, it sounds like we need to get this in. AIUI 2.10.5 enables the Lua tracer by default, so the behaviour here of dropping the --internal-use-lua-tracing flag will be to use Lua tracing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants