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

Add support for running ML-powered queries for JS security-extended behind ml_powered_queries feature flag #857

Merged
merged 6 commits into from
Jan 6, 2022

Conversation

henrymercer
Copy link
Contributor

This PR builds on #856 to add support for running ML-powered queries. Currently these queries would only be run for customers (a) performing JavaScript analysis, (b) running the security-extended (or security-and-quality) query suite, and (c) whose repos are opted into the ml_powered_queries feature flag.

The action happens in src/config-utils.ts: when all of these conditions hold, we add the codeql/javascript-experimental-atm-queries pack to the analysis.

Since the security review for this feature is not yet complete, these queries will currently return no results. We will not roll out the feature flag until this security review is complete to avoid needlessly increasing the runtime of customers' analyses by downloading the ML-powered query pack.

Points for review

We want to enable ML-powered queries based in part on which query suite the customer is running. We resolve query suites into query paths in the init action, and the analysis action to my knowledge doesn't know what query suites the user requested. Therefore the init action is the natural point to test whether we should run ML-powered queries.

However this does necessitate an API call to test feature flag enablement in the init action as well as the existing one in the analysis action that's used for database upload.

Another option is saving some kind of information (e.g. feature flags or the query suites requested) to the config file in the init action and loading them in the analysis action. I didn't go with this since it seems like overengineering for the benefit of one fewer API call, and the API call in consideration doesn't count towards rate limits, but what do reviewers think?

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.

@henrymercer henrymercer requested a review from a team as a code owner December 15, 2021 20:04
Base automatically changed from henrymercer/feature-flagging to main December 16, 2021 16:18
@henrymercer henrymercer force-pushed the henrymercer/ml-powered-queries branch 3 times, most recently from 14a0324 to 02bfe3b Compare December 16, 2021 17:20
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Couple of questions inline.

Also, to answer yours, I think it's fine to make the feature flags request in init AND analyze.

What do you think about adding a comment of storing the feature flags in the config file as a potential future improvement?

src/config-utils.ts Outdated Show resolved Hide resolved
src/init-action.ts Show resolved Hide resolved
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

This is good to go in after updating the version range.

@henrymercer henrymercer force-pushed the henrymercer/ml-powered-queries branch from c2a20cd to e7fe6da Compare January 6, 2022 11:58
@henrymercer henrymercer merged commit 848e514 into main Jan 6, 2022
@henrymercer henrymercer deleted the henrymercer/ml-powered-queries branch January 6, 2022 17:55
@github-actions github-actions bot mentioned this pull request Jan 11, 2022
5 tasks
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

2 participants