Closes #4949: Label-based filtering for Helm via predicates #4997

VenkatRamaraju · 2021-06-18T17:52:11Z

Description of the change:
Added a predicate to filter events based on labels.

Motivation for the change:
Allow Helm operators to only watch CRs with specific labels defined in watches.yaml, as mentioned in #4949.

internal/helm/controller/controller.go

estroz · 2021-07-28T18:41:33Z

Needs a changelog fragment (addition) and docs in https://master.sdk.operatorframework.io/docs/building-operators/helm/reference/watches/.

internal/helm/controller/controller.go

VenkatRamaraju · 2021-07-29T15:27:44Z

Test output:

$ go test -v                                                                                         
=== RUN   TestFilterPredicate
--- PASS: TestFilterPredicate (0.27s)
=== RUN   TestHasAnnotation
--- PASS: TestHasAnnotation (0.00s)
PASS
ok  	github.com/operator-framework/operator-sdk/internal/helm/controller	0.656s

Additionally, go unit tests still seem to be failing on this PR even though they pass locally.

ok  	github.com/operator-framework/operator-sdk/internal/helm/client	5.622s	coverage: 11.8% of statements
ok  	github.com/operator-framework/operator-sdk/internal/helm/controller	5.684s	coverage: 9.7% of statements
ok  	github.com/operator-framework/operator-sdk/internal/helm/flags	4.701s	coverage: 93.8% of statements

internal/helm/controller/controller_test.go

estroz

/lgtm

internal/helm/controller/controller_test.go

estroz · 2021-07-29T16:54:59Z

internal/helm/controller/controller_test.go

+	cfg, _ := config.GetConfig()
+	mgr, _ := manager.New(cfg, manager.Options{})


I'm surprised this returns a valid manager. @VenkatRamaraju does this pass locally for you?

Ah it does not pass. I recommend refactoring the predicate parse logic into its own function and writing a test specifically for that function.

estroz · 2021-07-29T20:30:42Z

internal/helm/controller/controller.go

+	if !reflect.ValueOf(selector).IsZero() {
+		p, err := ctrlpredicate.LabelSelectorPredicate(selector)
+		if err != nil {
+			log.Error(err, "Unable to create predicate from selector.")


Suggested change

log.Error(err, "Unable to create predicate from selector.")

return fmt.Errorf("error constructing predicate from watches selector: %v", err), nil

do you mean return nil, fmt.Errorf("error constructing predicate from watches selector: %v", err) since the first return type is predicate and the second return type is error?

estroz · 2021-07-29T20:30:57Z

internal/helm/controller/controller.go

@@ -93,6 +104,21 @@ func Add(mgr manager.Manager, options WatchOptions) error {
 	return nil
 }

+// parsePredicateSelector parses the selector in the WatchOptions and creates a predicate
+// that is used to filter resources based on the specified selector
+func parsePredicateSelector(selector metav1.LabelSelector) ctrlpredicate.Predicate {


This function should return an error since the user will not want their operator to run if their selectors are not applied.

Suggested change

func parsePredicateSelector(selector metav1.LabelSelector) ctrlpredicate.Predicate {

func parsePredicateSelector(selector metav1.LabelSelector) (ctrlpredicate.Predicate, error) {

estroz · 2021-07-29T20:32:01Z

internal/helm/controller/controller.go

+		} else {
+			return p
+		}
+	}
+	return nil


Suggested change

} else {

return p

}

}

return nil

return p, nil

}

return nil, nil

estroz · 2021-07-29T21:22:45Z

internal/helm/controller/controller.go

+	if !reflect.ValueOf(selector).IsZero() {
+		p, err := ctrlpredicate.LabelSelectorPredicate(selector)
+		if err != nil {
+			return nil, fmt.Errorf("Error constructing predicate from watches selector: %v", err)


Error messages must start with a lowercase character if alphabetical.

Suggested change

return nil, fmt.Errorf("Error constructing predicate from watches selector: %v", err)

return nil, fmt.Errorf("error constructing predicate from watches selector: %v", err)

estroz

One fix then LGTM

fabianvf · 2021-07-30T20:19:46Z

changelog/fragments/helm-labelselector.yaml

@@ -0,0 +1,7 @@
+entries:
+  - description: >
+      Added a filter predicate for labels based on selectors specified in watches.yaml.


You should make it clear this is for the helm-operator specifically

fabianvf

/lgtm

Code all looks good, changelog fragment probably needs a slight tweak though

Signed-off-by: Venkat Ramaraju <venky2063@gmail.com>

jmrodri · 2021-08-02T17:49:46Z

/lgtm

Signed-off-by: Venkat Ramaraju <venky2063@gmail.com>

Signed-off-by: Venkat Ramaraju <venky2063@gmail.com> Signed-off-by: Thierry Wasylczenko <thierry.wasylczenko@gmail.com>

openshift-ci bot requested review from fabianvf and jmccormick2001 June 18, 2021 17:52

VenkatRamaraju force-pushed the Watch-Labels branch from d386566 to c05f541 Compare June 18, 2021 17:53

VenkatRamaraju mentioned this pull request Jun 21, 2021

Helm Operator for A/B deployment #4949

Closed

estroz reviewed Jun 21, 2021

View reviewed changes

internal/helm/controller/controller.go Outdated Show resolved Hide resolved

estroz suggested changes Jun 21, 2021

View reviewed changes

internal/helm/controller/controller.go Outdated Show resolved Hide resolved

estroz reviewed Jun 22, 2021

View reviewed changes

internal/helm/controller/controller.go Show resolved Hide resolved

internal/helm/controller/controller.go Outdated Show resolved Hide resolved

estroz reviewed Jun 22, 2021

View reviewed changes

internal/helm/controller/controller.go Outdated Show resolved Hide resolved

VenkatRamaraju force-pushed the Watch-Labels branch 2 times, most recently from 07e445c to 12e0fc3 Compare June 22, 2021 20:43

VenkatRamaraju mentioned this pull request Jul 23, 2021

Refactoring code for label-based filtering for Ansible Operators #5086

Merged

VenkatRamaraju requested a review from estroz July 28, 2021 18:36

VenkatRamaraju force-pushed the Watch-Labels branch 2 times, most recently from 1cbf0bc to 658fafa Compare July 28, 2021 20:13

estroz reviewed Jul 28, 2021

View reviewed changes

internal/helm/controller/controller.go Outdated Show resolved Hide resolved

VenkatRamaraju force-pushed the Watch-Labels branch from 72ec429 to b62d5ca Compare July 29, 2021 15:26

VenkatRamaraju requested a review from estroz July 29, 2021 15:27

VenkatRamaraju force-pushed the Watch-Labels branch from b62d5ca to 6942467 Compare July 29, 2021 15:54

VenkatRamaraju changed the title ~~Closes #4949: Label-based filtering for Helm via predicates~~ [WIP] Closes #4949: Label-based filtering for Helm via predicates Jul 29, 2021

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2021

estroz reviewed Jul 29, 2021

View reviewed changes

internal/helm/controller/controller_test.go Show resolved Hide resolved

estroz reviewed Jul 29, 2021

View reviewed changes

internal/helm/controller/controller_test.go Outdated Show resolved Hide resolved

estroz suggested changes Jul 29, 2021

View reviewed changes

openshift-ci bot assigned estroz Jul 29, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021

VenkatRamaraju force-pushed the Watch-Labels branch from 98e539d to b38ce97 Compare July 29, 2021 18:30

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021

VenkatRamaraju force-pushed the Watch-Labels branch from b38ce97 to 1128eae Compare July 29, 2021 18:31

VenkatRamaraju changed the title ~~[WIP] Closes #4949: Label-based filtering for Helm via predicates~~ Closes #4949: Label-based filtering for Helm via predicates Jul 29, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2021

VenkatRamaraju requested a review from estroz July 29, 2021 18:48

estroz suggested changes Jul 29, 2021

View reviewed changes

VenkatRamaraju force-pushed the Watch-Labels branch from 1128eae to 0f4b47a Compare July 29, 2021 21:17

estroz reviewed Jul 29, 2021

View reviewed changes

estroz approved these changes Jul 29, 2021

View reviewed changes

VenkatRamaraju force-pushed the Watch-Labels branch 2 times, most recently from 94e295b to 42a7d92 Compare July 29, 2021 22:00

fabianvf reviewed Jul 30, 2021

View reviewed changes

fabianvf approved these changes Jul 30, 2021

View reviewed changes

openshift-ci bot assigned fabianvf Jul 30, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2021

predicate for helm

c04315b

Signed-off-by: Venkat Ramaraju <venky2063@gmail.com>

VenkatRamaraju force-pushed the Watch-Labels branch from 42a7d92 to c04315b Compare July 30, 2021 20:36

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2021

openshift-ci bot assigned jmrodri Aug 2, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2021

jmrodri merged commit 65480f4 into operator-framework:master Aug 2, 2021

bentito pushed a commit to bentito/operator-sdk that referenced this pull request Aug 24, 2021

predicate for helm (operator-framework#4997)

91fc5a4

Signed-off-by: Venkat Ramaraju <venky2063@gmail.com>

twasyl pushed a commit to twasyl/operator-sdk that referenced this pull request Sep 3, 2021

predicate for helm (operator-framework#4997)

9187548

Signed-off-by: Venkat Ramaraju <venky2063@gmail.com> Signed-off-by: Thierry Wasylczenko <thierry.wasylczenko@gmail.com>

varshaprasad96 mentioned this pull request Dec 6, 2021

Apply updates from operator-sdk helm-operator operator-framework/helm-operator-plugins#63

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #4949: Label-based filtering for Helm via predicates #4997

Closes #4949: Label-based filtering for Helm via predicates #4997

VenkatRamaraju commented Jun 18, 2021

estroz commented Jul 28, 2021 •

edited

Loading

VenkatRamaraju commented Jul 29, 2021 •

edited

Loading

estroz left a comment

estroz Jul 29, 2021

estroz Jul 29, 2021

estroz Jul 29, 2021

VenkatRamaraju Jul 29, 2021

estroz Jul 29, 2021

estroz Jul 29, 2021

estroz Jul 29, 2021

estroz Jul 29, 2021

estroz left a comment

fabianvf Jul 30, 2021

fabianvf left a comment

jmrodri commented Aug 2, 2021

		cfg, _ := config.GetConfig()
		mgr, _ := manager.New(cfg, manager.Options{})

	log.Error(err, "Unable to create predicate from selector.")
	return fmt.Errorf("error constructing predicate from watches selector: %v", err), nil

	func parsePredicateSelector(selector metav1.LabelSelector) ctrlpredicate.Predicate {
	func parsePredicateSelector(selector metav1.LabelSelector) (ctrlpredicate.Predicate, error) {

	return nil, fmt.Errorf("Error constructing predicate from watches selector: %v", err)
	return nil, fmt.Errorf("error constructing predicate from watches selector: %v", err)

Closes #4949: Label-based filtering for Helm via predicates #4997

Closes #4949: Label-based filtering for Helm via predicates #4997

Conversation

VenkatRamaraju commented Jun 18, 2021

estroz commented Jul 28, 2021 • edited Loading

VenkatRamaraju commented Jul 29, 2021 • edited Loading

estroz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianvf left a comment

Choose a reason for hiding this comment

jmrodri commented Aug 2, 2021

estroz commented Jul 28, 2021 •

edited

Loading

VenkatRamaraju commented Jul 29, 2021 •

edited

Loading