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

Decouple webhook and manager packages #190

Closed
adrienjt opened this issue Oct 31, 2018 · 2 comments
Closed

Decouple webhook and manager packages #190

adrienjt opened this issue Oct 31, 2018 · 2 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@adrienjt
Copy link

Context

I'm currently integrating multicluster-controller with controller-runtime's webhook package, which, unfortunately, depends on the manager package.

Multicluster-controller doesn't use controller-runtime's manager package, but a more lightweight version that only orchestrates runnables (caches and controllers), while the cluster dependencies are extracted into multiple Cluster structs.

Problem

Controller-runtime's webhook package depends on the manager.Manager interface, even though it only needs a fraction of it, namely Add(Runnable) for the server, and GetScheme() and GetRESTMapper() for the builder.

I could create an ad-hoc implementation of manager.Manager on my side, with lots of panics in the methods webhook doesn't need, forwarding the three that are actually needed to a multicluster-controller manager and cluster, but I believe there is a cleaner solution, which could also benefit others.

Proposed Solution

Define small interfaces where they're needed. They will be implemented implicitly by controller-runtime's controllerManager struct, and could be implemented by third-party packages.

// in pkg/webhook/server.go
type Manager interface {
	Add(Runnable) error
} 
// in pkg/webhook/admission/builder/builder.go
type Manager interface {
	GetScheme() *runtime.Scheme
	GetRESTMapper() meta.RESTMapper
} 

It should also make testing easier, and would be arguably more idiomatic: https://blog.chewxy.com/2018/03/18/golang-interfaces/

If you agree to this proposed change, I can submit a PR.

@DirectXMan12
Copy link
Contributor

/kind feature
/priority important-longterm

Yeah, I'd be happy to accept a PR fixing this -- it makes sense to me to declare exactly what's needed.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Nov 2, 2018
@DirectXMan12
Copy link
Contributor

mostly solved by #323

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

No branches or pull requests

3 participants