Skip to content

✨ introduce LabelChangedPredicate to trigger event on label change #1315

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 11, 2021
Merged

✨ introduce LabelChangedPredicate to trigger event on label change #1315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dad8159
introduce LabelChangedPredicate to trigger event on label change
mars1024 Jan 4, 2021
52c9e6f
disable duplication linter on predicate tests
mars1024 Jan 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions pkg/predicate/predicate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,36 @@ func (AnnotationChangedPredicate) Update(e event.UpdateEvent) bool {
return !reflect.DeepEqual(e.ObjectNew.GetAnnotations(), e.ObjectOld.GetAnnotations())
}

// LabelChangedPredicate implements a default update predicate function on label change.
//
// This predicate will skip update events that have no change in the object's label.
// It is intended to be used in conjunction with the GenerationChangedPredicate, as in the following example:
//
// Controller.Watch(
// &source.Kind{Type: v1.MyCustomKind},
// &handler.EnqueueRequestForObject{},
// predicate.Or(predicate.GenerationChangedPredicate{}, predicate.LabelChangedPredicate{}))
//
// This will be helpful when object's labels is carrying some extra specification information beyond object's spec,
// and the controller will be triggered if any valid spec change (not only in spec, but also in labels) happens.
type LabelChangedPredicate struct {
Funcs
Copy link
Member

Choose a reason for hiding this comment

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

Can you use Funcs.UpdateFunc rather than overwriting it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, maybe I didn't catch your point, I'm just doing the same way like ResourceVersionChangedPredicate and GenerationChangedPredicate, overriding the Update method and inherit the others.

If we want to assign Funcs.UpdateFunc, another constructing function will be introduced for this, and I think maybe it's not easy for using.

Copy link
Member

Choose a reason for hiding this comment

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

Ah that's how you came up with it. From a user perspective it is just calling a constructor rather than using a type :)

The reason I think it's slightly better is that it might be confusing to have an embedded UpdateFunc described as being called for Update events that does not actually get called for Update events :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) Got your point, but your suggestion is keeping same as the other predicates or change all the predicates to a new way (use a constructor instead of stating a type), but I think the second way maybe a break change for some "legacy" users.
Or maybe we could just use the constructing way for the new LabelChangedPredicate?

}

// Update implements default UpdateEvent filter for checking label change
func (LabelChangedPredicate) Update(e event.UpdateEvent) bool {
if e.ObjectOld == nil {
log.Error(nil, "Update event has no old object to update", "event", e)
return false
}
if e.ObjectNew == nil {
log.Error(nil, "Update event has no new object for update", "event", e)
return false
}

return !reflect.DeepEqual(e.ObjectNew.GetLabels(), e.ObjectOld.GetLabels())
}

// And returns a composite predicate that implements a logical AND of the predicates passed to it.
func And(predicates ...Predicate) Predicate {
return and{predicates}
Expand Down
202 changes: 202 additions & 0 deletions pkg/predicate/predicate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,9 @@ var _ = Describe("Predicate", func() {

})

// AnnotationChangedPredicate has almost identical test cases as LabelChangedPredicates,
// so the duplication linter should be muted on both two test suites.
// nolint:dupl
Describe("When checking an AnnotationChangedPredicate", func() {
instance := predicate.AnnotationChangedPredicate{}
Context("Where the old object is missing", func() {
Expand Down Expand Up @@ -610,6 +613,205 @@ var _ = Describe("Predicate", func() {
})
})

// LabelChangedPredicates has almost identical test cases as AnnotationChangedPredicates,
// so the duplication linter should be muted on both two test suites.
// nolint:dupl
Describe("When checking a LabelChangedPredicate", func() {
instance := predicate.LabelChangedPredicate{}
Context("Where the old object is missing", func() {
It("should return false", func() {
new := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
Labels: map[string]string{
"foo": "bar",
},
}}

evt := event.UpdateEvent{
ObjectNew: new,
}
Expect(instance.Create(event.CreateEvent{})).To(BeTrue())
Expect(instance.Delete(event.DeleteEvent{})).To(BeTrue())
Expect(instance.Generic(event.GenericEvent{})).To(BeTrue())
Expect(instance.Update(evt)).To(BeFalse())
})
})

Context("Where the new object is missing", func() {
It("should return false", func() {
old := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
Labels: map[string]string{
"foo": "bar",
},
}}

evt := event.UpdateEvent{
ObjectOld: old,
}
Expect(instance.Create(event.CreateEvent{})).To(BeTrue())
Expect(instance.Delete(event.DeleteEvent{})).To(BeTrue())
Expect(instance.Generic(event.GenericEvent{})).To(BeTrue())
Expect(instance.Update(evt)).To(BeFalse())
})
})

Context("Where the labels are empty", func() {
It("should return false", func() {
new := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
}}

old := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
}}

evt := event.UpdateEvent{
ObjectOld: old,
ObjectNew: new,
}
Expect(instance.Create(event.CreateEvent{})).To(BeTrue())
Expect(instance.Delete(event.DeleteEvent{})).To(BeTrue())
Expect(instance.Generic(event.GenericEvent{})).To(BeTrue())
Expect(instance.Update(evt)).To(BeFalse())
})
})

Context("Where the labels haven't changed", func() {
It("should return false", func() {
new := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
Labels: map[string]string{
"foo": "bar",
},
}}

old := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
Labels: map[string]string{
"foo": "bar",
},
}}

evt := event.UpdateEvent{
ObjectOld: old,
ObjectNew: new,
}
Expect(instance.Create(event.CreateEvent{})).To(BeTrue())
Expect(instance.Delete(event.DeleteEvent{})).To(BeTrue())
Expect(instance.Generic(event.GenericEvent{})).To(BeTrue())
Expect(instance.Update(evt)).To(BeFalse())
})
})

Context("Where a label value has changed", func() {
It("should return true", func() {
new := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
Labels: map[string]string{
"foo": "bar",
},
}}

old := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
Labels: map[string]string{
"foo": "bee",
},
}}

evt := event.UpdateEvent{
ObjectOld: old,
ObjectNew: new,
}
Expect(instance.Create(event.CreateEvent{})).To(BeTrue())
Expect(instance.Delete(event.DeleteEvent{})).To(BeTrue())
Expect(instance.Generic(event.GenericEvent{})).To(BeTrue())
Expect(instance.Update(evt)).To(BeTrue())
})
})

Context("Where a label has been added", func() {
It("should return true", func() {
new := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
Labels: map[string]string{
"foo": "bar",
},
}}

old := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
Labels: map[string]string{
"foo": "bar",
"faa": "bor",
},
}}

evt := event.UpdateEvent{
ObjectOld: old,
ObjectNew: new,
}
Expect(instance.Create(event.CreateEvent{})).To(BeTrue())
Expect(instance.Delete(event.DeleteEvent{})).To(BeTrue())
Expect(instance.Generic(event.GenericEvent{})).To(BeTrue())
Expect(instance.Update(evt)).To(BeTrue())
})
})

Context("Where a label has been removed", func() {
It("should return true", func() {
new := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
Labels: map[string]string{
"foo": "bar",
"faa": "bor",
},
}}

old := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "baz",
Namespace: "biz",
Labels: map[string]string{
"foo": "bar",
},
}}

evt := event.UpdateEvent{
ObjectOld: old,
ObjectNew: new,
}
Expect(instance.Create(event.CreateEvent{})).To(BeTrue())
Expect(instance.Delete(event.DeleteEvent{})).To(BeTrue())
Expect(instance.Generic(event.GenericEvent{})).To(BeTrue())
Expect(instance.Update(evt)).To(BeTrue())
})
})
})

Context("With a boolean predicate", func() {
funcs := func(pass bool) predicate.Funcs {
return predicate.Funcs{
Expand Down