Conversation
|
Only rollouts that are |
|
Blocked on mermaid diagram updates |
RJAK11
left a comment
RJAK11
left a comment
There was a problem hiding this comment.
Overall this looks amazing and everything seemed mostly correct to me!! I just noticed a few things that we might want to consider
| required_status_next = None | ||
| required_publish_status = NimbusExperiment.PublishStatus.IDLE | ||
|
|
||
| status = NimbusExperiment.Status.PREVIEW |
There was a problem hiding this comment.
Based on first part of Cancel from Preview diagram I think we need:
| status = NimbusExperiment.Status.PREVIEW | |
| status = NimbusExperiment.Status.DRAFT |
There was a problem hiding this comment.
oh i just noticed. are there supposed to be another set of diagrams for launching from the preview state? that specific transition doesnt look like it has a corresponding mermaid diagram even though i believe it should be a valid transition
There was a problem hiding this comment.
disregard last comment i just read your explanation oops
| publish_status = NimbusExperiment.PublishStatus.REVIEW | ||
|
|
||
| def get_changelog_message(self): | ||
| return f"{self.request.user} requested rollout launch without Preview" |
There was a problem hiding this comment.
| return f"{self.request.user} requested rollout launch without Preview" | |
| return f"{self.request.user} requested rollout launch from Preview" |
| def get_changelog_message(self): | ||
| return f"{self.request.user} requested rollout launch without Preview" | ||
|
|
||
|
|
There was a problem hiding this comment.
Would we actually need PreviewReviewApproveRolloutForm and PreviewReviewRejectForm? Based on the "Cancel from Preview" diagram, I think launching from Preview sends the rollout back to Draft first so the rollout would be in the same Draft/Live/Review state as a Draft launch, so these forms might technically duplicate DraftReviewApproveRolloutForm and DraftReviewRejectForm. Maybe we could mirror the old UI and combine them if that seems right?
| experiment = super().save(commit=commit) | ||
| experiment.stage_rollout_phase_advance() | ||
| experiment.allocate_bucket_range() | ||
| nimbus_synchronize_preview_experiments_in_kinto.apply_async(countdown=5) |
There was a problem hiding this comment.
I assume we don't need this since we are going from Live to Live
| nimbus_synchronize_preview_experiments_in_kinto.apply_async(countdown=5) |
| @transaction.atomic | ||
| def save(self, commit=True): | ||
| experiment = super().save(commit=commit) | ||
| experiment.disable_rollout() |
There was a problem hiding this comment.
Here we disable the rollout but if the changes were to be rejected in remote settings I don't think we would restore them based on:
I assume we would need to make changes in that function as well
| @transaction.atomic | ||
| def save(self, commit=True): | ||
| experiment = super().save(commit=commit) | ||
| experiment.stage_rollout_phase_advance(copy_current_if_missing=True) |
There was a problem hiding this comment.
Same thing here as mentioned above since we would advance the phase but not restore the original one if it gets rejected in remote settings
|
|
||
| def disable_rollout(self): | ||
| if self.is_rollout: | ||
| self.population_percent = 0 |
There was a problem hiding this comment.
Not fully sure but I thought disable meant unpublish from remote settings to avoid piling up too many recipes based on the slack conversation
3 participants
Because
DisabledstateThis commit
Fixes #15085