[Fixs #109] Adding confirmation on delete actions

[xmudrii]

Fixs #109
Continues work from #110
@bryanl I would like review if possible, in free time ofc. :D

Adding confirmation on delete actions

This is basic yes/no confirmation. User need to answer y, ye or yes. For every other answer, command will be stopped.
For now it is only implemented for Droplet delete action.

List of done things that are working:

• Created confirmation.go
• Added confirmation before Droplet delete (both on ID & Tag name)
• Added local force flag
• Added special function for improved output
• Created test units for confirmation.go
• Updated test units for droplets.go
• Fixed Travis CI errors

Things could be done:

• Add confirmation on delete for other actions

Explanation

First of all I created confirmation.go because of reusability. It would not be bad to extend it on volume delete, images delete, but this is something we will discuss later. Anyways I decided to make it reusable from beginning.

It is working like, you have to answer y or yes (case doesn't matters as I convert it to lower). If you answer anything other, operation will be aborted.

I added Force flag to doit, but I'm not sure is it right way, I would like some suggestion on this.

[xmudrii]

Update 10/06/2016:

First of all I did some AskForConfirm code refactor. Now it is only one function and it is using bufio.NewReader instead of fmt.Scanf. It is because I'm in progress of creating tests and it found it easier.
It can be still reverted, but epic low chance.

I have improved question, it is not over 4 lines, it is only in one line now:

Warning: Are you sure you want to delete droplets(s) (y/N)? *user input - y, n...*

This involved creation of new warn function - warnConfirm, so it would like @bryanl to confirm me is it OK to create new func.

CI is still unresolved, but it will be soon. :)

[xmudrii]

Update 10/07/2016 - whole changelog:

confirmation.go:

Confirmation input is changed to be in fashion like retrieveUserTokenFunc. It made me easier to make tests for it and I wanted to use same input method across whole project.
It is now returning error instead of bool as I see error is mostly used and I want to create it with some standard.

confirmation_test.go

I created basic test unit that test:

• Input yes
• Input no
• Input any
• Input with error

I made sure that it uses testify like other tests at doctl.

doit.go

Added global bool flag - force. It can be called as --force or only -f.

droplets.go

Implemented ask for confirmation.

droplets_test.go

Added Force for tests.

errors.go

Added warnConfirm function for displaying Are you sure message.
Problem with warn was \n\n.
warn would output:

Warning: Are you sure you want to delete droplet(s)? (y/N) ?
--- new line ---
--- new line ---
*User input goes here* - y/n

As I wanted it to be in one line e.g. :

Warning: Are you sure you want to delete droplet(s)? (y/N) ? *User input goes here* - y/n

I created new function warnConfirm that works like this.

Comments:

My main concern is Force flag. I have feel like it can be implemented much better. I'm not sure is it good idea to make it global, and is it good idea to call Force from droplets.go and droplets_test.go.
@bryanl Removed [WIP] flag, it is usable and stable now. Improvements can probably be done, but I would like review at first, as it has bit changes. :)

[bryanl]

Should force be global option or is it specific to the command?

[xmudrii]

For now I was thinking about adding it only to RunDropletDelete. Because at every other command it is unusable, actually it would do nothing.
This way was like, most easiest, I would be able to use both --force & -f and it worked.
At first I wanted to make it only for RunDropletDelete, so I used AddBoolFlag but I was not sure how to add alias to it (--force and -f) and what to add in doit.go for it.

[xmudrii]

@bryanl force flag has been updated!
I made it now to work as local flag tied to cmdRunDropletDelete. Global flag was overkill IMO, it is for now only usable on one action, and even in case we extend to other commands, it is only for delete.
I can revert to global if needed, but I think it should be specific to command. Anyways you know it better to me, so I'm open to suggestions. ;)

It's ready to be merged, I think.
I would like opinion on extending it to other delete actions. As far as I see there are following delete actions:

• Domains
• Domains - Records
• Floating IPs
• Images
• SSH keys
• Tags

One drawback of it is that it need to be called with --force. I'm out of luck to make it only with -f, as aliasOpt("f") is not working on AddBoolFlag.
Tests are corrected to use new force flag and Travis CI/Appveyor passes successfully :)

[bryanl]

@xmudrii There is an issue with short flags. I've seen it before, but haven't tracked down the culprit in pflags.

[bryanl]

typo :)

[xmudrii]

@bryanl didn't noticed, it happens :D Fixed typo.
Maybe I could change it to Force droplet deletion ? What would sound better

[justintime4tea]

When doctl is used within automation how do we "bypass" the dialog or perform "--non-interactive" actions?

[xmudrii]

You can do it by using force flag, e.g. doctl compute droplet delete id --force. :)