Conversation
|
Only rollouts that are |
|
Blocked on mermaid diagram updates |
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
jaredlockhart
left a comment
There was a problem hiding this comment.
This is coming along great! At a glance I think it's all looking solid to me, really clear structure and patterns. I'd suggest breaking this up for a couple reasons
- Good to get practice breaking up big commits
- Easier to review in pieces
- Easier to revert a single piece if we find a bug after we deploy
An example of how we could break this up
- Add "Disabled" state and just whatever fixes are needed to get that to pass
- Add the phase transition methods and their tests
- Update the kinto jobs and Q methods to handle the new disabled cases
- Add the forms for the state transitions and tests
- Add the views/urls the state transitions and tests
Give it a shot, here's a sketch of how I usually break up large commits
First file individual tickets for each smaller split so you still have 1 pr : 1 ticket
<starting from current branch>
git checkout -b whatever-branch-split-1
git reset --soft HEAD~1
<unstage everything and add one at a time, or unstage one at a time>
git commit -m 'Branch phase 1'
<push and make pr>
<repeat>
jaredlockhart
left a comment
There was a problem hiding this comment.
And actually it occurs to me that we can't/shouldn't make the kinto workflow changes becuase we risk breaking publishing/unpublishing existing rollouts. Theoretically we could gate all the kinto task changes on (experiment.is_rollout, experiment.phases.count() > 0) to identify 'old' vs 'new' rollouts, but there's still a chance we miss that somewhere and break the RS flows for current rollouts. So maybe what we should do is pull just the kinto task changes out into a separate PR, mark it as blocked, and then hold on on it until after the rest of the UI has gone through sufficient QA and then we can merge it more closely to when we're actually getting ready to make the switch.
|
+1 to @jaredlockhart comment to split this work, and yeah we should hold changes to the kinto |
|
This pr has been broken up into the follow 5 for easier reviewing and potential future reverts. The prs should be merged in the following order:
|
Because
DisabledstateThis commit
Fixes #15085