-
Notifications
You must be signed in to change notification settings - Fork 1
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
Refactor reconciler #120
Refactor reconciler #120
Conversation
Right now I'm adding at least some simple unit tests to all the steps that are now single functions. After that the changes should be okay to review. |
Add tenant as owner to clusters
With this change the pull methods now work instantly. If a `cluster` is created, it will trigger a `tenant` reconcile via owner reference. The tenant reconcile in turn triggers a gitrepo reconcile via the `EnqueueRequestsFromMapFunc` and watches on both `clusters` and `tenants`.
Some changes to the order of thins make the unit tests fail. Also there were tests that checked the older push method for gitRepo changes. Those were removed.
Finished all the new unittests and fixes for the old ones. This can now be reviewed. Not sure if I should split this up in some smaller PRs though. |
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.
- Keep an eye on the scope of assertions
- Consider to use loops for the pipelines
- Consider to use maps for test cases
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.
All those excessive empty lines makes the code visually appear restless. This is irritating me. I have left some suggestions but then stopped as it started to become excessive.
I suggest to at least drop empty lists after {
and before }
.
I took almost all afternoon to find some very weird and racy issues with the integration tests. After some time I found that our implementation with loops to apply the steps was deeply flawed when using maps. They are not ordered in Go. So the order of the steps was random each time. I changed it to a slice and struct now everything seems to be working properly again. |
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.
The code style of one test file completely goes against what was established within the others. Please align that one with the others.
They are no longer necessary.
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.
LGTM. Thanks for the work.
Disclaimer. I only reviewed coding style and coding structure. I am unable to assert functionality of the code.
Thanks @corvus-ch From the automated reconcile tests and my manual testing the functionality should be okay. |
The reconcile functions are now splitted into smaller logical step functions that can be composed.
These step functions all follow the same interface, so they can be composed at will and accept an interface type for the CRD. There are a few exceptions that will cast the interfaces to concrete objects within the function.
Also each controller now strictly only manipulates its own objects. The git repository objects are still created by other controllers, but changes won't be propagated. Instead the git repositories will now collect the information by themselves.
Relates to: #80
Checklist