Make error message more human readable #5563

nanikjava · 2019-10-07T22:17:15Z

Changes made:

service.go - changes to the error string returned
service_test.go - modify TestWaitAndMaybeOpenService test case to accomodate
for the new changes

Changes made: * service.go - changes to the error string returned * service_test.go - modify TestWaitAndMaybeOpenService test case to accomodate for the new changes

k8s-ci-robot · 2019-10-07T22:17:23Z

Hi @nanikjava. 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 /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.

k8s-ci-robot · 2019-10-07T22:17:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nanikjava
To complete the pull request process, please assign ra489
You can assign the PR to them by writing /assign @ra489 in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

minikube-bot · 2019-10-07T22:18:04Z

Can one of the admins verify this patch?

TravisBuddy · 2019-10-07T22:23:07Z

Travis tests have failed

Hey @nanikjava,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

make test

which go-bindata || GO111MODULE=off GOBIN=/home/travis/gopath/bin go get github.com/jteeuwen/go-bindata/...
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.12.9.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.0/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/assets/assets.go -pkg assets deploy/addons/...
gofmt -s -w pkg/minikube/assets/assets.go
which go-bindata || GO111MODULE=off GOBIN=/home/travis/gopath/bin go get github.com/jteeuwen/go-bindata/...
/home/travis/gopath/bin/go-bindata
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.12.9.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.0/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/translate/translations.go -pkg translate translations/...
gofmt -s -w pkg/minikube/translate/translations.go
./test.sh
= make lint =============================================================
golangci/golangci-lint info checking GitHub for tag 'v1.18.0'
golangci/golangci-lint info found version: 1.18.0 for v1.18.0/linux/amd64
golangci/golangci-lint info installed out/linters/golangci-lint
pkg/minikube/service/service_test.go:104: File is not `goimports`-ed (goimports)

Makefile:335: recipe for target 'lint-ci' failed
make[1]: *** [lint-ci] Error 1
= go mod ================================================================
ok
Makefile:232: recipe for target 'test' failed
make: *** [test] Error 4

TravisBuddy Request Identifier: 07ff37a0-e951-11e9-97f2-39e9ef4a9673

codecov-io · 2019-10-07T23:19:01Z

Codecov Report

Merging #5563 into master will increase coverage by 0.6%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #5563     +/-   ##
=========================================
+ Coverage   36.63%   37.23%   +0.6%     
=========================================
  Files         102      103      +1     
  Lines        7346     7528    +182     
=========================================
+ Hits         2691     2803    +112     
- Misses       4298     4352     +54     
- Partials      357      373     +16

Impacted Files	Coverage Δ
pkg/minikube/service/service.go	`76.19% <100%> (ø)`	⬆️
pkg/minikube/config/profile.go	`71.59% <0%> (-1.67%)`	⬇️
cmd/minikube/cmd/start.go	`22.63% <0%> (-0.03%)`	⬇️
pkg/minikube/assets/addons.go	`36.66% <0%> (ø)`	⬆️
cmd/minikube/cmd/config/profile.go	`0% <0%> (ø)`	⬆️
cmd/minikube/cmd/version.go	`0% <0%> (ø)`	⬆️
cmd/minikube/cmd/update-check.go	`0% <0%> (ø)`	⬆️
pkg/minikube/cluster/machine.go	`50% <0%> (ø)`
pkg/minikube/bootstrapper/kubeadm/kubeadm.go	`19.85% <0%> (+0.39%)`	⬆️
pkg/minikube/cluster/cluster.go	`38.05% <0%> (+0.58%)`	⬆️
... and 4 more

tstromberg · 2019-10-09T16:50:07Z

pkg/minikube/service/service.go

@@ -275,7 +275,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 with 'minikube service %s -n Y'", service, service)


I love this text, but it should detect if namespace is default, otherwise the error message doesn't make much sense.

Made changes to the string to include the service name.

@tstromberg any feedback ? Thanks

tstromberg · 2019-10-09T16:54:44Z

pkg/minikube/service/service_test.go

@@ -824,6 +845,7 @@ func TestWaitAndMaybeOpenService(t *testing.T) {
 		urlMode     bool
 		https       bool
 		err         bool
+		nondefault  bool


bools should never be negative or have a name that starts with with no: it gets too confusing, particularly for non-english readers, when nondefault=false. I also don't understand what this bool is the default of.

Instead of default or nondefault, I think this would be more readable if each test directly defined which servicesMap to use.

Will look into the test again and refactor to make it more readable for code structure

Refactor the code to split out the test on it's own to make it more easier to understand for other. Let me know what you think ?. Thanks

… error text to include service name

nanikjava · 2019-10-09T20:20:17Z

/assign

RA489 · 2019-10-10T05:41:07Z

/ok-to-test

tstromberg · 2019-10-17T17:13:38Z

pkg/minikube/service/service.go

@@ -275,7 +275,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 %s namespace , please try with 'minikube service %s -n Y'", service, namespace, service)


If I understand properly, "Y" doesn't have a special meaning. What do you think about something like this:

Service %s was not found in the %q namespace. You may select another namespace by using: 'minikube service %s -n <namespace>'

Make sense. PR has been updated. Thanks

tstromberg

Thank you!

Make error message more human readable

197215f

Changes made: * service.go - changes to the error string returned * service_test.go - modify TestWaitAndMaybeOpenService test case to accomodate for the new changes

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 7, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 7, 2019

nanikjava mentioned this pull request Oct 7, 2019

Improve error message for service not in default namespace. #5271

Closed

k8s-ci-robot requested review from blueelvis and medyagh October 7, 2019 22:17

Fix linting

75ce59d

tstromberg reviewed Oct 9, 2019

View reviewed changes

tstromberg suggested changes Oct 9, 2019

View reviewed changes

Move the non-found-service test into separate function and modify the…

733f7bc

… error text to include service name

k8s-ci-robot assigned nanikjava Oct 9, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 10, 2019

nanikjava requested a review from tstromberg October 16, 2019 00:19

tstromberg suggested changes Oct 17, 2019

View reviewed changes

Modify output text to make it more readable as per PR comment

10ff44e

tstromberg approved these changes Oct 17, 2019

View reviewed changes

tstromberg merged commit 3f86651 into kubernetes:master Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make error message more human readable #5563

Make error message more human readable #5563

nanikjava commented Oct 7, 2019

k8s-ci-robot commented Oct 7, 2019

k8s-ci-robot commented Oct 7, 2019

minikube-bot commented Oct 7, 2019

TravisBuddy commented Oct 7, 2019

codecov-io commented Oct 7, 2019 •

edited

Loading

tstromberg Oct 9, 2019

nanikjava Oct 9, 2019

nanikjava Oct 13, 2019

tstromberg Oct 9, 2019

nanikjava Oct 9, 2019

nanikjava Oct 9, 2019

nanikjava commented Oct 9, 2019

RA489 commented Oct 10, 2019

tstromberg Oct 17, 2019

nanikjava Oct 17, 2019

tstromberg left a comment

Make error message more human readable #5563

Make error message more human readable #5563

Conversation

nanikjava commented Oct 7, 2019

k8s-ci-robot commented Oct 7, 2019

k8s-ci-robot commented Oct 7, 2019

minikube-bot commented Oct 7, 2019

TravisBuddy commented Oct 7, 2019

Travis tests have failed

1st Build

TravisBuddy Request Identifier: 07ff37a0-e951-11e9-97f2-39e9ef4a9673

codecov-io commented Oct 7, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanikjava commented Oct 9, 2019

RA489 commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstromberg left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 7, 2019 •

edited

Loading