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

Start running ATM queries again #999

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Conversation

edoardopirovano
Copy link
Contributor

Between the time when I ported the Action's code for parsing config files to the CLI and when we actually switched the Action to using the new code, the Action's code gained an interesting new effect that was not in the ported code: If the feature flag for ATM was set the Action would, while it was parsing the file, also add the ATM packs to it if they weren't already present. When we switched over to using the parsing in the CLI, this effect was lost and we stopped running ATM queries (!!).

This PR is a rather hacky fix that addresses this by recording (when the Action parses the config file - which it still does even if it later doesn't need it) whether it injected the ML queries. Then, when we pass the full config to the CLI we also augment it with the ML queries if we injected them before. It's not how I would like to do it, but there's a couple of reasons we can't do something more elegant:

  1. We don't want to wait for a CLI release to get things working again, so moving this injection into the CLI isn't an option. Additionally, the CLI would need to know whether the feature flag is set, and also the version of the ML pack to inject (which during the beta we want to record in the Action due to its shorter release cycle), so we'd have to pass a couple more flags in.
  2. We can't stop injecting into the Action's parsed version of the config file as well because old CLIs still use this. Plus, the telemetry looks here to know whether the ML queries are enabled.

This code is also missing tests, we have an internal issue tracking this. I'll manually test this for now, and before we touch this code again we should have an integration test that actually runs ATM queries, so we don't accidentally disable them again.

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.

@edoardopirovano edoardopirovano requested a review from a team as a code owner March 25, 2022 17:36
@@ -423,7 +431,7 @@ async function parseQueryUses(
featureFlags: FeatureFlags,
logger: Logger,
configFile?: string
) {
): Promise<boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just looking at this function, it's not clear what the return value is for. Can you add @return to the jsdoc above to document it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this is a good point. I've expanded the documentation.

Comment on lines 841 to 842
injectedMlQueries ||
(await parseQueryUses(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right? It looks like if a previous query input injected ML queries, then we won't parse any further ones. Do we want to call parseQueryUses no matter what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eek, yes you're right. We definitely don't want to short-circuiting of the || to happen here and need to call parseQueryUses regardless. I've fixed that.

@edoardopirovano edoardopirovano merged commit 0ed0799 into main Mar 28, 2022
@edoardopirovano edoardopirovano deleted the edoardo/reenable-atm branch March 28, 2022 21:06
This was referenced Mar 30, 2022
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