Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Add --confirm flag to delete component without asking for confirmation #568

Merged
merged 1 commit into from
Jun 5, 2020

Conversation

knrt10
Copy link
Member

@knrt10 knrt10 commented Jun 5, 2020

fixes #495

Signed-off-by: knrt10 kautilya@kinvolk.io

@knrt10 knrt10 requested review from surajssd and invidian June 5, 2020 08:41
@knrt10 knrt10 force-pushed the knrt10/delete-component-flag branch 2 times, most recently from c267bde to 2b60357 Compare June 5, 2020 08:46
@knrt10 knrt10 requested a review from johananl as a code owner June 5, 2020 08:46
invidian
invidian previously approved these changes Jun 5, 2020
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Looks OK

Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

Just a nit.

cli/cmd/component-delete.go Outdated Show resolved Hide resolved
Fixes #495

Signed-off-by: knrt10 <kautilya@kinvolk.io>
@knrt10 knrt10 force-pushed the knrt10/delete-component-flag branch from eedd2a9 to feb4981 Compare June 5, 2020 10:40
Copy link
Member

@ipochi ipochi left a comment

Choose a reason for hiding this comment

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

@knrt10 Thanks for the PR 🎉

LGTM

Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

lgtm

@knrt10 knrt10 merged commit 74959ad into master Jun 5, 2020
@knrt10 knrt10 deleted the knrt10/delete-component-flag branch June 5, 2020 11:03
Copy link
Member

@johananl johananl 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 the PR!

Looks like I'm too late. I have a few nits. Will leave it up to you to decide what to do about them.

@@ -44,6 +44,7 @@ func init() {
componentCmd.AddCommand(componentDeleteCmd)
pf := componentDeleteCmd.PersistentFlags()
pf.BoolVarP(&deleteNamespace, "delete-namespace", "", false, "Delete namespace with component.")
pf.BoolVarP(&confirm, "confirm", "", false, "Delete component without asking for confirmation.")
Copy link
Member

@johananl johananl Jun 5, 2020

Choose a reason for hiding this comment

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

AFAICT we don't end flag descriptions with periods. Example:

pf.BoolVarP(&confirm, "confirm", "", false, "Upgrade cluster without asking for confirmation")

) {
contextLogger.Info("Components deletion cancelled.")
return
if !confirm {
Copy link
Member

Choose a reason for hiding this comment

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

Nit - can save some indentation:

diff --git a/cli/cmd/component-delete.go b/cli/cmd/component-delete.go
index 76dfee7c..f4393f02 100644
--- a/cli/cmd/component-delete.go
+++ b/cli/cmd/component-delete.go
@@ -80,16 +80,14 @@ func runDelete(cmd *cobra.Command, args []string) {
                componentsObjects[i] = compObj
        }
 
-       if !confirm {
-               if !askForConfirmation(
-                       fmt.Sprintf(
-                               "The following components will be deleted:\n\t%s\n\nAre you sure you want to proceed?",
-                               strings.Join(componentsToDelete, "\n\t"),
-                       ),
-               ) {
-                       contextLogger.Info("Components deletion cancelled.")
-                       return
-               }
+       if !confirm && !askForConfirmation(
+               fmt.Sprintf(
+                       "The following components will be deleted:\n\t%s\n\nAre you sure you want to proceed?",
+                       strings.Join(componentsToDelete, "\n\t"),
+               ),
+       ) {
+               contextLogger.Info("Components deletion cancelled.")
+               return
        }
 
        kubeconfig, err := getKubeconfig()

@knrt10
Copy link
Member Author

knrt10 commented Jun 5, 2020

Should I revert the changes and create a new PR @johananl?

@iaguis
Copy link
Contributor

iaguis commented Jun 5, 2020

Should I revert the changes and create a new PR @johananl?

I'd create a new one that fixes periods at the end of flag descriptions for the whole project and another that does the indentation optimization.

@knrt10
Copy link
Member Author

knrt10 commented Jun 5, 2020

on it

@johananl
Copy link
Member

johananl commented Jun 5, 2020

Should I revert the changes and create a new PR @johananl?

Nah, no need to discard your good work :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add flag --confirm to lokoctl component delete
5 participants