-
Notifications
You must be signed in to change notification settings - Fork 208
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
fix: enable kube events for hooks with namespace.labelSelector bindings #411
Conversation
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'd prefer strict assertions.
// Wait until informer for new NS appears. | ||
g.Eventually(func() bool { | ||
monImpl := mon.(*monitor) | ||
return len(monImpl.VaryingInformers) == 3 |
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.
Counting seems good enough since this behavior looks deterministic. But I suggest to be more strict and test that VaryingInformers
actually contains key "test-ns-2"
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've rewrite this test.
// Wait until informer appears. | ||
g.Eventually(func() bool { | ||
monImpl := mon.(*monitor) | ||
return len(monImpl.VaryingInformers) == 2 |
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.
Counting seems good enough since this behavior looks deterministic. But I suggest to be more strict and test that VaryingInformers
actually contains expected keys
|
||
// Should update snapshot. | ||
g.Eventually(func() bool { | ||
return len(mon.Snapshot()) == 4 |
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 would prefer to find exact object reference in snapshots. We probably don't case about the amount, but we do care about the inclusion of the configmap. What if there is a bug that copies snap[0]
all the time orgrows the slice with nils?
Signed-off-by: Ivan Mikheykin <ivan.mikheykin@flant.com>
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.
Lgtm
Overview
Add flag to enable KubeEvents for varying informers after Synchronization phase.
What this PR does / why we need it
See #379 and #399. This fix enables events for hooks with namespace.labelSelector.
Special notes for your reviewer
"before" situation can be emulated by commenting
m.eventsEnabled = true
in monitor.go. Test will fail:Does this PR introduce a user-facing change?