Skip to content

Prompt to install CRD manually when Kind no found#57

Closed
interma wants to merge 1 commit into
kubernetes-sigs:masterfrom
interma:prompt_nocrd
Closed

Prompt to install CRD manually when Kind no found#57
interma wants to merge 1 commit into
kubernetes-sigs:masterfrom
interma:prompt_nocrd

Conversation

@interma
Copy link
Copy Markdown

@interma interma commented Aug 10, 2018

Fix: kubernetes-sigs/kubebuilder#337

New output:

$ make run
go generate ./pkg/... ./cmd/...
go fmt ./pkg/... ./cmd/...
go vet ./pkg/... ./cmd/...
go run ./cmd/manager/main.go
2018/08/10 14:44:52 Registering Components.
2018/08/10 14:44:52 No kind {core.hawq.org MyResource} found, please install CRD in advance.
exit status 1
make: *** [run] Error 1

Thanks for the review.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 10, 2018
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: interma
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers:

If they are not already assigned, you can assign the PR to them by writing /assign in a comment when ready.

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 size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 10, 2018
log.Fatal(err)
switch err.(type) {
case *meta.NoKindMatchError:
log.Fatalf("No kind %v found, please install CRD in advance.", err.(*meta.NoKindMatchError).GroupKind)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am wondering if we should add more context in the error in the controller-runtime library itself instead of putting the logic in generated code, WDYT ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

My thoughts:
Generally, lib (controller-tools) emits all errors and client (kubebuilder) decides what behavior to perform. Based on this point, my PR make sense.

But considering our client code is generated, more concise the better. I think to handle this error in controller-tools is also a good idea. Do you have some ideas about which code should I modify?

Thanks!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@droot just a ping, any thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, didn't follow this thread.

Actually, our client code is no longer generated. Kubebuilder project uses client pkg from https://github.com/kubernetes-sigs/controller-runtime. May be, we can just wrap the error when returning from client's pkg. Can you pl. investigate.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure, I will check later.
I will ping you if have some problems. Thanks.

@interma
Copy link
Copy Markdown
Author

interma commented Aug 31, 2018

@droot I opened a new PR in controller-runtime:
kubernetes-sigs/controller-runtime#129

Please help to review, thanks.

@droot
Copy link
Copy Markdown
Contributor

droot commented Aug 31, 2018

closing this PR for now.

@droot droot closed this Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants