Skip to content
This repository has been archived by the owner on Mar 17, 2023. It is now read-only.

Update predicate #17

Closed
wants to merge 1 commit into from
Closed

Update predicate #17

wants to merge 1 commit into from

Conversation

ash2k
Copy link
Contributor

@ash2k ash2k commented Jun 12, 2018

Fixes #16.

@ash2k ash2k added the enhancement New feature or request label Jun 12, 2018
@ash2k ash2k self-assigned this Jun 12, 2018
@ash2k ash2k requested a review from a team June 12, 2018 11:24
@amckague
Copy link
Contributor

Testing?

@@ -22,6 +22,8 @@ type ZapNameField func(name string) zap.Field
type Descriptor struct {
// Group Version Kind of objects a controller can process.
Gvk schema.GroupVersionKind
// IsInterestingUpdate is an optional predicate that is consulted before enqueuing an object on update event.
IsInterestingUpdate func(oldObj, newObj runtime.Object) bool
Copy link
Contributor

Choose a reason for hiding this comment

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

"Interesting" is fun, but I wonder if maybe something like ShouldUpdate() is more descriptive?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was going to comment the exact same thing before I saw your comment on another line 👍

@ash2k
Copy link
Contributor Author

ash2k commented Jun 14, 2018

I've been thinking about an alternative (more flexible) approach where we let the controller constructor attach whatever event handler it wants to use. Approach in this PR does not feel right, it is not generic enough...

@ash2k ash2k closed this Nov 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants