Skip to content

Advice user on non-existing profile instead of crash#4896

Merged
medyagh merged 1 commit intokubernetes:masterfrom
RA489:profilesm
Aug 14, 2019
Merged

Advice user on non-existing profile instead of crash#4896
medyagh merged 1 commit intokubernetes:masterfrom
RA489:profilesm

Conversation

@RA489
Copy link
Copy Markdown

@RA489 RA489 commented Jul 28, 2019

When minikube profile is delete and try to do minkube ssh , it crashes with error
fixes #4889

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 28, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jul 28, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RA489

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2019
@RA489
Copy link
Copy Markdown
Author

RA489 commented Jul 28, 2019

/assign @RA489

medyagh
medyagh reviewed Jul 28, 2019
View reviewed changes
Comment thread pkg/minikube/problem/err_map.go Outdated
Issues: []int{4599},
},
"PROFILE_NOT_FOUND": {
Regexp: re(`Could not find the minikube profile`),
Copy link
Copy Markdown
Member

@medyagh medyagh Jul 28, 2019

Choose a reason for hiding this comment

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

the regex should be, "Machine does not exist for api.Exists(lis)"

Copy link
Copy Markdown
Author

@RA489 RA489 Jul 29, 2019

Choose a reason for hiding this comment

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

Done!

medyagh
medyagh requested changes Jul 28, 2019
View reviewed changes
Comment thread pkg/minikube/problem/err_map.go Outdated
Advice: "Please make sure the service you are looking for is deployed or is in the correct namespace.",
Issues: []int{4599},
},
"PROFILE_NOT_FOUND": {
Copy link
Copy Markdown
Member

@medyagh medyagh Jul 28, 2019

Choose a reason for hiding this comment

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

this should be "MACHINE_NOT_FOUND"

Copy link
Copy Markdown
Author

@RA489 RA489 Jul 29, 2019

Choose a reason for hiding this comment

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

Done!

@medyagh medyagh changed the title Advice user when minikube profile is deleted wip: Advice user when minikube profile is deleted Jul 28, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2019
@RA489 RA489 force-pushed the profilesm branch 2 times, most recently from bc86b59 to ff81ad7 Compare July 29, 2019 01:56
@RA489
Copy link
Copy Markdown
Author

RA489 commented Jul 29, 2019

@medyagh Thanks for your reviews! I have updated the PR as per your suggestions.

@RA489 RA489 changed the title wip: Advice user when minikube profile is deleted Advice user when minikube profile is deleted Jul 29, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2019
medyagh
medyagh requested changes Jul 29, 2019
View reviewed changes
Comment thread pkg/minikube/problem/err_map.go Outdated
Issues: []int{4599},
},
"MACHINE_NOT_FOUND": {
Regexp: re(`Machine does not exist for api.Exists(lis)`),
Copy link
Copy Markdown
Member

@medyagh medyagh Jul 29, 2019

Choose a reason for hiding this comment

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

the (lis) is extra ( that was my example profile)
it should be "Machine does not exist for api.Exists"

I recommend attaching the output of the console to try, I recmmend
deleting your minikube then
then try ./out/minikube ssh -p p1 -- ls -lah

then paste the output in the PR review so we know it worked :)

Copy link
Copy Markdown
Author

@RA489 RA489 Aug 1, 2019

Choose a reason for hiding this comment

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

sure

Copy link
Copy Markdown
Author

@RA489 RA489 Aug 1, 2019

Choose a reason for hiding this comment

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

Done!

@medyagh medyagh changed the title Advice user when minikube profile is deleted Advice user does command ssh on non existing profile Jul 29, 2019
@RA489
Copy link
Copy Markdown
Author

RA489 commented Aug 2, 2019

@medyagh attaching the output of the console ```
root@openstack-nti-18:/home/ubuntu/k8sworkspaces/minikube/out# minikube ssh -p p1 -- ls -lah

💣 Error getting host: Machine does not exist for api.Exists(p1)

😿 Sorry that minikube crashed. If this was unexpected, we would love to hear from you:
👉 https://github.com/kubernetes/minikube/issues/new

@tstromberg
Copy link
Copy Markdown
Contributor

tstromberg commented Aug 14, 2019

I don't think this is taking the optimal approach: the error is still opaque.

Rather than advice, I would prefer that ssh to exit with a very clear message, such as:

There is no active "minikube" cluster to ssh to.

I took a poke around, and think this could be solved by updating ssh.go to use api.Exists, such as:

	exists, err := api.Exists(machineName)
	if err != nil {
		return exit.WIthError(...)
	}
	if !exists {
               return exit.WithCode(exit.Data, "crafty error message")
       }

Alternatively, I saw that dashboard.go does some fancy error type checking, but api.Exists seems the clearest.

@tstromberg tstromberg added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2019
@medyagh
Copy link
Copy Markdown
Member

medyagh commented Aug 14, 2019

@RA489 Thank you for this contribution ! this will make user experience better

@medyagh medyagh merged commit 99f5771 into kubernetes:master Aug 14, 2019
@medyagh medyagh changed the title Advice user does command ssh on non existing profile Advice user on non-existing profile Aug 14, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2019
@medyagh medyagh changed the title Advice user on non-existing profile Advice user on non-existing profile instead of crash Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@medyagh medyagh medyagh requested changes

@afbjorklund afbjorklund Awaiting requested review from afbjorklund

@sharifelgamal sharifelgamal Awaiting requested review from sharifelgamal

+1 more reviewer

@tstromberg tstromberg tstromberg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@RA489 RA489

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

UI: Advice user when minikube profile is deleted

4 participants

@RA489 @k8s-ci-robot @tstromberg @medyagh