-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
remove schedule validation #2218
Conversation
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.
lgtm 👍 Thank you!
thanks @cpanato! I tried this out and it works fine as-is: the One small improvement we could make is to replaced the deleted Schedule existence check with a check to see if there are any backups labeled with the schedule name (i.e. do a |
@skriss @ashish-amarnath made the change, please let me know what do you think. thanks again for the review |
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.
two minor comments -- LGTM once those are resolved
one other thing - please add a changelog file! |
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
@skriss done :) ptal |
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.
LGTM - @ashish-amarnath please take another look
thanks for the revisions @cpanato! |
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.
One minor comment about the variable name. It kinda threw me off that we were querying a list of backup
objects into a variable called scheduleItems
.
My comment was about renaming scheduleItems
to backupItems
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
@ashish-amarnath done, thanks for the inputs! |
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.
Thanks for working through this @cpanato!
the backup can be created from a schedule in another cluster and when trying to restore that in a new cluster this schedule might not exist, so removing this validation
More context see:
--from-schedule
to reference a schedule created in Cluster A #1168