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.

Sign up for GitHub

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

Jump to bottom

Refactor reconcilers and implementations #250

Merged
merged 13 commits into from
Oct 14, 2022
Merged

Refactor reconcilers and implementations #250

merged 13 commits into from
Oct 14, 2022
+2,381 −1,158

Conversation

pleshakov
Copy link
Contributor

Previously, all reconcilers and implementations looked almost the same.

This PR:

  • Replaces all reconcilers with a single one.
  • Removes all implementations. Now the reconciler sends upsert and delete events to the event channel instead of the implementations. The sending to the events part of the reconciler doesn't have the shutdown bug (a possibility to hang while sending to the channel during the shutdown) all implementations shared.
  • Removes the sdk package.

Additionally, the Manager now uses the same logger as the other components.

@github-actions github-actions bot added the chore Pull requests for routine tasks label Sep 23, 2022
@pleshakov pleshakov added the bug Something isn't working label Sep 23, 2022
@pleshakov pleshakov force-pushed the chore/refactor-implementations-and-controllers branch from 05f549b to bd92697 Compare September 23, 2022 23:06
@github-actions github-actions bot added dependencies Pull requests that update a dependency file and removed bug Something isn't working labels Sep 23, 2022
@pleshakov pleshakov force-pushed the chore/refactor-implementations-and-controllers branch from 966c5f4 to c6e289f Compare September 26, 2022 15:26
@pleshakov pleshakov added the bug Something isn't working label Sep 26, 2022
kate-osborn
kate-osborn requested changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 🔥

Really nice work! This is a huge improvement over what we had before.

Just a couple of small comments and questions.

internal/controller/controller_suit_test.go Outdated Show resolved Hide resolved
internal/controller/reconciler.go Outdated Show resolved Hide resolved
internal/controller/reconciler_test.go Outdated Show resolved Hide resolved
internal/controller/reconciler_test.go Outdated Show resolved Hide resolved
internal/controller/reconciler_test.go Outdated Show resolved Hide resolved
internal/controller/reconciler_test.go Outdated Show resolved Hide resolved
internal/controller/register.go Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the bug Something isn't working label Sep 28, 2022
@pleshakov pleshakov force-pushed the chore/refactor-implementations-and-controllers branch 4 times, most recently from a594e1a to 6a252c1 Compare October 5, 2022 18:17
@pleshakov
Copy link
Contributor Author

Changes from the previous version:

  • reconciler.Reconciler was renamed to reconciler.Implementation
  • changes from Deduplicate endpoints #253 were incorporated into the refactor -- manager package now includes index package (for indices) and predicate (for predicates)
  • tests were added to test controller registration

@pleshakov pleshakov added the bug Something isn't working label Oct 5, 2022
kate-osborn
kate-osborn requested changes Oct 6, 2022
View reviewed changes
internal/manager/controllers.go Outdated Show resolved Hide resolved
internal/manager/controllers.go Outdated Show resolved Hide resolved
internal/manager/controllers.go Outdated Show resolved Hide resolved
internal/manager/controllers.go Show resolved Hide resolved
internal/manager/controllers.go Outdated Show resolved Hide resolved
internal/manager/controllers.go Outdated Show resolved Hide resolved
internal/manager/controllers_test.go Outdated Show resolved Hide resolved
internal/manager/manager.go Outdated Show resolved Hide resolved
internal/manager/manager.go Outdated Show resolved Hide resolved
@pleshakov pleshakov force-pushed the chore/refactor-implementations-and-controllers branch from 6a252c1 to 2d818d6 Compare October 7, 2022 22:15
@github-actions github-actions bot removed the bug Something isn't working label Oct 7, 2022
kate-osborn
kate-osborn reviewed Oct 10, 2022
View reviewed changes
internal/manager/controllers.go Outdated Show resolved Hide resolved
internal/manager/controllers.go Outdated Show resolved Hide resolved
internal/manager/index/endpointslice.go Show resolved Hide resolved
kate-osborn
kate-osborn approved these changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@pleshakov pleshakov added the bug Something isn't working label Oct 12, 2022
@github-actions github-actions bot removed the bug Something isn't working label Oct 14, 2022
@pleshakov
Refactor reconcilers and implementations
e5b21b0 
Previously, all reconcilers and implementations looked almost the same.

This commit:
- Replaces all reconcilers with a single one.
- Removes all implementations. Now the reconciler sends upsert and
delete events to the event channel instead of the implementations.
The sending to the events part of the reconciler doesn't have
the shutdown bug (a possibility to hang while sending to the channel
during the shutdown) all implementations shared.
- Removes the sdk package.

Additionally, the Manager now uses the same logger as the other
components.
@pleshakov
Update deps
1b3cba0
@pleshakov
rename extractValue to indexFunc
1ef8263
@pleshakov
Rename prepareIndex to addIndex
4c0626f
@pleshakov
Simplify error message
4cbf962
@pleshakov
Add the filter package
67097d6
@pleshakov
Rename k8sEventFilter to k8sPredicate
71fea1f
@pleshakov
Refactor EndpointSliceFieldIndexes
b2bfe8d
@pleshakov
Clarify
13dae5b
@pleshakov
Add a comment
0a67e50
@pleshakov
Introduce options
9718f69
@pleshakov
Clarify why not deep copy
d1cf071
@pleshakov pleshakov force-pushed the chore/refactor-implementations-and-controllers branch from 53b9cb8 to d1cf071 Compare October 14, 2022 16:55
@pleshakov pleshakov added the bug Something isn't working label Oct 14, 2022
@pleshakov pleshakov merged commit 7328fee into main Oct 14, 2022
@pleshakov pleshakov deleted the chore/refactor-implementations-and-controllers branch October 14, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kate-osborn kate-osborn kate-osborn approved these changes

@f5yacobucci f5yacobucci f5yacobucci left review comments

Assignees
No one assigned
Labels
bug Something isn't working chore Pull requests for routine tasks dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pleshakov @f5yacobucci @kate-osborn