Fix missing confirmation for reset

[shibumi]

What this PR does / why we need it:

This PR adds a request for confirmation for the reset subcommand.
We have a request for confirmation already in the apply subcommand, hence it should not be much work
adding the same question to reset. I really think reset should have such a question, because it destroys
the whole cluster.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

None

Special notes for your reviewer:

The functionality works so far, but I am not sure about the implementation. Actually it would be nicer to have access to the
LiveCluster object inside of reset, because the Cluster object doesn't seem to be "Dynamic Worker"-aware.
Furthermore it looks like the hostnames are missing when I test it:

The following nodes will be resetted: 
        + reset control plane node "" (10.88.10.2)
        + reset control plane node "" (10.88.10.3)
        + reset control plane node "" (10.88.10.4)
        + reset static worker nodes "" (10.88.10.11)
        + reset static worker nodes "" (10.88.10.12)
        + reset static worker nodes "" (10.88.10.13)
Do you want to proceed (yes/no): no

INFO[01:49:46 CET] Operation canceled.     

Does this have something to do with missing network access? I tested this without the VPN connection to my cluster (I did not have another cluster to test right now and I did not want to delete one of our company clusters out of a mistake).

Does this PR introduce a user-facing change?:

[BREAKING/ACTION REQUIRED] The kubeone reset command requires an explicit confirmation like the apply command. The command can be automatically approved by using the --auto-approve flag.

[kubermatic-bot]

Hi @shibumi. Thanks for your PR.

I'm waiting for a kubermatic member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[shibumi]

/assign @xrstf

[kron4eg]

This is breaking CLI change. But on the other hand it's precautionary measure. I'm not sure how to proceed.
@xmudrii WDYT?

[shibumi]

@kron4eg it is indeed a breaking CLI change, but I really think we should have such functionality :) My solution may not be the best, but it's definitely something the reset sub command should have. I was really surprised when I used kubeone reset the first time.

[xmudrii]

@shibumi Thank you for the PR! We've discussed it internally, and we agree that there should be a confirmation for the reset command because it's very destructive. However, this is a breaking CLI change and it should be handle as such. We considered the following two approaches:

1. Create a new command that would replace the reset command, and that would have the confirmation. The new command would deprecate the reset command, which we would remove after one or two releases
   • The problem is that reset very well describes what's done by the command, so it would be hard to come up with a new name
   • It's not that big breaking change that requires a new command
2. Starting with the upcoming 1.2 release (planned for the upcoming weeks), we would show a warning when users run kubeone reset that starting with the 1.3 release, the command will require confirmation. We would also announce this in the changelog. In this case, we would merge your PR after creating the release branch for the 1.2 release (that's going to happen in the upcoming weeks)

We've decided to go with the second approach (to show a warning) because it seems like the best approach for moving forward.

I'll put this PR on an explicit hold until we don't create a release branch. In meanwhile, I'll leave some comments which you can address.
/hold

If you want, you can create another PR that would add a warning to the kubeone reset command to say that the command will ask for a confirmation starting with the 1.3 release. It can say something along:

This command will require an explicit confirmation starting with the next release

[shibumi]

• It's not that big breaking change that requires a new command

2. Starting with the upcoming 1.2 release (planned for the upcoming weeks), we would show a warning when users run kubeone reset that starting with the 1.3 release, the command will require confirmation. We would also announce this in the changelog. In this case, we would merge your PR after creating the release branch for the 1.2 release (that's going to happen in the upcoming weeks)

We've decided to go with the second approach (to show a warning) because it seems like the best approach for moving forward.

Full ACK. The second option makes more sense. Would be a shame to deprecate the reset command only for a confirmation flag.

I'll put this PR on an explicit hold until we don't create a release branch. In meanwhile, I'll leave some comments which you can address.

Will have a look on your suggestions after work :) Until tomorrow you should have an updated version.

/hold

If you want, you can create another PR that would add a warning to the kubeone reset command to say that the command will ask for a confirmation starting with the 1.3 release. It can say something along:

This command will require an explicit confirmation starting with the next release

Will have a look on this, too.

[xmudrii]

@shibumi You'll also need to update E2E tests to run the reset command with the --auto-approve flag here:

kubeone/test/e2e/kubeone.go

Lines 185 to 197 in 1683388

	// Reset runs 'kubeone reset' command to destroy worker nodes and unprovision the cluster
	func (k1 *Kubeone) Reset() error {
	err := k1.run("reset",
	"-v",
	"--tfjson", "tf.json",
	"--destroy-workers",
	"--manifest", k1.ConfigurationFilePath)
	if err != nil {
	return fmt.Errorf("destroing workers failed: %w", err)
	}
	return nil
	}

[shibumi]

@xmudrii I made sure that the tests are running through now..

[xmudrii]

Tiny nits.

[shibumi]

@xmudrii done

[xmudrii]

/test all

[shibumi]

@xmudrii done. Shall I squash commits or is the commit history fine? My feeling is it's a little bit too atomic.

[xmudrii]

@shibumi We automatically squash all PRs on merge.

/test all

[shibumi]

@xmudrii what does this mean?

pkg/cmd/apply.go:29: File is not goimports-ed with -local k8c.io/kubeone (goimports)
"github.com/spf13/pflag"

[xmudrii]

/test all

[xmudrii]

@shibumi Thank you for the contribution!

/lgtm
/approve

@kron4eg PTAL and remove the hold.

[kubermatic-bot]

LGTM label has been added.

Git tree hash: c3c1ab0c33ba562bed1049b514eb18a20aad8b48

[kubermatic-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shibumi, xmudrii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [xmudrii]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[shibumi]

@xmudrii /retest please :) it was a timeout of your Ci system.

[kron4eg]

/retest

[xmudrii]

/retest

[shibumi]

The merge label is blocking the merge

[kron4eg]

/unhold