✨Add predicates as variadic args for Owns, For, and Watches func #602

mszostok · 2019-09-12T15:49:54Z

Description

This PR adds options for defining the Predicate. Watches, Owns, For, and Watches
Add tests coverage for implemented logic using ginko & gomega

Fixes: #572

mszostok · 2019-09-24T12:30:21Z

Discussed with @DirectXMan12 and he has right,
basically it's not a breaking change if someone is using those methods directly, but if someone had func like

DoAThing(cb func(runtime.Obj) *ctrl.Builder)

then the compiler will throw an error.

Because of that, this will be postponed

DirectXMan12 · 2019-12-03T02:07:18Z

we'll roll this in to the next set of breaking changes. I'll add a milestone or somesuch

vincepri · 2020-02-05T16:35:21Z

@mszostok Are you still interested / do you have time to work on this?

mszostok · 2020-02-05T16:43:07Z

Hi @vincepri

Yes, if you are interested in having that functionality then I can fix it. You have some deadline? Because I can do it even tomorrow:)

vincepri · 2020-02-05T16:44:30Z

@mszostok Either this week / next week would be great to have this merged :) we're trying to get to a release soon-ish. Thank you for your help!

gerred · 2020-02-05T21:56:49Z

@mszostok If you need a pair on this, I'm available too. 👍 on getting this landed.

DirectXMan12 · 2020-02-05T22:19:12Z

(lmk if you need someone to force-push a rebase here)

k8s-ci-robot · 2020-02-06T12:59:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mszostok
To complete the pull request process, please assign gerred
You can assign the PR to them by writing /assign @gerred in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vincepri · 2020-02-07T15:52:08Z

@mszostok Is this PR ready to be reviewed in more depth? Is it still a draft?

DirectXMan12 · 2020-02-10T21:21:16Z

I'm gonna fix this up. I'm pretty sure this is ready.

DirectXMan12 · 2020-02-10T21:24:00Z

pkg/builder/controller.go

 			IsController: true,
 		}
-		if err := blder.ctrl.Watch(src, hdler, blder.predicates...); err != nil {
+		allPredicates := append(blder.globalPredicates, obj.predicates...)


this needs to be a copy first, otherwise you'll end up growing the list with other objects' predicates.

(or, rather, you could, depending on what Go decides to do with the slice)

you are right I missed it

This allows per-watch predicates for For, Owns, and Watches in the builder. These are in addition to the global predicate set, which apply to all watches.

DirectXMan12 · 2020-02-10T21:35:00Z

Should be good now

DirectXMan12 · 2020-02-10T21:35:59Z

(or rather, it looks good to me)

vincepri · 2020-02-10T21:51:45Z

pkg/builder/controller.go

 	return blder
 }

 // Owns defines types of Objects being *generated* by the ControllerManagedBy, and configures the ControllerManagedBy to respond to
 // create / delete / update events by *reconciling the owner object*.  This is the equivalent of calling
 // Watches(&source.Kind{Type: <ForType-apiType>}, &handler.EnqueueRequestForOwner{OwnerType: apiType, IsController: true})
-func (blder *Builder) Owns(apiType runtime.Object) *Builder {
-	blder.managedObjects = append(blder.managedObjects, apiType)
+func (blder *Builder) Owns(apiType runtime.Object, prct ...predicate.Predicate) *Builder {


@DirectXMan12 Is this the best way to add this functionality? Having varargs here prevents us to add more functionality in the future if we wanted to expose more configurable options.

We could opt instead for a generic interface, similar for what we do for client.List for example.

yep, it is a functional options pattern and it could be better in that case at the beginning I just do not want to make it to complicated because I thought I will not break a contract

vincepri · 2020-02-10T21:57:04Z

pkg/builder/controller.go

+		allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...)
+		allPredicates = append(allPredicates, w.predicates...)


This could be simplified with

allPredicates := append(blder.globalPredicates, w.predicates...)

I was like that at the beginning but @DirectXMan12 is right, it should be copied: #602 (comment)

vincepri · 2020-02-10T21:58:00Z

pkg/builder/controller.go

 // For defines the type of Object being *reconciled*, and configures the ControllerManagedBy to respond to create / delete /
 // update events by *reconciling the object*.
 // This is the equivalent of calling
 // Watches(&source.Kind{Type: apiType}, &handler.EnqueueRequestForObject{})
-func (blder *Builder) For(apiType runtime.Object) *Builder {
-	blder.apiType = apiType
+func (blder *Builder) For(reconciledAPIType runtime.Object, prct ...predicate.Predicate) *Builder {


Suggested change

func (blder *Builder) For(reconciledAPIType runtime.Object, prct ...predicate.Predicate) *Builder {

func (blder *Builder) For(object runtime.Object, predicates ...predicate.Predicate) *Builder {

No need for shorter names

vincepri · 2020-02-10T21:58:10Z

pkg/builder/controller.go

+// simpleWatch represents the information required to construct a "simple" For
+// or Owns watch.
+type simpleWatch struct {
+	item       runtime.Object


Suggested change

item runtime.Object

object runtime.Object

vincepri · 2020-02-10T22:23:24Z

pkg/builder/controller.go

+		allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...)
+		allPredicates = append(allPredicates, obj.predicates...)


This could be simplified with

allPredicates := append(blder.globalPredicates, w.predicates...)

same as here: #602 (comment)

mszostok · 2020-02-13T10:20:09Z

@vincepri @DirectXMan12 should I change it to functional options pattern as described here? #602 (comment)

I can do it today

mszostok · 2020-02-14T16:19:01Z

Hi

@vincepri @DirectXMan12 so I provided implementation with Functional options. Here is PR: #799

I've prepared a dedicated pull request so you can check which option is better.

mszostok · 2020-03-13T19:55:46Z

closing in favor of: #799

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 12, 2019

k8s-ci-robot requested review from droot and pwittrock September 12, 2019 15:50

mszostok force-pushed the myk-watches branch from 1b4ff4d to 3658a3c Compare September 12, 2019 16:05

mszostok changed the title ~~✨Add as predicates as variadic args for Owns, For, and Watches func~~ ✨Add predicates as variadic args for Owns, For, and Watches func Sep 13, 2019

DirectXMan12 added this to the breaking-next milestone Dec 3, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2020

DirectXMan12 assigned gerred Feb 5, 2020

DirectXMan12 assigned vincepri Feb 5, 2020

mszostok force-pushed the myk-watches branch from 3658a3c to 83965e4 Compare February 6, 2020 12:59

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2020

DirectXMan12 reviewed Feb 10, 2020

View reviewed changes

Add per-watch predicates for Owns/For/Watches

4c1d0e4

This allows per-watch predicates for For, Owns, and Watches in the builder. These are in addition to the global predicate set, which apply to all watches.

DirectXMan12 force-pushed the myk-watches branch from 83965e4 to 4c1d0e4 Compare February 10, 2020 21:34

DirectXMan12 marked this pull request as ready for review February 10, 2020 21:35

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 10, 2020

vincepri reviewed Feb 10, 2020

View reviewed changes

mszostok mentioned this pull request Feb 14, 2020

⚠️ Add predicates as variadic args for Owns, For, and Watches func #799

Merged

mszostok closed this Mar 13, 2020

		allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...)
		allPredicates = append(allPredicates, w.predicates...)

	func (blder Builder) For(reconciledAPIType runtime.Object, prct ...predicate.Predicate) Builder {
	func (blder Builder) For(object runtime.Object, predicates ...predicate.Predicate) Builder {

		allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...)
		allPredicates = append(allPredicates, obj.predicates...)

✨Add predicates as variadic args for Owns, For, and Watches func #602

✨Add predicates as variadic args for Owns, For, and Watches func #602

Uh oh!

Conversation

mszostok commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mszostok commented Sep 24, 2019

Uh oh!

DirectXMan12 commented Dec 3, 2019

Uh oh!

vincepri commented Feb 5, 2020

Uh oh!

mszostok commented Feb 5, 2020

Uh oh!

vincepri commented Feb 5, 2020

Uh oh!

gerred commented Feb 5, 2020

Uh oh!

DirectXMan12 commented Feb 5, 2020

Uh oh!

k8s-ci-robot commented Feb 6, 2020

Uh oh!

vincepri commented Feb 7, 2020

Uh oh!

DirectXMan12 commented Feb 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 commented Feb 10, 2020

Uh oh!

DirectXMan12 commented Feb 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mszostok Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mszostok commented Feb 13, 2020

Uh oh!

mszostok commented Feb 14, 2020

Uh oh!

mszostok commented Mar 13, 2020

Uh oh!

Uh oh!

mszostok commented Sep 12, 2019 •

edited

Loading

mszostok Feb 13, 2020 •

edited

Loading