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 #5739 feat(nimbus): trigger kinto update from publish status #5935

Merged
merged 1 commit into from Jul 13, 2021

Conversation

jaredlockhart
Copy link
Collaborator

Because

  • We want updates to live experiments to be triggered by users manually
  • This only current live update is pausing an experiment
  • In the future we will support updating other fields for live experiments

This commit

  • Removes the automatically triggered pause update logic
  • Refactors the live update kinto logic to be triggered by publish status the same way launch/end are
  • Refactors the V5 API to look at published_dto to determine if enrollment is ended
  • Updates the NimbusExperimentFactory to update published_dto after each live update
  • Updates the kinto diagrams to include setting is_paused from the UI to make frontend integration clearer for the current end enrollment flow

Because

* We want updates to live experiments to be triggered by users manually
* This only current live update is pausing an experiment
* In the future we will support updating other fields for live experiments

This commit

* Removes the automatically triggered pause update logic
* Refactors the live update kinto logic to be triggered by publish status the same way launch/end are
* Refactors the V5 API to look at published_dto to determine if enrollment is ended
* Updates the NimbusExperimentFactory to update published_dto after each live update
* Updates the kinto diagrams to include setting is_paused from the UI to make frontend integration clearer for the current end enrollment flow
Copy link
Contributor

@lmorchard lmorchard left a comment

Choose a reason for hiding this comment

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

Looking good to me, gave it a run locally and seemed okay for a few experiments. I have one stuck in "waiting" status - but that's probably still issue #5737

Comment on lines +305 to +307
@property
def is_paused_published(self):
return bool(self.published_dto and self.published_dto.get("isEnrollmentPaused"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, cool, this pattern looks generically handy for determining when particular updates have gotten published

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly yeah the more we switch things over to doing live updates, we'll need a way to distinguish unpublished/published changes. We might want something fancier like a proper revision system or something in the future, but for now this minimally works.

self.assertEqual(
data,
{
"application": experiment.application,
"channel": experiment.channel,
"feature_config": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing really changed here besides popping out the nested data for a separate comparison below, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I don't know why the feature_config suddenly started complaining, but I had to move things around to add the published_dto comparison and then feature_config started being cranky? But no I didn't change anything with it.

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