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

store: add a mechanism to summarize changes #4276

Merged
merged 1 commit into from Mar 8, 2021
Merged

Conversation

nicks
Copy link
Member

@nicks nicks commented Mar 4, 2021

Hello @maiamcc, @landism, @milas,

Please review the following commits I made in branch nicks/summarize:

322e263 (2021-03-04 18:20:51 -0500)
store: add a mechanism to summarize changes
The goal of this is to help us move more towards a system like
Kubernetes (https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.8.2/pkg/reconcile#Request)
where the system tells us which object is changing, rather
than writing lots of code to figure it out on our own.

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@nicks nicks requested review from landism, maiamcc and milas March 4, 2021 23:23
Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

This looks fine to me, but I do wonder if given the goal is

the system tells us which object is changing

Would it be easier to have something like a TypedAction interface that has Types() []schema.GroupVersionKind (could also restrict to 1x type) and then just pass the list of kinds to OnChange (which would be nil for any "legacy" events that don't implement the iface)? (Alternatively, could also have TypedSubscriber interface which also provides a list of kinds it cares about so that the notifier could handle the filtering.)

The goal of this is to help us move more towards a system like
Kubernetes (https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.8.2/pkg/reconcile#Request)
where the system tells us which object is changing, rather
than writing lots of code to figure it out on our own.
@nicks
Copy link
Member Author

nicks commented Mar 8, 2021

Ya, that might be where this object ends up - with a list of types. But I'm pretty sure it's going to need to be more complex than that. For example,

  • actions can be batched together, and there are cases where you'll need to know if there are ANY legacy actions
  • the Reconcile() function tends to expect the name of the object that changed

@nicks nicks merged commit 27d3b78 into master Mar 8, 2021
@nicks nicks deleted the nicks/summarize branch March 8, 2021 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants