Skip to content

Conversation

smira
Copy link
Member

@smira smira commented Sep 1, 2025

This is extracted from #633 as a separate fix.

The idea is that if we merge/deduplicate watch events, we should do this by immutable attributes of the resource: namespace, type, id.

The previous implementation might split two events on e.g. finalizers being empty, so two events which report finalizers empty/finalizers non-empty might be delivered in the wrong order: first empty, next non-empty.

It might not trigger a real bug due to the way controllers work, but still is not consistent and correct.

This is extracted from cosi-project#633 as a separate fix.

The idea is that if we merge/deduplicate watch events, we should do this
by immutable attributes of the resource: namespace, type, id.

The previous implementation might split two events on e.g. finalizers
being empty, so two events which report finalizers empty/finalizers
non-empty might be delivered in the wrong order: first empty, next
non-empty.

It might not trigger a real bug due to the way controllers work, but
still is not consistent and correct.

Signed-off-by: Andrey Smirnov <[email protected]>
@github-project-automation github-project-automation bot moved this to To Do in Planning Sep 1, 2025
@talos-bot talos-bot moved this from To Do to In Review in Planning Sep 1, 2025
@github-project-automation github-project-automation bot moved this from In Review to Approved in Planning Sep 1, 2025
Copy link
Collaborator

@frezbo frezbo left a comment

Choose a reason for hiding this comment

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

no tests?

@smira
Copy link
Member Author

smira commented Sep 1, 2025

no tests?

yes, existing tests test this indirectly, it's the very core part of COSI

@smira
Copy link
Member Author

smira commented Sep 1, 2025

/m

@talos-bot talos-bot merged commit 23b4690 into cosi-project:main Sep 1, 2025
14 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Done in Planning Sep 1, 2025
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.

5 participants