Prompt to install CRD when Kind no found #129

interma · 2018-08-31T02:41:06Z

New output:

$ make run
go generate ./pkg/... ./cmd/...
go fmt ./pkg/... ./cmd/...
go vet ./pkg/... ./cmd/...
go run ./cmd/manager/main.go
2018/08/31 10:40:04 Registering Components.
2018/08/31 10:40:04 must install CRD {core.hawq.org MyResource} before calling Start
exit status 1
make: *** [run] Error 1

Thanks for the review.

DirectXMan12 · 2018-09-04T20:50:39Z

pkg/source/source.go

-		return err
+		switch err.(type) {
+		case *meta.NoKindMatchError:
+			return fmt.Errorf("must install CRD %v before calling Start", err.(*meta.NoKindMatchError).GroupKind)


maybe use %s here instead to get it pretty-printed?

Updated.
They are the same output.

DirectXMan12 · 2018-09-05T17:08:42Z

looks good, please squash the commits together

interma · 2018-09-06T03:22:26Z

@DirectXMan12 Done.
Thanks for the review.

DirectXMan12 · 2018-09-06T17:38:33Z

one last thing before merge -- this message could potentially be confusing if the resource isn't a CRD, so we might want to change the wording a bit. Also, we lose the ability to have users inspect the error themselves, which probably isn't the most useful, but might be worth considering...

Maybe we can just wrap the error so that people can still check, but provide our own error message?

interma · 2018-09-10T09:30:05Z

@DirectXMan12
Yes, wrap error is better and I modified error message:

$ make run
go generate ./pkg/... ./cmd/...
go fmt ./pkg/... ./cmd/...
go vet ./pkg/... ./cmd/...
go run ./cmd/manager/main.go
2018/09/10 17:25:27 Registering Components.
2018/09/10 17:25:27 if {core.hawq.org MyResource} is a CRD, should install it before calling Start: no matches for kind "MyResource" in version "core.hawq.org/v1alpha1"
exit status 1
make: *** [run] Error 1

PTAL, thanks.

DirectXMan12 · 2018-09-10T15:46:05Z

@droot @pwittrock @mengqiy we should decide if we want something like errors.Wrap in our codebase -- it's not a terrible idea, but it's probably something we should explicitly decide.

In the mean time, if we want to move this forward, we might just want to log a warning at V(1) instead of modifying the error. Otherwise, we can just wait to make that decision.

mengqiy · 2018-09-11T05:20:45Z

Gopkg.lock

@@ -3,96 +3,75 @@

 [[projects]]
  branch = "default"
-  digest = "1:24df057f15e7a09e75c1241cbe6f6590fd3eac9804f1110b02efade3214f042d"


I bet you were using an older version of dep.
Please upgrade it and rerun it, it should not remove these digest lines.

Thanks @mengqiy You are right.

before:

$ dep version dep: version : devel build date : git hash : go version : go1.10.3 go compiler : gc platform : darwin/amd64

after upgrade:

$ dep version dep: version : v0.5.0 build date : 2018-07-26 git hash : 224a564 go version : go1.10.3 go compiler : gc platform : darwin/amd64 features : ImportDuringSolve=false

mengqiy · 2018-09-11T05:22:16Z

Gopkg.toml

@@ -20,6 +20,7 @@ required = ["sigs.k8s.io/testing_frameworks/integration",
            "github.com/go-openapi/spec",
            "k8s.io/kube-openapi/pkg/common",
            "k8s.io/apiextensions-apiserver",
+			"github.com/pkg/errors",


nit: please make it well indented :)

mengqiy · 2018-09-11T05:26:47Z

errors.Wrap

@DirectXMan12 I don't think it's a bad idea. What is your concern if we introduce this into our codebase?

mengqiy · 2018-09-11T18:00:24Z

@interma It seems you still have a conflict in Gopkg.lock.

k8s-ci-robot · 2018-09-11T18:00:27Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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. I understand the commands that are listed here.

interma · 2018-09-12T10:51:04Z

Yes, @mengqiy
Looks I need to rebase from master.
I will fix them after resigning my CLA as an employee.

Thanks for your review.

interma · 2018-09-21T02:20:24Z

/check-cla

interma · 2018-09-21T06:44:33Z

@mengqiy
Rebased and no conflict now.

@DirectXMan12 @droot
PTAL, thanks.

droot · 2018-09-21T17:26:02Z

@interma change is looking good except the introduction of errors pkg.

Not super sure in introducing errors pkg. One thing I have observed with errors pkg is, you need to have a consistent scheme for wrapping, otherwise you end up with weird looking msg at the top of the stack. For ex. whether to include pkg-name at the start "controller:", not repeating "error" in the content (can cause stuttering) etc.

I would suggest if we can make this change without introducing errors pkg, it will be easier to move forward. Since in this case, the caller program pretty much exits if they are unable to setup the watches, we are even fine with logging at error level instead warning.

/cc @DirectXMan12

interma · 2018-09-25T07:51:31Z

Thanks for your review. @droot
Wait for @DirectXMan12 opinion.

DirectXMan12 · 2018-09-25T15:22:00Z

agree with @droot

interma · 2018-09-26T03:15:41Z

Thanks @DirectXMan12 @droot
A little confused:
You mean I don't need to wrap the error and just refine error message here (like I did before)
or use another way to wrap error (but I don't know how to do it...) ?

droot · 2018-09-26T18:03:34Z

You mean I don't need to wrap the error and just refine error message here (like I did before)
or use another way to wrap error (but I don't know how to do it...) ?

Sorry I wasn't clear about the change:
I was suggesting to log the message at error level (so that users gets the issue) and avoiding the wrapping the error for now (because we don't want to lose the original error).

interma · 2018-09-27T09:26:29Z

@droot got it.
I committed again, PTAL.

Thanks.

DirectXMan12

minor nit, otherwise good to go.

DirectXMan12 · 2018-09-27T19:45:06Z

pkg/source/source.go

@@ -80,6 +84,11 @@ func (ks *Kind) Start(handler handler.EventHandler, queue workqueue.RateLimiting
 	// Lookup the Informer from the Cache and add an EventHandler which populates the Queue
 	i, err := ks.cache.GetInformer(ks.Type)
 	if err != nil {
+		switch err.(type) {


no need to for the one item switch here --

if kindMatchErr, ok := err.(*meta.NoKindMatchError); ok { log.Error(err, /* rest of message goes here */) } return err

interma · 2018-09-28T04:20:56Z

@DirectXMan12 @droot
All fixed and rebased.

There's a very small PR, but thanks you guys for helping me improve the code!

DirectXMan12 · 2018-10-03T15:35:19Z

/approve
/lgtm

thanks for your contribution, and for your patience!

k8s-ci-robot · 2018-10-03T15:35:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, interma

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:

~~OWNERS~~ [DirectXMan12]

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

Prompt to install CRD when Kind no found

Add / Update GitBook *Test* and *Docs* chapters

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 31, 2018

k8s-ci-robot requested review from DirectXMan12 and droot August 31, 2018 02:41

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 31, 2018

This was referenced Aug 31, 2018

Prompt to install CRD manually when Kind no found kubernetes-sigs/controller-tools#57

Closed

With no CRDs installed, the controller fails with confusing error message kubernetes-sigs/kubebuilder#337

Closed

DirectXMan12 suggested changes Sep 4, 2018

View reviewed changes

interma force-pushed the prompt_nocrd branch from b32eec8 to 05e5ab2 Compare September 6, 2018 03:21

interma force-pushed the prompt_nocrd branch from b1ecf6a to cd4ab3c Compare September 10, 2018 09:24

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 10, 2018

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 11, 2018

mengqiy reviewed Sep 11, 2018

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 11, 2018

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 11, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 21, 2018

interma force-pushed the prompt_nocrd branch from 42f8b4f to 63d0bbb Compare September 21, 2018 06:42

k8s-ci-robot requested a review from DirectXMan12 September 21, 2018 17:26

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 27, 2018

DirectXMan12 suggested changes Sep 27, 2018

View reviewed changes

Prompt to install CRD when Kind no found

919948d

interma force-pushed the prompt_nocrd branch from 1f4f6fd to 919948d Compare September 28, 2018 04:18

k8s-ci-robot assigned DirectXMan12 Oct 3, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2018

DirectXMan12 approved these changes Oct 3, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2018

k8s-ci-robot merged commit ddf0390 into kubernetes-sigs:master Oct 3, 2018

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#129 from interma/prompt_nocrd

f723dda

Prompt to install CRD when Kind no found

interma mentioned this pull request Dec 12, 2018

REQUEST: New membership for interma kubernetes/org#301

Closed

6 tasks

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #129 from pwittrock/gitbook-original

705362c

Add / Update GitBook *Test* and *Docs* chapters

Prompt to install CRD when Kind no found #129

Prompt to install CRD when Kind no found #129

Uh oh!

Conversation

interma commented Aug 31, 2018

Uh oh!

DirectXMan12 Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

interma Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 commented Sep 5, 2018

Uh oh!

interma commented Sep 6, 2018

Uh oh!

DirectXMan12 commented Sep 6, 2018

Uh oh!

interma commented Sep 10, 2018

Uh oh!

DirectXMan12 commented Sep 10, 2018

Uh oh!

mengqiy Sep 11, 2018

Choose a reason for hiding this comment

Uh oh!

interma Sep 11, 2018

Choose a reason for hiding this comment

Uh oh!

mengqiy Sep 11, 2018

Choose a reason for hiding this comment

Uh oh!

mengqiy commented Sep 11, 2018

Uh oh!

mengqiy commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Sep 11, 2018

Uh oh!

interma commented Sep 12, 2018

Uh oh!

interma commented Sep 21, 2018

Uh oh!

interma commented Sep 21, 2018

Uh oh!

droot commented Sep 21, 2018

Uh oh!

interma commented Sep 25, 2018

Uh oh!

DirectXMan12 commented Sep 25, 2018

Uh oh!

interma commented Sep 26, 2018

Uh oh!

droot commented Sep 26, 2018

Uh oh!

interma commented Sep 27, 2018

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

interma Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

interma commented Sep 28, 2018

Uh oh!

DirectXMan12 commented Oct 3, 2018

Uh oh!

k8s-ci-robot commented Oct 3, 2018

Uh oh!

Uh oh!

mengqiy commented Sep 11, 2018 •

edited

Loading