-
Notifications
You must be signed in to change notification settings - Fork 263
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
Distinguish missing Serving/Eventing API from unsupported version #1868
base: main
Are you sure you want to change the base?
Conversation
Hi @xiangpingjiang. Thanks for your PR. I'm waiting for a knative 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xiangpingjiang 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiangpingjiang: 2 warnings.
In response to this:
Description
Distinguish missing Serving/Eventing API from unsupported version
Changes
Reference
Fixes #1858
Release Note
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.
pkg/errors/errors.go
Outdated
msg := fmt.Sprintf("no or newer Knative %s API found on the backend, please verify the installation or "+ | ||
"update the 'kn' client", firstCharToUpper(name)) | ||
return NewKNError(msg) | ||
func NewInvalidCRD(apiGroup string, resourceName string) *KNError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint comments: exported function NewInvalidCRD should have comment or be unexported. More info.
"update the 'kn' client", resourceName) | ||
return NewKNError(msg) | ||
|
||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint indent: if block ends with a return statement, so drop this else and outdent its block. More info.
/ok-to-test |
Signed-off-by: pingjiang <xiangpingjiang1998@gmail.com>
Signed-off-by: pingjiang <xiangpingjiang1998@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1868 +/- ##
==========================================
- Coverage 79.62% 79.49% -0.14%
==========================================
Files 179 179
Lines 13927 13969 +42
==========================================
+ Hits 11090 11104 +14
- Misses 2070 2097 +27
- Partials 767 768 +1 ☔ View full report in Codecov by Sentry. |
/retest |
if home := homedir.HomeDir(); home != "" { | ||
kubeconfig = flag.String("kubeconfig", filepath.Join(home, ".kube", "config"), "(optional) absolute path to the kubeconfig file") | ||
} else { | ||
kubeconfig = flag.String("kubeconfig", "", "absolute path to the kubeconfig file") | ||
} | ||
flag.Parse() | ||
// use the current context in kubeconfig | ||
config, _ := clientcmd.BuildConfigFromFlags("", *kubeconfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of initializing new kube client instance in such a manner. We carry-over KnParams
instance that holds all the clients commonly used, in addition it covers Kube's ClientConfig parsing, reflecting --kubeconfig
and --context
flags. I.e. you would need to cover more edge cases here.
I'd rather inject the KnParams
into the new error functions and retrieve the needed clients from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsimansk ,
I also want to reuse the KnParams, but I didn't find the right way. I will try to find how to inject the KnParams.
@@ -21,12 +21,23 @@ import ( | |||
"unicode/utf8" | |||
) | |||
|
|||
func NewInvalidCRD(apiGroup string) *KNError { | |||
func NewInvalidCRD(apiGroup string, canFindResource string) *KNError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about creating a specific cause functions per message. E.g. NewInvalidCRDCauseUnknown
, or define cause as a const string?
It might be easier to understand and consume whenever such new error is going to be added elsewhere etc.
This Pull Request is stale because it has been open for 90 days with |
/remove-lifecycle stale |
This Pull Request is stale because it has been open for 90 days with |
This Pull Request is stale because it has been open for 90 days with |
Description
Distinguish missing Serving/Eventing API from unsupported version
Changes
Reference
Fixes #1858
Release Note