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

[Feat] Merge Watch & Local events. Switch by uid & type grouping. #616

Merged

Conversation

Crespalvaro
Copy link
Contributor

Watcher and Local events are now merged to produce a sequence containing all captured events instead of just the sequence that was last produced. Events will still be switch but only at group level, grouping by resource Uid and sub-grouping by event type, therefore Keeping alway last event for each type of each resources, preventing events accumulation.

Fixes #585 #579

@Crespalvaro
Copy link
Contributor Author

These changes were tested on a private prod like cluster. Performed tests are the following:

  • Controller with single resource entity
  • Controller with multiple resource entities
  • Ensure Watcher recreation in all cases does not accumulate reconcile events
  • Ensure Spec/Status changes and their corresponding Reconcile/StatusModified events are treated correctly without breaking reconcile timed requeue

Watcher and Local events are now merged to produce a sequence containing
all captured events instead of just the sequence that was last produced.
Events will still be switch but only at group level, grouping by
resource Uid and sub-grouping by event type, therefore Keeping alway
last event for each type of each resources, preventing events
accumulation.
@Crespalvaro Crespalvaro force-pushed the feature/merge-watcher-localevents branch from fdc810d to a362ba5 Compare September 28, 2023 07:06
@buehler
Copy link
Owner

buehler commented Sep 28, 2023

Thank you @Crespalvaro for the contribution!

When I'm more or less ready with v8 (on the default branch), I very much like your opinion on it :-)

@buehler
Copy link
Owner

buehler commented Sep 28, 2023

Whoops, there are some errors in testing, can you please fix these and commit again? The tests should automatically run as far as I know.

@Crespalvaro
Copy link
Contributor Author

@buehler You can count with it! Ill give it a check once you have it ready.

Apologizes, I did not update Test, let me put my hands on it

@Crespalvaro
Copy link
Contributor Author

Crespalvaro commented Sep 28, 2023

Should be good now. I believe it stills requires your approval @buehler

@buehler buehler merged commit d6031c6 into buehler:release Sep 29, 2023
5 checks passed
@buehler
Copy link
Owner

buehler commented Oct 11, 2023

@Crespalvaro if you have some spare time, you can test the v8 version now.
The operators do work and are integration-tested. However, webhook-wise, only validation webhooks are supported. I need to get my head around the other two variants (mutation and conversion hooks).

There is a general usage documentation in the respective readmes. The Generator package can provide you with convenience functions.

@Crespalvaro
Copy link
Contributor Author

@buehler Thanks for letting me know. I'm right about to finish a big delivery for my project, therefore I expect to have some spare time to dedicate to such testing soon. I'll be coming back to you with my feedback

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.

2 participants