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

fix #7440 feat(nimbus): migrate sticky targeting configs #7446

Merged
merged 1 commit into from
Jun 27, 2022
Merged

Conversation

jaredlockhart
Copy link
Collaborator

@jaredlockhart jaredlockhart commented Jun 21, 2022

Because

  • Now that the sticky targeting clause is moved right into the NimbusExperiment targeting method
  • There's no need for the sticky clause to be included in individual targeting configs

This commit

  • Removes the sticky clauses from all desktop and mobile targeting configs
  • Removes the now redundant 'pip_never_used_sticky' targeting config and migrates experiments that used it to 'pip_never_used'
  • Migrates all existing experiments that use a previously sticky targeting config to have is_sticky=True
  • Discovered that some JEXL clauses were not being evaluated at all as part of the integration tests becuase of short circuiting behaviour in the JEXL evaluator
  • Reworked the SDK integration test to pass values in as their expected types rather than as strings which was triggering a suppressed error about comparing strings to integers
  • Filed Invalid variables in left hand of JEXL or expression are not evaluated #7449 as a followup

@jaredlockhart jaredlockhart force-pushed the 7440 branch 2 times, most recently from 89aee1f to 03ccecb Compare June 22, 2022 16:41
@jaredlockhart jaredlockhart requested a review from b4handjr as a code owner June 22, 2022 16:41
Comment on lines +2 to +3
"query_result": {
"id": 15734423,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh vscode moved the indentation around

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The actual change was trying to get the custom_targeting_attributes to have the expected types, ie integers and bools, rather than strings. But it turns out this whole thing gets parsed and expects only string types. But you can pass in custom_targeting_attributes separately into the helper encoded as JSON with the actual types, so I ended up just taking it out of here entirely.

Because

* Now that the sticky targeting clause is moved right into the NimbusExperiment targeting method
* There's no need for the sticky clause to be included in individual targeting configs

This commit

* Removes the sticky clauses from all desktop and mobile targeting configs
* Removes the now redundant 'pip_never_used_sticky' targeting config and migrates experiments that used it to 'pip_never_used'
* Migrates all existing experiments that use a previously sticky targeting config to have is_sticky=True
Copy link
Contributor

@yashika-khurana yashika-khurana left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@dmose
Copy link
Member

dmose commented Jun 24, 2022

The desktop sticky migration stuff looks good to me. r=dmose

@jaredlockhart jaredlockhart merged commit 3a1aa13 into main Jun 27, 2022
@jaredlockhart jaredlockhart deleted the 7440 branch June 27, 2022 15:13
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.

4 participants