Skip to content
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

How should I inject other dependencies into reconcilers? #102

Closed
grantr opened this issue Aug 3, 2018 · 3 comments
Closed

How should I inject other dependencies into reconcilers? #102

grantr opened this issue Aug 3, 2018 · 3 comments

Comments

@grantr
Copy link
Contributor

grantr commented Aug 3, 2018

Is there a plan to allow the inject interfaces to inject arbitrary dependencies? Or maybe there's a better way to do this?

Concrete use case: I want reconcilers to have access to a logger instance, but it's not a logr logger so I can't use the log promise feature, and I'd like to avoid relying on package variables if possible. Also, I'd like each controller to have the same initialization signature ProvideController(manager.Manager, <something>) (and I acknowledge this might be a silly requirement).

@droot maybe you have some context on plans for inject.

@DirectXMan12
Copy link
Contributor

🚂 🚋 🚋 <-- the dependency injection train

At some point during the design of this framework, we had a discussion about dependency injection, and the upshot was that we were really hesitant about adding full-scale generic dependency injection, and decided to avoid it, and have a couple tightly scoped cases inside the library that didn't leak out (with the exception of InjectClient). I'm (so far) still on board with that sentiment.

My initial suggestion would just be to pass the logger in at object construction. If that's not possible for whatever reason, you could totally implement something similar to what we do internally -- things that need a Foo implement an InjectFoo interface, and then the code that has a Foo and a handle to those things checks each thing to see if it implements InjectFoo, and if it does, it calls that. Can you provide a bit more information on your particular usecase? Maybe some real or mock code that you're trying to achieve?

@grantr
Copy link
Contributor Author

grantr commented Aug 3, 2018

Thanks for the background, I think the instinct to be wary of dependency injection is reasonable. Sounds like it's essentially an internal implementation detail. Since it's possible to call mrg.GetClient() when creating a controller, is there really a need to expose InjectClient? Maybe we (as users of the library) should drop our usage of that.

@DirectXMan12
Copy link
Contributor

DirectXMan12 commented Aug 6, 2018

I think the goal of that was probably to allow users to not have to write constructors -- with the Inject method, it's easy to just have a struct that contains your options as fields. It's a nice feature of the code, since you maintain readability by having field names matching with the values (you should see some of the constructors in kubernetes with identical parameter types next to each other and no indication of what they are when reading the calling code). You can also achieve that with an "options struct" pattern, mostly.

@grantr grantr closed this as completed Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants