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

✨ lightweight dependency injection #182

Closed
wants to merge 1 commit into from

Conversation

pwittrock
Copy link
Contributor

No description provided.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pwittrock

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 25, 2018
@pwittrock
Copy link
Contributor Author

@DirectXMan12 WDYT?

There are a couple use cases for this:

  1. Injecting client-go generated clients and informers into Controllers for use with source.Informer
  2. Injecting cluster-api actuators into Controllers instead of hard wiring them.

@pwittrock pwittrock force-pushed the injector branch 2 times, most recently from b1157ab to 3838fe0 Compare October 26, 2018 21:51
@pwittrock
Copy link
Contributor Author

@droot thoughts?

@droot
Copy link
Contributor

droot commented Nov 2, 2018

Took a look. In my mind, the primary use-case for dependency injection for a way to provide user-managed-objects like Reconcilers, Sources with the objects that only manager knows about. Injecting deps which belongs completely in the user-managed-space (which is second use-case you mentioned above), not super sure about the advantages. Discuss offline next week ?

@DirectXMan12
Copy link
Contributor

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 2, 2018
@pwittrock
Copy link
Contributor Author

Here is an example of the problem we are trying to solve:

https://github.com/kubernetes-sigs/cluster-api-provider-gcp/blob/master/cmd/manager/main.go#L78

Take a look at how the actuator is passed around.

@droot
Copy link
Contributor

droot commented Nov 6, 2018

@pwittrock Took a look. our current scaffolding project structure isn't flexible enough for people to initialize newReconciler from the entry point main (the way stuff is wired using pkg init()) which is forcing us to solve this problem at the layer below (controller-runtime/manager) which I am not super sure about.

@ironcladlou
Copy link
Contributor

ironcladlou commented Nov 28, 2018

I found it easier to implement inversion of control by just skipping the SDK boilerplate and using controller-runtime as a library (part of openshift/cluster-ingress-operator#70).

In this context, I feel like the underlying problem is not lack of a dependency injection system (i.e. more framework) but instead a more IoC-friendly architecture.

That said, I acknowledge that IoC might not be important to everyone and there are many ways to go about it.

@pwittrock pwittrock changed the title lightweight dependency injection ✨ lightweight dependency injection Dec 19, 2018
@pwittrock pwittrock force-pushed the injector branch 3 times, most recently from 638dd95 to 4dba4c0 Compare December 19, 2018 03:57
@pwittrock
Copy link
Contributor Author

@ironcladlou I mostly agree, though I think we still will need an answer for those who want to work within the framework as structured today.

@pwittrock
Copy link
Contributor Author

PTAL

@pwittrock pwittrock closed this Feb 8, 2019
DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants