Generate clients, fakeclients, informers and listers for go consumers of cartographer to import

[rawlingsj]

Initial stab and generating clients etc that kube builder doesn't generate. This is just an initial attempt which we can discuss / tidy up/ work on.

/cc @kauana @cirocosta @jsanin

[netlify[bot]]

✔️ Deploy Preview for elated-stonebraker-105904 canceled.

🔨 Explore the source changes: 6c4cae8d3663fa500c74b1f537f8d52b7f870a6d

🔍 Inspect the deploy log: https://app.netlify.com/sites/elated-stonebraker-105904/deploys/61a10ce0c4d0d30007e6a0d2

[github-actions[bot]]

Hello @rawlingsj!, thanks for opening your first Pull Request. Someone will review it soon. Thank you for committing to make Cartographer better

[emmjohnson]

Hey @rawlingsj it looks like the imports got messed up with the changes. It would be nice to revert those changes so we can see the actual changes.

[cirocosta]

also, is it common practice to include the fakes? cartographer itself doesn't have any need for them as is - would it be enough to expose the listers/informers/etc but not these fakes for the custom resources?

[kauana]

@cirocosta Not sure if this is common practice, but this is what Knative does.

[evankanderson]

also, is it common practice to include the fakes? cartographer itself doesn't have any need for them as is - would it be enough to expose the listers/informers/etc but not these fakes for the custom resources?

I think the fakes make it easier for clients to test, rather than for Cartographer itself.

Many of these typed clients are particularly intended for use by other components looking to consume Cartographer. Hopefully, go 1.18's type parameters ("generics") will reduce the need for this sort of boilerplate code generation.

[rawlingsj]

Just force pushed to my branch so that the changes are clearer, also did the folder move outside of goland which avoided the annoying package import changes. There are three separate commits to also help with seeing code changes verses generated changes.

[rawlingsj]

@kauana @jsanin did you have any joy at seeing if the clients branch includes what you need?

[kauana]

Hey @rawlingsj, we haven't tried the new branch yet, but we did learn today that if you are generating clients for cluster scoped resources (such as ClusterSupplyChain) you should have the tag +genclient:nonNamespaced added to your resource type like this example. We were using the listers from this PR and our controller wasn't reconciling property; turns out that's what we were missing.

[rawlingsj]

OK thanks @kauana - I've added that extra annotation it for the cluster resources, could you take a look and try it out when you get a chance please?

I'll rebase the PR and switch to ready for review once confirmed it's working as expected.

[jsanin]

hey @rawlingsj I am having issues trying this out because we are using Knative sample controller as based to create our controllers. It turns out that this project (cartographer) depends on k8s.io/client-go v0.22.3 and this version has a new method in this interface DeploymentInterface. https://github.com/kubernetes/client-go/blob/v0.22.3/kubernetes/typed/apps/v1/deployment.go

ApplyScale(ctx context.Context, deploymentName string, scale *applyconfigurationsautoscalingv1.ScaleApplyConfiguration, opts metav1.ApplyOptions) (*autoscalingv1.Scale, error)

And the current version of knative serving depends on k8s.io/client-go v0.21.4 so I have a compilation error:

# knative.dev/pkg/client/injection/kube/client
vendor/knative.dev/pkg/client/injection/kube/client/client.go:1236:9: cannot use &wrapAppsV1DeploymentImpl{...} (type *wrapAppsV1DeploymentImpl) as type "k8s.io/client-go/kubernetes/typed/apps/v1".DeploymentInterface in return argument:
	*wrapAppsV1DeploymentImpl does not implement "k8s.io/client-go/kubernetes/typed/apps/v1".DeploymentInterface (missing ApplyScale method)

I am not sure how to solve this ... I am still working on it

[rawlingsj]

Yeah - that's the issue with importing informers etc using go dependencies. You should be able to fix client-go versions using a replace in your go.mod file. Alternatively if that becomes a pain you could try avoiding typed informers and use the dynamic Kubernetes client.

[jsanin]

@rawlingsj thanks for the tip about replace. So it now compiles. For future reference this is what we add to our project:

replace (
	github.com/vmware-tanzu/cartographer => github.com/rawlingsj/cartographer v0.0.7-0.20211117083742-bdefc96eef5c
	k8s.io/api => k8s.io/api v0.21.4
	k8s.io/apimachinery => k8s.io/apimachinery v0.21.4
	k8s.io/client-go => k8s.io/client-go v0.21.4
)

we have not been able to test it yet. As the listers, clientset and informers need to be re-generated after adding the // +genclient:nonNamespaced

We still see the cluster supply chain lister as namespaced. I can see that in this interface

type ClusterSupplyChainLister interface {
	// List lists all ClusterSupplyChains in the indexer.
	// Objects returned here must be treated as read-only.
	List(selector labels.Selector) (ret []*v1alpha1.ClusterSupplyChain, err error)
	// ClusterSupplyChains returns an object that can list and get ClusterSupplyChains.
	ClusterSupplyChains(namespace string) ClusterSupplyChainNamespaceLister
	ClusterSupplyChainListerExpansion
}

Will you re-run the code generation and push a new version ? thanks a lot

[squeedee]

@rawlingsj Would love to see a make gen-client target that will generate new client code when the api sources change.

[squeedee]

@rawlingsj A refactor that we've long talked about, to dry up some of the common code in all our controller reconcilers, would greatly benefit from our crd objects implementing client.Object. Seeing as this branch gives us that, I was wondering what the current timeline looks like for getting it out of 'draft' ? What are the blockers and how can I help get us past them?

[rawlingsj]

what the current timeline looks like for getting it out of 'draft' ? What are the blockers and how can I help get us past them?

sounds good - yeah I've pinged @kauana asking for some feedback to confirm the generated clients did what was needed. My plan was once that's confirmed to rebase, (add the make target from comment above), and take it off draft.

So nothing really left to do other than confirming from @kauana or @jsanin their needs are addressed with it

[jsanin]

Hi there. I have been busy getting some things done before the code freeze. I should be able to review this by Monday. Sorry for the delay

[rawlingsj]

Rebased pull request and added the make gen-client target to simplify generating clients, fakes and listers in the future.

Ideally we should probably still wait for @jsanin or @kauana to confirm their use of the clients is working as expected but I'll take off draft while we wait.

[squeedee]

make gen-client fails for me with errors like:

Generating clientset for carto:v1alpha1 at github.com/vmware-tanzu/cartographer/pkg/generated/clientset
F1130 20:11:41.172228    2391 main.go:64] Error: Failed executing generator: some packages had errors:
errors in package "github.com/vmware-tanzu/cartographer/pkg/generated/clientset/versioned":
open ../../../github.com/vmware-tanzu/cartographer/pkg/generated/clientset/versioned/doc.go: no such file or directory
open ../../../github.com/vmware-tanzu/cartographer/pkg/generated/clientset/versioned/clientset.go: no such file or directory

.....

goroutine 1 [running]:
k8s.io/klog/v2.stacks(0x1)
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:1026 +0x8a
k8s.io/klog/v2.(*loggingT).output(0x15cdc20, 0x3, {0x0, 0x0}, 0xc0135f7dc0, 0x0, {0x1448533, 0xc013c61b30}, 0x0, 0x0)
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:975 +0x63d
k8s.io/klog/v2.(*loggingT).printf(0x6, 0x1376190, {0x0, 0x0}, {0x0, 0x0}, {0x1340b86, 0x9}, {0xc013c61b30, 0x1, ...})
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:753 +0x1e5
k8s.io/klog/v2.Fatalf(...)
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:1514
main.main()
	/Users/vmware/go/pkg/mod/k8s.io/[email protected]/cmd/client-gen/main.go:64 +0x38e

goroutine 6 [chan receive]:
k8s.io/klog/v2.(*loggingT).flushDaemon(0x0)
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:1169 +0x6a
created by k8s.io/klog/v2.init.0
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:420 +0xfb
make: *** [gen-client] Error 255

[kauana]

make gen-client fails for me with errors like:

Generating clientset for carto:v1alpha1 at github.com/vmware-tanzu/cartographer/pkg/generated/clientset
F1130 20:11:41.172228    2391 main.go:64] Error: Failed executing generator: some packages had errors:
errors in package "github.com/vmware-tanzu/cartographer/pkg/generated/clientset/versioned":
open ../../../github.com/vmware-tanzu/cartographer/pkg/generated/clientset/versioned/doc.go: no such file or directory
open ../../../github.com/vmware-tanzu/cartographer/pkg/generated/clientset/versioned/clientset.go: no such file or directory

.....

goroutine 1 [running]:
k8s.io/klog/v2.stacks(0x1)
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:1026 +0x8a
k8s.io/klog/v2.(*loggingT).output(0x15cdc20, 0x3, {0x0, 0x0}, 0xc0135f7dc0, 0x0, {0x1448533, 0xc013c61b30}, 0x0, 0x0)
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:975 +0x63d
k8s.io/klog/v2.(*loggingT).printf(0x6, 0x1376190, {0x0, 0x0}, {0x0, 0x0}, {0x1340b86, 0x9}, {0xc013c61b30, 0x1, ...})
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:753 +0x1e5
k8s.io/klog/v2.Fatalf(...)
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:1514
main.main()
	/Users/vmware/go/pkg/mod/k8s.io/[email protected]/cmd/client-gen/main.go:64 +0x38e

goroutine 6 [chan receive]:
k8s.io/klog/v2.(*loggingT).flushDaemon(0x0)
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:1169 +0x6a
created by k8s.io/klog/v2.init.0
	/Users/vmware/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:420 +0xfb
make: *** [gen-client] Error 255

That's the exact same error I got when I try running it

[suarezjulian]

This is actually something we are now doing ourselves in our projects ( only client-gen though) in our team. It will be great if we can get this merged to use this instead

[scothis]

This is an unpopular opinion, but depending on foreign types and clients for k8s resources is "considered harmful".

There are frequent (about once a year) upstream api breakages in k8s that force all consumers to be either before the break or after the break. You cannot mix. Depending on those foreign types/clients also means you depend on their transitive dependencies. If there's a bug that's fixed on one side of the break, but you also depend on a component that hasn't updated, tough.

A pattern that works for me is to copy the structs for types you care about (or reverse them from a structural schema) and use the controller-runtime client.

[squeedee]

@rawlingsj can we take @scothis last piece of advise, and in doing so close this PR as wont-merge?