Refactoring code for label-based filtering for Ansible Operators

internal/ansible/controller/controller.go

@@ -17,6 +17,7 @@ package controller
 import (
 	"fmt"
 	"os"
+	"reflect"
 	"strings"
 	"time"
 
@@ -33,7 +34,6 @@ import (
 
 	"github.com/operator-framework/operator-sdk/internal/ansible/events"
 	"github.com/operator-framework/operator-sdk/internal/ansible/handler"
-	"github.com/operator-framework/operator-sdk/internal/ansible/predicate"
 	"github.com/operator-framework/operator-sdk/internal/ansible/runner"
 )
 
@@ -103,12 +103,17 @@ func Add(mgr manager.Manager, options Options) *controller.Controller {
 	predicates := []ctrlpredicate.Predicate{
 		ctrlpredicate.Or(ctrlpredicate.GenerationChangedPredicate{}, libpredicate.NoGenerationPredicate{}),
 	}
-	filterPredicate, err := predicate.NewResourceFilterPredicate(options.Selector)
+
+	p, err := parsePredicateSelector(options.Selector)
+
 	if err != nil {
-		log.Error(err, "Error creating resource filter predicate")
+		log.Error(err, "")
 		os.Exit(1)
 	}
-	predicates = append(predicates, filterPredicate)
+
+	if p != nil {
+		predicates = append(predicates, p)
+	}
 
 	u := &unstructured.Unstructured{}
 	u.SetGroupVersionKind(options.GVK)
@@ -120,3 +125,17 @@ func Add(mgr manager.Manager, options Options) *controller.Controller {
 
 	return &c
 }
+
+// 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, error) {
+	// If a selector has been specified in watches.yaml, add it to the watch's predicates.
+	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)
+		}
+		return p, nil
+	}
+	return nil, nil
+}

internal/ansible/controller/controller_test.go

@@ -0,0 +1,39 @@
+// Copyright 2021 The Operator-SDK Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package controller
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestFilterPredicate(t *testing.T) {
+	matchLabelPass := make(map[string]string)
+	matchLabelPass["testKey"] = "testValue"
+	selectorPass := metav1.LabelSelector{
+		MatchLabels: matchLabelPass,
+	}
+	noSelector := metav1.LabelSelector{}
+
+	passPredicate, err := parsePredicateSelector(selectorPass)
+	assert.Equal(t, nil, err, "Verify that no error is thrown on a valid populated selector")
+	assert.NotEqual(t, nil, passPredicate, "Verify that a predicate is constructed using a valid selector")
+
+	nilPredicate, err := parsePredicateSelector(noSelector)
+	assert.Equal(t, nil, err, "Verify that no error is thrown on a valid unpopulated selector")
+	assert.Equal(t, nil, nilPredicate, "Verify correct parsing of an unpopulated selector")
+}

internal/ansible/watches/watches.go

@@ -85,36 +85,6 @@ var (
 	ansibleVerbosityDefault        = 2
 )
 
-// Creates, populates, and returns a LabelSelector object. Used in Unmarshal().
-func parseLabelSelector(dls tempLabelSelector) metav1.LabelSelector {
-	obj := metav1.LabelSelector{}
-	obj.MatchLabels = dls.MatchLabels
-
-	for _, v := range dls.MatchExpressions {
-		requirement := metav1.LabelSelectorRequirement{
-			Key:      v.Key,
-			Operator: v.Operator,
-			Values:   v.Values,
-		}
-
-		obj.MatchExpressions = append(obj.MatchExpressions, requirement)
-	}
-
-	return obj
-}
-
-// Temporary structs created to store yaml parsing
-type tempLabelSelector struct {
-	MatchLabels      map[string]string `yaml:"matchLabels,omitempty"`
-	MatchExpressions []tempRequirement `json:"matchExpressions,omitempty"`
-}
-
-type tempRequirement struct {
-	Key      string                       `json:"key"`
-	Operator metav1.LabelSelectorOperator `json:"operator"`
-	Values   []string                     `json:"values,omitempty"`
-}
-
 // Use an alias struct to handle complex types
 type alias struct {
 	Group                       string                    `yaml:"group"`
@@ -132,7 +102,7 @@ type alias struct {
 	MarkUnsafe                  *bool                     `yaml:"markUnsafe"`
 	Blacklist                   []schema.GroupVersionKind `yaml:"blacklist,omitempty"`
 	Finalizer                   *Finalizer                `yaml:"finalizer"`
-	Selector                    tempLabelSelector         `yaml:"selector"`
+	Selector                    metav1.LabelSelector      `yaml:"selector"`
 }
 
 // buildWatch will build Watch based on the values parsed from alias
@@ -201,7 +171,7 @@ func (w *Watch) setValuesFromAlias(tmp alias) error {
 		return err
 	}
 	w.addRolePlaybookPaths(wd)
-	w.Selector = parseLabelSelector(tmp.Selector)
+	w.Selector = tmp.Selector
 
 	return nil
 }