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

Introduce a feature-flag to enable/disable lua-based tracing. #1057

Merged
merged 5 commits into from
May 25, 2022

Conversation

criemen
Copy link
Contributor

@criemen criemen commented May 5, 2022

This allows us to gradually roll out (or even roll back)
Lua-based tracing in case problems occur.

This should not be merged before we are sure that 2.9.2 is the right CLI version to target, but otherwise this PR can be reviewed already.

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.

@criemen criemen marked this pull request as ready for review May 5, 2022 10:28
@criemen criemen requested a review from a team as a code owner May 5, 2022 10:28
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good. I think we should add some tests that the appropriate arguments get passed / not passed when we expect. There are some examples of that for ML-powered queries in config-utils.test.ts.

src/codeql.ts Show resolved Hide resolved
@criemen criemen requested a review from henrymercer May 9, 2022 12:03
@criemen criemen force-pushed the criemen/lua-tracing-ff branch from 12c03b4 to 93a3483 Compare May 9, 2022 12:03
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Before we merge, we should check that (a) 2.9.2 is the right CLI version as you mention in the PR description, and (b) that each of the CodeQL bundles containing CLI v2.9.2 have completely rolled out, in order to avoid alert churn from customers using different bundles in their Code Scanning runs. Alternatively we could merge with just (a) and make sure the feature flag is disabled until (b) holds.

@criemen
Copy link
Contributor Author

criemen commented May 10, 2022

Regarding b):
I believe action rolls out new images per repository, so unless they roll back an image mid-rollout, a customer should not see their codeql version jumping back and forth. Even then, this'll likely get you wobbles in alerts due to new/changed queries.
Also, in theory, enabling/disabling the Lua tracer does not change the generated database or the alerts that are being found (but, well, there might be bugs).
Thus, waiting with enabling the feature flag until 2.9.2 is fully rolled out is reasonable, with the exception of testing/internal repositories.
Testing with these internal repositories is the reason I'd like to get this merged as soon as we've settled a).

@henrymercer
Copy link
Contributor

I believe action rolls out new images per repository, so unless they roll back an image mid-rollout, a customer should not see their codeql version jumping back and forth.

If that's the case, that would simplify dealing with these types of PRs. Do you have a source for this you could internally link me to?

criemen added 2 commits May 16, 2022 09:16
This allows us to gradually roll out (or even roll back)
Lua-based tracing in case problems occur.
@criemen criemen force-pushed the criemen/lua-tracing-ff branch from 93a3483 to c9829af Compare May 16, 2022 09:17
@criemen
Copy link
Contributor Author

criemen commented May 16, 2022

Rebased on top of main to address a conflict.
I'd like to merge this now, as 2.9.2 is indeed the right release. Rolling the feature flag out will have to wait once the new CLI is in the toolcache, though.

@criemen criemen force-pushed the criemen/lua-tracing-ff branch from c9829af to 970e087 Compare May 16, 2022 09:40
@criemen criemen marked this pull request as draft May 16, 2022 21:13
@criemen criemen marked this pull request as ready for review May 25, 2022 07:39
@criemen criemen enabled auto-merge May 25, 2022 09:48
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.

2 participants