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

Added confirmation for storage deletion #447

Merged
merged 1 commit into from
May 25, 2018

Conversation

surajnarwade
Copy link
Contributor

@surajnarwade surajnarwade commented May 11, 2018

fixes #228

Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

Please don't forget to update tests.

cmd/storage.go Outdated
if storageForceDeleteflag {
confirmDeletion = "y"
} else {
fmt.Printf("Are you sure you want to delete the application: %v? [y/N] ", storageName)
Copy link
Member

Choose a reason for hiding this comment

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

application?

@surajnarwade surajnarwade force-pushed the storage-del branch 2 times, most recently from 9b71a1a to 5f27b38 Compare May 11, 2018 13:22
@surajnarwade
Copy link
Contributor Author

ping @kadel

@kadel kadel added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 18, 2018
cmd/storage.go Outdated
err = storage.Remove(client, storageName, applicationName, componentName)
checkError(err, "failed to delete storage")
switch storageName {
case "":
Copy link
Member

Choose a reason for hiding this comment

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

deleting all storage from component was removed in #463

This should be addressed in rebase

@surajnarwade surajnarwade removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 23, 2018
@kadel kadel added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 23, 2018
cmd/storage.go Outdated
if storageForceDeleteflag {
confirmDeletion = "y"
} else {
fmt.Printf("Are you sure you want to delete the storage: %v? [y/N] ", storageName)
Copy link
Member

Choose a reason for hiding this comment

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

It would be good if this would also show if the storage is mounted or not and if yes than to what component.

If not mounted:
Are you sure you want to delete the storage %v?

if mounted:
Are you sure you want to delete the storage %v mounted to %s in %s component?
For example:
Are you sure you want to delete the storage data mounted to /data in backend component?

Copy link
Member

Choose a reason for hiding this comment

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

I've opened new issue to address this. So we don't' have to block this pr

#473

@kadel
Copy link
Member

kadel commented May 23, 2018

Partially fixes #228

Why only partially?

@surajnarwade surajnarwade removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 24, 2018
@kadel kadel added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 24, 2018
@kadel
Copy link
Member

kadel commented May 24, 2018

Sorry about that. It needs another rebase :-(

@surajnarwade
Copy link
Contributor Author

@kadel no worries, that was easy rebase :P

@surajnarwade surajnarwade removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 25, 2018
@kadel
Copy link
Member

kadel commented May 25, 2018

Great work @surajnarwade.

@kadel kadel merged commit 304ed23 into redhat-developer:master May 25, 2018
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.

delete storage
2 participants