Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refactor: renaming core classes and APIs #646
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: renaming core classes and APIs #646
Changes from 3 commits
d478e35
3b76d5e
5919eac
74920ab
3f92747
fb58685
55b859d
6f07321
444f7d1
8b1be24
424f800
adc543a
1d148ac
9ec3471
910293a
349fc11
2e0ec8b
75b8e0e
8811dc3
224bcdf
94980ee
cadbc8a
0f9eaba
725fb06
b51e69b
0521f8e
4105def
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think that if we do this change, we need to remove the
controller
part completely everywhere and only talk aboutReconciler
because otherwiser it gets too confusing. Like what is aController
is our SDK?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.
Answered above, the controller. Reconciler is just the logic how to reconciler, but this is basically same in controller runtime, the controller is the whole , reconciler is just part of it.
Unfortunate that we don't have a class representing the controller itslef, Maybe to renamae ConfiguredController to Controller?
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.
Maybe this class should be renamed
ConfiguredReconciler
or maybe simplyController
, where for our SDK aController
would be aReconciler
associated with its configuration? In this case, we should also rename theController
annotation to something different, maybeConfiguration
… Then again, the more I think about it, maybe we shouldn't be renaming things too much if there is no obvious benefit from doing so… because otherwise it will mean that we confuse our existing users and we will need to update all our existing documentation / articles…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.
hehe, yes also wrote this above , maybe this should be the
Controller
, and the@Controller
annotation should be@ControllerConfiguration
.I think this class should be our high level aggregate (therefore name it as Controller), so it will contiain the Reconciler, the EventSourceManager, the EventProcessor (former DefaultEventHandler). Basically every aspect of a single controller instance.
The benefit is that we are now closer to go terminology, but on the other hand yes it can be confusing for the users. But getting it right now for v2 is probably the good time to do it "once and for all" :)