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
Rolling Updates: Add support for --rollback. #7575
Conversation
Looks good, though I would prefer |
I also am not a fan of abort. I believe @thockin feels similarly. |
Fixes #4140 |
Previous discussion: |
I don't super care. If the preference is for |
I'm fine with either --abort or --rollback, and will accept the consensus. When talking about it, I'm sure I'll continue to say "rollback". Additionally, FWIW, a reasonably common pattern internally is for teams to leave the old template around until the next rollout, to facilitate easy rollback. I'd like to expose the "preserve" cleanup policy and would like to support rollback/abort in the scenario where the rollout actually completed but the old RC was not deleted. Don't know if that changes your opinion about which is more appropriate or not. |
Thinking about this feature in the context of #1743, what effect would this feature have on the proposed monotonic revision number for deployments? |
@ironcladlou I don't think it should have any effect. At the API level, this is actually a roll-forward to the old version. That said, maybe you actually want the revision number to rewind? Can we have a vote on --rollback vs. --abort? I'd like to get this PR merged. |
Allow me to cast my vote for... --rollback |
--rollback Sent from my iPhone
|
--rollback (fwiw) |
Switched to ptal |
lgtm. will merge on green if no objections |
There's no test. Did you at least try this? The rename cleanup policy is the default. Does that make sense in this case? |
3221b9a
to
b499e1f
Compare
Yes, this has been manually tested. Without a better test framework in place this is tricky to test in an automated way. I could probably get something going in test-cmd.sh if that is required. Thanks |
lgtm, but you should probably change the commit description |
LGTM |
The commit description does still refer to --abort |
Changed the commit description. Thanks! |
Rolling Updates: Add support for --rollback.
Rolling Updates: Add support for --rollback.
@ghodss @jlowdermilk @deads2k @ironcladlou
Fixes #4140