-
Notifications
You must be signed in to change notification settings - Fork 331
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
Sketch out an alternate way of injecting clients/informers #2210
Conversation
edbc6c3
to
8d820d8
Compare
Codecov Report
@@ Coverage Diff @@
## main #2210 +/- ##
==========================================
- Coverage 65.99% 63.91% -2.09%
==========================================
Files 220 220
Lines 9232 9529 +297
==========================================
- Hits 6093 6090 -3
- Misses 2869 3168 +299
- Partials 270 271 +1
Continue to review full report at Codecov.
|
8d820d8
to
87675d7
Compare
The eventing failure is real. For packages like |
87675d7
to
b24f589
Compare
I believe the other two issues should be resolved by:
Where these repos were using older K8s clients. |
They were not, but it turns out their funniness is a result of the dance we're doing around kubebuilder layouts downstream. I believe these should now be fixed by:
🤞 |
b24f589
to
376c5b0
Compare
Woot, finally clean (failure is the coverage report, which always seems to fail)! |
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 like the direction, a couple nits.
/approve
@@ -124,4 +175,49 @@ func Get(ctx {{.contextContext|raw}}) {{.informersTypedInformer|raw}} { | |||
} | |||
return untyped.({{.informersTypedInformer|raw}}) | |||
} | |||
|
|||
type wrapper struct { | |||
client {{.clientSetInterface|raw}} |
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.
client {{.clientSetInterface|raw}} | |
client {{ .clientSetInterface|raw }} |
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.
nit, half the file does that way and half does not add spaces, so idk, I don't care too much.
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.
Yeah, the squished way was making my eyes bleed. Perhaps we should follow up with a formatting pass?
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, n3wscott 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 |
…ion. This also includes a number of manually created files of the form `*_expansion.go`, which are needed to satisfy some of the Kubernetes manual type "expansions". At present, these are all simply `panic("NYI")`, but may become more than that in the future.
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 incorporated the feedback as a separate commit (in the middle) and there were no changes when codegen ran. 👍
@@ -124,4 +175,49 @@ func Get(ctx {{.contextContext|raw}}) {{.informersTypedInformer|raw}} { | |||
} | |||
return untyped.({{.informersTypedInformer|raw}}) | |||
} | |||
|
|||
type wrapper struct { | |||
client {{.clientSetInterface|raw}} |
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.
Yeah, the squished way was making my eyes bleed. Perhaps we should follow up with a formatting pass?
376c5b0
to
7240a38
Compare
I think this is real interesting and could be used to solve the heavy mix of duck type and strongly typed clients in reconcilers, and our fun and hard to find cache mixing bugs. /lgtm |
One of the long-standing motivations for the injection-based controller architecture has been to enable us to synthesize different backing implementations, which can be made available to controller constructors by substituting what
foo.Get(ctx)
accesses.In this change I have implemented codegen that sets up a new
injection.Dynamic
with an alternate implementation of clients and listers all backed byk8s.io/client-go/dynamic.Interface
. This is currently not usable viaknative.dev/pkg/injection/sharedmain
, but can be used directly by replacinginjection.Default.SetupInformers
with:The first commit has the changes to
injection/
andcodegen/
to support this (including the manually craftednamespacedkube
implementation for secrets). The second commit has the results of code-generation, and a handful of*_expansion.go
files needed to fully satisfykubernetes.Interface
.As far as testing this... with what's here, we can generate a build-able
kubernetes.Interface
, I have also been playing with testing this in a downstream tekton repository, and the main gap running e2e testing is some of the event-handling logic, which lives inclient_expansion.go
and is currently NYI (I left all*_expansion.go
skeletal to try to keep the scope slightly narrower)./kind feature
/assign @n3wscott