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.

Sign up for GitHub

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

Jump to bottom

Make uninstall more robust and informative #3618

Merged
merged 6 commits into from
Mar 26, 2021
Merged

Make uninstall more robust and informative #3618

merged 6 commits into from
Mar 26, 2021

Conversation

carlisia
Copy link
Contributor

@carlisia carlisia commented Mar 24, 2021 •
edited
Loading

Signed-off-by: Carlisia carlisia@vmware.com

Thank you for contributing to Velero!

Please add a summary of your change

  • used the same existing API clients as the original code
  • aggregated the errors until all operations are done
  • added more informative messages
  • added confirmation
  • added --wait (for the namespace termination) and --force (to be script friendly and skip confirmation)
  • the output from --wait will look the same to the user as the --wait for creating backups

image

PS: 2e2 tests pass on Kind, testing it against AWS..

Does your change fix a particular issue?

Fixes #3599

Please indicate you've done the following:

Make uninstall more robust and informative
4fff2a4 
Signed-off-by: Carlisia <carlisia@vmware.com>
@carlisia carlisia added the kind/release-blocker Must fix issues for the coming release (milestone) label Mar 24, 2021
@carlisia carlisia added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Mar 24, 2021
@carlisia carlisia mentioned this pull request Mar 24, 2021
Closed
3 tasks
@carlisia carlisia marked this pull request as ready for review March 24, 2021 19:12
@carlisia carlisia requested a review from dsu-igeek March 24, 2021 19:13
dsu-igeek
dsu-igeek suggested changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@dsu-igeek dsu-igeek left a comment

Choose a reason for hiding this comment

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

Looks good, just some fixes to the messages.

pkg/cmd/cli/uninstall/uninstall.go Outdated Show resolved Hide resolved
pkg/cmd/cli/uninstall/uninstall.go Outdated Show resolved Hide resolved
pkg/cmd/cli/uninstall/uninstall.go Outdated Show resolved Hide resolved
test/e2e/velero_utils.go Outdated Show resolved Hide resolved
zubron
zubron previously requested changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

Thanks, @carlisia! This looks good, I really like the clearer feedback to the user about what's happening 👍 I just have a few additional comments along with the ones already mentioned by Dave.

pkg/cmd/cli/uninstall/uninstall.go Outdated Show resolved Hide resolved
pkg/cmd/cli/uninstall/uninstall.go Outdated Show resolved Hide resolved
Carlisia added 2 commits March 25, 2021 13:24
Addressed Dave's review
61c891f 
Signed-off-by: Carlisia <carlisia@vmware.com>
Addressed Bridget's review
ae47ac0 
Signed-off-by: Carlisia <carlisia@vmware.com>
ashish-amarnath
ashish-amarnath previously requested changes Mar 25, 2021
View reviewed changes
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.

LGTM overall.
I have a few minor comments.

pkg/cmd/cli/uninstall/uninstall.go Show resolved Hide resolved
pkg/cmd/cli/uninstall/uninstall.go Outdated Show resolved Hide resolved
Address Ashish's review
c64f45e 
Signed-off-by: Carlisia <carlisia@vmware.com>
Adress Ashish's second review
6decba9 
Signed-off-by: Carlisia <carlisia@vmware.com>
@carlisia
Copy link
Contributor Author

carlisia commented Mar 26, 2021 •
edited
Loading

For reviewers: if you think the return on LN 155 seem weird, you are thinking like me and expected the cancel() call to return but no, it continues that iteration til the end and then considers it "done". The return was needed.

dsu-igeek
dsu-igeek previously approved these changes Mar 26, 2021
View reviewed changes
Remove unnecessary check
e498a23 
Signed-off-by: Carlisia <carlisia@vmware.com>
@dsu-igeek dsu-igeek dismissed zubron’s stale review March 26, 2021 00:27

Comments addressed

zubron
zubron approved these changes Mar 26, 2021
View reviewed changes
Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

Thanks, @carlisia!

@dsu-igeek dsu-igeek added this to the v1.6.0 milestone Mar 26, 2021
dsu-igeek
dsu-igeek approved these changes Mar 26, 2021
View reviewed changes
Copy link
Contributor

@dsu-igeek dsu-igeek left a comment

Choose a reason for hiding this comment

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

LGTM!

@dsu-igeek dsu-igeek merged commit 6a67347 into vmware-tanzu:main Mar 26, 2021
@carlisia carlisia deleted the c-uninstall branch March 29, 2021 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zubron zubron zubron approved these changes

@dsu-igeek dsu-igeek dsu-igeek approved these changes

@jenting jenting Awaiting requested review from jenting

@ashish-amarnath ashish-amarnath Awaiting requested review from ashish-amarnath

Assignees

@carlisia carlisia

Labels
has-e2e-tests kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes kind/release-blocker Must fix issues for the coming release (milestone)
Projects
None yet
Milestone
v1.6.0
Development

Successfully merging this pull request may close these issues.

Bug with the Velero uninstall cmd
4 participants
@carlisia @dsu-igeek @zubron @ashish-amarnath