FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Introduce a feature-flag to enable/disable lua-based tracing. by criemen · Pull Request #1057 · github/codeql-action · GitHub

/ codeql-action Public

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

Merged
criemen merged 5 commits into
mainfrom
criemen/lua-tracing-ff
May 25, 2022
ConversationCommits (5)ChecksFiles changed
Merged

Introduce a feature-flag to enable/disable lua-based tracing.#1057
criemen merged 5 commits into
mainfrom
criemen/lua-tracing-ff

Conversation

criemen commented May 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 marked this pull request as ready for review May 5, 2022 10:28
criemen requested a review from a team as a code owner May 5, 2022 10:28
henrymercer reviewed May 5, 2022
View reviewed changes

henrymercer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread src/codeql.ts
criemen requested a review from henrymercer May 9, 2022 12:03
criemen force-pushed the criemen/lua-tracing-ff branch from 12c03b4 to 93a3483 Compare May 9, 2022 12:03
henrymercer approved these changes May 9, 2022
View reviewed changes

henrymercer left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented May 10, 2022

Copy link
Copy Markdown
Contributor Author

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 commented May 10, 2022

Copy link
Copy Markdown
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
Introduce a feature-flag to enable/disable lua-based tracing.
9e9a842 
This allows us to gradually roll out (or even roll back)
Lua-based tracing in case problems occur.
Add tests for the Lua feature flag.
db50ada
criemen force-pushed the criemen/lua-tracing-ff branch from 93a3483 to c9829af Compare May 16, 2022 09:17

criemen commented May 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 force-pushed the criemen/lua-tracing-ff branch from c9829af to 970e087 Compare May 16, 2022 09:40
criemen marked this pull request as draft May 16, 2022 21:13
criemen marked this pull request as ready for review May 25, 2022 07:39
criemen enabled auto-merge May 25, 2022 09:48
criemen merged commit 822fe5e into main May 25, 2022
criemen deleted the criemen/lua-tracing-ff branch May 25, 2022 10:33
This was referenced Jun 1, 2022
Merged
Merged
This was referenced Apr 24, 2023
Open
Open
This was referenced Apr 25, 2023
Open
Open
Open
snyk-bot mentioned this pull request Apr 25, 2023
Open
This was referenced Apr 26, 2023
Open
Open
aliscco mentioned this pull request Apr 26, 2023
Open
This was referenced Apr 27, 2023
Open
Open
This was referenced Apr 27, 2023
Open
Open
Open
snyk-bot mentioned this pull request Apr 28, 2023
Open
This was referenced Jul 12, 2023
Open
Open
Open
Open
Open
Open
This was referenced Nov 28, 2023
Open
Open
Open
Open
Open
Open
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

henrymercer henrymercer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL