Reword missing API error to mention client update #1497
Conversation
There was a problem hiding this comment.
@dsimansk: 1 warning.
In response to this:
Description
Add a mention of client update to common missing API error message. In case of older client with newer backend with removed API, it might confusing why there's a failure.
Changes
- Reword missing API error to mention client update
Reference
Fixes #
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.
| ) | ||
|
|
||
| func newInvalidCRD(apiGroup string) *KNError { | ||
| func NewInvalidCRD(apiGroup string) *KNError { |
There was a problem hiding this comment.
Golint comments: exported function NewInvalidCRD should have comment or be unexported. More info.
knative-prow-robot
added
size/M
pkg/errors/errors.go
Outdated
| parts := strings.Split(apiGroup, ".") | ||
| name := parts[0] | ||
| msg := fmt.Sprintf("no Knative %s API found on the backend, please verify the installation", name) | ||
| msg := fmt.Sprintf("no Knative %s API found on the backend, please verify the installation or "+ | ||
| "update the 'kn' client to latest version", firstCharToUpper(name)) |
There was a problem hiding this comment.
Might be better to call it matching version?
There was a problem hiding this comment.
I wonder whether 'no Knative API' found is appropriate as there could be a Knative API available but too new. So maybe rephrase to "no or newer Knative %s API found on the backend, please verify the installation or update this client"
I just would say update, as it's kind of clear that it should be moved to a matching client.
There was a problem hiding this comment.
@rhuss addressed in latest update.
Codecov Report
@@ Coverage Diff @@
## main #1497 +/- ##
==========================================
+ Coverage 79.32% 79.33% +0.01%
==========================================
Files 162 162
Lines 8501 8509 +8
==========================================
+ Hits 6743 6751 +8
Misses 1076 1076
Partials 682 682
Continue to review full report at Codecov.
|
pkg/errors/errors.go
Outdated
| parts := strings.Split(apiGroup, ".") | ||
| name := parts[0] | ||
| msg := fmt.Sprintf("no Knative %s API found on the backend, please verify the installation", name) | ||
| msg := fmt.Sprintf("no Knative %s API found on the backend, please verify the installation or "+ | ||
| "update the 'kn' client to latest version", firstCharToUpper(name)) |
There was a problem hiding this comment.
I wonder whether 'no Knative API' found is appropriate as there could be a Knative API available but too new. So maybe rephrase to "no or newer Knative %s API found on the backend, please verify the installation or update this client"
I just would say update, as it's kind of clear that it should be moved to a matching client.
| @@ -37,3 +40,11 @@ func newNoRouteToHost(errString string) *KNError { | |||
| func newNoKubeConfig(errString string) *KNError { | |||
| return NewKNError("no kubeconfig has been provided, please use a valid configuration to connect to the cluster") | |||
| } | |||
|
|
|||
| func firstCharToUpper(s string) string { | |||
There was a problem hiding this comment.
+1
Actually, I would use this function, too, before emitting an error message to the console, so that error messages that start with a lowercase letter (golang convention) will be upcased. There should be a single exit-point for errors to do the massaging of the error message to print. But that's of course another story ;-)
dsimansk
force-pushed
the
pr/reword-missing-api
branch
from
ac749aa
to
164178a
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dsimansk, rhuss 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:
Approvers can indicate their approval by writing |
Description
Add a mention of client update to common missing API error message. In case of older client with newer backend with removed API, it might confusing why there's a failure.
Changes
Reference
Fixes #