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

cmd/minikube: delete accept no arguments #1718

Merged
merged 1 commit into from
Jul 24, 2017

Conversation

darkowlzz
Copy link
Contributor

This changeset makes it necessary to pass a profile/machine name in order
to delete. This would help avoid accidental deletion of machine by just
delete command.

Fixes #1683

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 20, 2017
@codecov-io
Copy link

codecov-io commented Jul 20, 2017

Codecov Report

Merging #1718 into master will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1718      +/-   ##
==========================================
+ Coverage   36.23%   36.25%   +0.01%     
==========================================
  Files          51       51              
  Lines        3367     3371       +4     
==========================================
+ Hits         1220     1222       +2     
- Misses       1966     1969       +3     
+ Partials      181      180       -1
Impacted Files Coverage Δ
cmd/minikube/cmd/delete.go 8.33% <0%> (-1.67%) ⬇️
pkg/util/kubeconfig/config.go 46.85% <0%> (+1.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f32754f...17ba029. Read the comment docs.

@darkowlzz
Copy link
Contributor Author

Bump to verify CLA

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 20, 2017
@darkowlzz
Copy link
Contributor Author

darkowlzz commented Jul 20, 2017

oops! This isn't ready. Doesn't accepts the argument as profile. Fixing... 😅

Ready 👍

@darkowlzz darkowlzz force-pushed the 1683-delete-machine-profile branch from a43e5e1 to 20c8128 Compare July 20, 2017 13:17
@r2d4
Copy link
Contributor

r2d4 commented Jul 20, 2017

@minikube-bot ok to test

@darkowlzz darkowlzz force-pushed the 1683-delete-machine-profile branch from 20c8128 to 38ae8b0 Compare July 21, 2017 06:19
@darkowlzz
Copy link
Contributor Author

Some help with the failing test?
The test times out after 30 minutes. I'm not sure what's causing that. The delete command after the test seems to work and delete the vm.

Short: "Deletes a local kubernetes cluster",
Long: `Deletes a local kubernetes cluster. This command deletes the VM, and removes all
associated files.`,
Use: "delete MINIKUBE_PROFILE_NAME",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to break the default minikube delete case.

As a fix for #1683, I think we should just make sure to error on unknown arguments after command, rather than silently ignore them. i.e., $ minikube delete arg1 arg2 should fail, since we shouldn't ignore unknown arguments

As far as profiles go, each command accepts the --profile flag.

$ minikube delete, should work and delete the default machine
$ minikube delete --profile vm2 should delete the minikube profile vm2

I'm not a huge fan of moving the profile flag behavior to an optional noun after minikube <start, stop, delete> since we'd probably have to have every command behave that way. Sorry if my original comment on the issue was unclear,

minikube delete either takes no args or just the machine name

I think it really can't take the machine name without a pretty big change to the UX (since all commands should behave similarly.) But the enforcing the former (no unknown args) is definitely something we should do.

Let me know if that makes sense

Copy link
Contributor Author

@darkowlzz darkowlzz Jul 22, 2017

Choose a reason for hiding this comment

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

Yep, that makes sense. Consistency among the commands :)
I have updated the PR and made delete not accept any argument.

This changeset makes the delete command to error out if any argument is passed
to it.
@darkowlzz darkowlzz force-pushed the 1683-delete-machine-profile branch from 38ae8b0 to 17ba029 Compare July 22, 2017 12:44
@darkowlzz darkowlzz changed the title delete require profile/machine name cmd/minikube: delete accept no arguments Jul 22, 2017
@darkowlzz
Copy link
Contributor Author

Failing again 🤷‍♀️

Copy link
Contributor

@r2d4 r2d4 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 working through the feedback! And thanks for the PR!

@r2d4 r2d4 merged commit 5e5a0fb into kubernetes:master Jul 24, 2017
@javajon
Copy link

javajon commented Jul 24, 2017

As the original requester for #1683 it is great to see this enhancement added. Thanks all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants