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
Support recovery from in the middle of a rename. #7620
Conversation
89e0e41
to
5d1a6b6
Compare
Ok, I added tests to validate this. Please review. thanks! |
lgtm. |
@@ -161,7 +171,7 @@ func RunRollingUpdate(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, arg | |||
|
|||
if len(args) >= 2 { | |||
newName = args[1] | |||
} else { | |||
} else if oldRc != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, why would oldRc ever be nil here? If we didn't find it, wouldn't we would have returned already inside the block starting at 137?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be time to break this function up. Too hard to follow the logic if you have to scroll thru multiple screen lengths.
829f6bf
to
b9553b0
Compare
Addressed comments, and merged in retries changes. Please take another look. Thanks! |
} | ||
|
||
keepOldName := false | ||
keepOldName := len(args) == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only be doing rename if --image
was set. Rolling updates to an rc from filename should use the name from the schema file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, if len(args) == 1 and --image isn't set, it will error out earlier in validateArgs, but I'm happy to add an explicit check here if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see that check in validateArguments. Also, len(args) == 1
is expected for the --filename
case, so that check wouldn't quite work. Just moving this assignment under the if len(image) != 0
block should suffice.
Some function references need fixing. From travis:
|
Ok, I think I forgot to push some pieces. Re-pushed, please take another look. |
dbdb30c
to
8f34f3f
Compare
lgtm. will merge on green. |
Support recovery from in the middle of a rename.
Code written @ 12:30am on a plane. Treat with skepticisim...
(and don't merge yet, I want to add tests)