Skip to content
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

Merged
merged 4 commits into from
Jan 31, 2020
Merged

remove schedule validation #2218

merged 4 commits into from
Jan 31, 2020

Conversation

cpanato
Copy link
Contributor

@cpanato cpanato commented Jan 24, 2020

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:

@cpanato cpanato requested a review from skriss January 24, 2020 08:18
carlisia
carlisia previously approved these changes Jan 27, 2020
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍 Thank you!

@skriss
Copy link
Member

skriss commented Jan 27, 2020

thanks @cpanato! I tried this out and it works fine as-is: the --from-schedule flag works as long as there is a backup labeled with the schedule name, regardless of whether or not that particular schedule currently exists in the cluster. In the case where no backups exist with the given schedule name as a label, the restore terminates with a phase of FailedValidation.

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 .List(...) on backups with a label selector on velero.io/schedule-name=<schedule name>). If there are none, we could return an error. This provides more immediate feedback to the CLI user in that scenario. I don't think it's essential, but would be nice and should only require a few lines of code if you're up for it.

pkg/cmd/cli/restore/create.go Outdated Show resolved Hide resolved
@cpanato
Copy link
Contributor Author

cpanato commented Jan 29, 2020

@skriss @ashish-amarnath made the change, please let me know what do you think. thanks again for the review

Copy link
Member

@skriss skriss left a 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

pkg/cmd/cli/restore/create.go Outdated Show resolved Hide resolved
pkg/cmd/cli/restore/create.go Outdated Show resolved Hide resolved
@skriss
Copy link
Member

skriss commented Jan 29, 2020

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>
@cpanato
Copy link
Contributor Author

cpanato commented Jan 29, 2020

@skriss done :) ptal

skriss
skriss previously approved these changes Jan 29, 2020
Copy link
Member

@skriss skriss left a 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

@skriss
Copy link
Member

skriss commented Jan 29, 2020

thanks for the revisions @cpanato!

Copy link
Member

@ashish-amarnath ashish-amarnath left a 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>
@cpanato
Copy link
Contributor Author

cpanato commented Jan 31, 2020

@ashish-amarnath done, thanks for the inputs!
@carlisia @skriss PTAL

Copy link
Member

@ashish-amarnath ashish-amarnath left a 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!

@ashish-amarnath ashish-amarnath merged commit 21264a1 into vmware-tanzu:master Jan 31, 2020
@cpanato cpanato deleted the GH-1168 branch January 31, 2020 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In Cluster B allow --from-schedule to reference a schedule created in Cluster A
4 participants