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

Converge on a consistent controller design #269

Closed
jbw976 opened this issue Jan 2, 2019 · 8 comments
Closed

Converge on a consistent controller design #269

jbw976 opened this issue Jan 2, 2019 · 8 comments
Assignees
Labels
design engineering Improvements to engineering process, hygiene, code quality, etc. reliability techdebt Technical debt accumulated from prioritizing speed over quality test
Milestone

Comments

@jbw976
Copy link
Member

jbw976 commented Jan 2, 2019

This will serve as an epic or umbrella issue for all the issues having to do with figuring out and implementing a consistent pattern and/or best practices for our controllers.

We have some fragmentation across the codebase as different controllers were authored by different developers, so we should converge on a single approach and have consistency across the codebase.

@jbw976 jbw976 added reliability design engineering Improvements to engineering process, hygiene, code quality, etc. test labels Jan 2, 2019
@jbw976 jbw976 modified the milestones: v0.2, v0.3 Jan 18, 2019
@negz
Copy link
Member

negz commented Jan 24, 2019

$ go test github.com/crossplaneio/crossplane/pkg/controller/gcp/compute -run "^(TestReconcile)$"                                                  
ok      github.com/crossplaneio/crossplane/pkg/controller/gcp/compute   0.023s [no tests to run]                                                                           

$ go test github.com/crossplaneio/crossplane/pkg/controller/aws/compute -run "^(TestReconcile)$"                                                  
ok      github.com/crossplaneio/crossplane/pkg/controller/aws/compute   0.025s [no tests to run]                                                                           

$ go test -timeout 30s github.com/crossplaneio/crossplane/pkg/controller/azure/compute -run "^(TestReconcile)$"                                   
2019/01/24 12:10:06 fork/exec /usr/local/kubebuilder/bin/etcd: no such file or directory                                                                                   
FAIL    github.com/crossplaneio/crossplane/pkg/controller/azure/compute 0.031s

I notice the Azure controller has a dependency on etcd from Kubebuilder. I considered raising an issue for this, but it seems like a subset of this issue.

@ichekrygin
Copy link
Member

@negz, you're correct, it is a subset. We should update (rewrite) controller unit test to follow "new" paradigm, i.e. w/out etcd + kubeapiserver involvement. For example: https://github.com/crossplaneio/crossplane/tree/master/pkg/controller/gcp/compute

@negz negz added the techdebt Technical debt accumulated from prioritizing speed over quality label Jun 3, 2019
@negz
Copy link
Member

negz commented Jun 3, 2019

https://github.com/crossplaneio/crossplane/blob/7b6097/design/reconciler-patterns.md

We've iterated once or twice on the design @jbw976 mentions in this issue, with the latest iteration being captured in the above design document. This issue is still very valid, but I believe we need some more work to reach consensus on a design before we proceed with refactoring.

@negz
Copy link
Member

negz commented Jun 3, 2019

Relates to #447

@negz
Copy link
Member

negz commented Jun 3, 2019

Relates to #298

@jbw976
Copy link
Member Author

jbw976 commented Jun 4, 2019

Relates to #510

@infinitecompute infinitecompute added Epic and removed Epic labels Jun 5, 2019
@jbw976 jbw976 changed the title Converge on a consistent controller design for testing Converge on a consistent controller design Jun 10, 2019
@jbw976 jbw976 added the epic label Jun 10, 2019
@muvaf
Copy link
Member

muvaf commented Aug 1, 2019

The generic managed reconciler implemented in #603 is decided to be used for all controllers as can be seen here #615 , #616 and #617 .

@jbw976 We can keep this open to signal that it's an ongoing effort or close it as we do have 1 pattern now for (almost) all controllers.

@jbw976
Copy link
Member Author

jbw976 commented Aug 1, 2019

My take is that with the new general managed reconciler now in master, that has captured the intent of this issue. We are tracking through the provider specific epics the remaining work to update all of the controllers to this new pattern.

Any documentation/guide work around how to use this controller and how to build high quality controllers/reconcilers for partners and contributors will be covered by #610. I'd like to close this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design engineering Improvements to engineering process, hygiene, code quality, etc. reliability techdebt Technical debt accumulated from prioritizing speed over quality test
Projects
None yet
Development

No branches or pull requests

5 participants