Improving error message for service not in default namespace #5312
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @rbrishabh. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rbrishabh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Can one of the admins verify this patch? |
rbrishabh
force-pushed
the
patch-error
branch
from
33012bf
to
8bb5b30
Compare
k8s-ci-robot
added
size/XS
Travis tests have failedHey @rbrishabh, 1st Buildmake testTravisBuddy Request Identifier: bea2a4b0-d46b-11e9-847a-07722ef8bbdd |
Travis tests have failedHey @rbrishabh, 1st Buildmake testTravisBuddy Request Identifier: 7c168d40-d46c-11e9-847a-07722ef8bbdd |
Travis tests have failedHey @rbrishabh, 1st Buildmake testTravisBuddy Request Identifier: 5d1629e0-d495-11e9-847a-07722ef8bbdd |
rbrishabh
force-pushed
the
patch-error
branch
from
6fb1dbb
to
353c690
Compare
n0npax
reviewed
Sep 15, 2019
| @@ -266,7 +266,7 @@ func WaitAndMaybeOpenService(api libmachine.API, namespace string, service strin | |||
| chkSVC := func() error { return CheckService(namespace, service) } | |||
|
|
|||
| if err := retry.Expo(chkSVC, time.Duration(interval)*time.Second, time.Duration(wait)*time.Second); err != nil { | |||
| return errors.Wrapf(err, "Could not find finalized endpoint being pointed to by %s", service) | |||
| return errors.Wrapf(err, "Service %s was not found in default namespace, please try again with 'minikube service %s -n %s'", service, service, namespace) | |||
There was a problem hiding this comment.
Hi,
I'm not the core reviewer, but I believe it's not the best solution.
function CheckService
looks like:
212 // CheckService checks if a service is listening on a port.
213 func CheckService(namespace string, service string) error {
214 client, err := K8s.GetCoreClient()
215 if err != nil {
216 return errors.Wrap(err, "Error getting kubernetes client")
217 }
218
219 svc, err := client.Services(namespace).Get(service, meta.GetOptions{})
220 if err != nil {
221 return &retry.RetriableError{
222 Err: errors.Wrapf(err, "Error getting service %s", service),
223 }
224 }
225 if len(svc.Spec.Ports) == 0 {
226 return fmt.Errorf("%s:%s has no ports", namespace, service)
227 }
228 glog.Infof("Found service: %+v", svc)
229 return nil
230 } what if error was returned due to no ports?
maybe before printing error about default namespace we should check if namespace == "default" and embed error msg from CheckService
There was a problem hiding this comment.
Hey @n0npax ! Thanks, I'll take another look at this today!
|
@minikube-bot OK to test |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
@minikube-bot OK to test |
tstromberg
added
the
do-not-merge/work-in-progress
|
@rbrishabh do you mind pasting before and after output of this PR ? |
|
I am not able to test this locally, unfortunately! |
|
Error: running mkcmp: exit status 1 |
|
@rbrishabh sorry this PR went stale. I just noticed we have a newer PR #5844 sadly I have to close this one in favor of the newer one. I hope you still consider picking up another good-first issue |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #5271
Hey @josedonizetti @medyagh
Does this fix the issue?
I am very new with go-lang, so I might have gone wrong.
If I went wrong, please point me in the correct direction and I will be more than happy to look into this!
Thanks! =)