Address review comments for operator conditions helpers (#42)

* Address review comments for opertor conditions helpers

* Move getOperatorNamespace() to a seprate utils package.
* Allow namespace to be set locally with an env variable.
* Modify the order of tests and add godoc to env variables.
* Remove redundant code.

* Update GetOperatorNamespace

Author: varshaprasad96

conditions/operator_conditions.go

@@ -15,13 +15,11 @@
 package conditions
 
 import (
-	"errors"
 	"fmt"
-	"io/ioutil"
 	"os"
-	"strings"
 
 	api "github.com/operator-framework/api/pkg/operators/v1"
+	"github.com/operator-framework/operator-lib/internal/utils"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -32,26 +30,29 @@ var (
 	ErrNoOperatorCondition = fmt.Errorf("operator Condition CRD is nil")
 )
 
-// TODO: verify from OLM if this will be the name of the environment variable
-// which is set for the Condition resource owned by the operator.
-const operatorCondEnvVar = "OPERATOR_CONDITION_NAME"
+const (
+	// operatorCondEnvVar is the env variable which
+	// contains the name of the Condition CR associated to the operator,
+	// set by OLM.
+	operatorCondEnvVar = "OPERATOR_CONDITION_NAME"
+)
 
-var readNamespace = func() ([]byte, error) {
-	return ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace")
-}
+var readNamespace = utils.GetOperatorNamespace
 
 // GetNamespacedName returns the NamespacedName of the CR. It returns an error
 // when the name of the CR cannot be found from the environment variable set by
-// OLM, or when the namespace cannot be found from the associated service account
-// secret.
+// OLM. Hence, GetNamespacedName() can provide the NamespacedName when the operator
+// is running on cluster and is being managed by OLM. If running locally, operator
+// writers are encouraged to skip this method or gracefully handle the errors by logging
+// a message.
 func GetNamespacedName() (*types.NamespacedName, error) {
 	conditionName := os.Getenv(operatorCondEnvVar)
 	if conditionName == "" {
-		return nil, fmt.Errorf("required env %s not set, cannot find operator condition CR for the operator", operatorCondEnvVar)
+		return nil, fmt.Errorf("could not determine operator condition name: environment variable %s not set", operatorCondEnvVar)
 	}
-	operatorNs, err := getOperatorNamespace()
+	operatorNs, err := readNamespace()
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("could not determine operator namespace: %v", err)
 	}
 	return &types.NamespacedName{Name: conditionName, Namespace: operatorNs}, nil
 }
@@ -118,16 +119,3 @@ func IsConditionStatusPresentAndEqual(operatorCondition *api.OperatorCondition,
 	}
 	return false, nil
 }
-
-// getOperatorNamespace returns the namespace the operator should be running in.
-func getOperatorNamespace() (string, error) {
-	nsBytes, err := readNamespace()
-	if err != nil {
-		if os.IsNotExist(err) {
-			return "", errors.New("cannot find namespace of the operator")
-		}
-		return "", err
-	}
-	ns := strings.TrimSpace(string(nsBytes))
-	return ns, nil
-}

conditions/operator_conditions_test.go

@@ -57,6 +57,47 @@ var _ = Describe("Conditions helpers", func() {
 			},
 		}
 	})
+
+	Describe("GetNamespacedName", func() {
+		It("should error when name of the operator condition cannot be found", func() {
+			err := os.Unsetenv(operatorCondEnvVar)
+			Expect(err).NotTo(HaveOccurred())
+
+			objKey, err := GetNamespacedName()
+			Expect(err).To(HaveOccurred())
+			Expect(objKey).To(BeNil())
+			Expect(err.Error()).To(ContainSubstring("could not determine operator condition name"))
+		})
+
+		It("should error when object namespace cannot be found", func() {
+			err := os.Setenv(operatorCondEnvVar, "test")
+			Expect(err).NotTo(HaveOccurred())
+
+			readNamespace = func() (string, error) {
+				return "", os.ErrNotExist
+			}
+
+			objKey, err := GetNamespacedName()
+			Expect(err).To(HaveOccurred())
+			Expect(objKey).To(BeNil())
+			Expect(err.Error()).To(ContainSubstring("could not determine operator namespace"))
+		})
+
+		It("should return the right namespaced name from SA namespace file", func() {
+			err := os.Setenv(operatorCondEnvVar, "test")
+			Expect(err).NotTo(HaveOccurred())
+
+			readNamespace = func() (string, error) {
+				return "testns", nil
+			}
+			objKey, err := GetNamespacedName()
+			Expect(err).NotTo(HaveOccurred())
+			Expect(objKey).NotTo(BeNil())
+			Expect(objKey.Name).To(BeEquivalentTo("test"))
+			Expect(objKey.Namespace).To(BeEquivalentTo("testns"))
+		})
+	})
+
 	Describe("SetOperatorCondition", func() {
 		It("should set condition status", func() {
 			newCond := metav1.Condition{
@@ -110,222 +151,145 @@ var _ = Describe("Conditions helpers", func() {
 	})
 
 	Describe("RemoveOperatorCondition", func() {
-		var (
-			rmvConditionType string
-		)
 		It("should remove the condition", func() {
-			rmvConditionType = conditionFoo
-			Expect(RemoveOperatorCondition(operatorCondition, rmvConditionType)).NotTo(HaveOccurred())
+			Expect(RemoveOperatorCondition(operatorCondition, conditionFoo)).NotTo(HaveOccurred())
 			Expect(len(operatorCondition.Status.Conditions)).To(BeEquivalentTo(0))
 		})
 		It("should not error when condition to be removed is not available", func() {
-			rmvConditionType = conditionBar
-			Expect(RemoveOperatorCondition(operatorCondition, rmvConditionType)).NotTo(HaveOccurred())
+			Expect(RemoveOperatorCondition(operatorCondition, conditionBar)).NotTo(HaveOccurred())
 			Expect(len(operatorCondition.Status.Conditions)).To(BeEquivalentTo(1))
 		})
 		It("should error when operatorCondition is nil", func() {
-			rmvConditionType = conditionFoo
-			err := RemoveOperatorCondition(nil, rmvConditionType)
+			err := RemoveOperatorCondition(nil, conditionFoo)
 			Expect(err).To(HaveOccurred())
 			Expect(err).Should(MatchError(ErrNoOperatorCondition))
 		})
 	})
 
 	Describe("FindOperatorCondition", func() {
-		var (
-			findConditionType string
-		)
-
 		It("should return the condition if it exists", func() {
-			findConditionType = conditionFoo
 			conditionToFind := &metav1.Condition{
 				Type:               conditionFoo,
 				Status:             metav1.ConditionTrue,
 				Reason:             "foo",
 				Message:            "The operator is in foo condition",
 				LastTransitionTime: transitionTime,
 			}
-			c, err := FindOperatorCondition(operatorCondition, findConditionType)
+			c, err := FindOperatorCondition(operatorCondition, conditionFoo)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(reflect.DeepEqual(c, conditionToFind)).To(BeTrue())
 		})
 		It("should return error when condition does not exist", func() {
-			findConditionType = conditionBar
-			c, err := FindOperatorCondition(operatorCondition, findConditionType)
+			c, err := FindOperatorCondition(operatorCondition, conditionBar)
 			Expect(err).To(HaveOccurred())
 			Expect(c).To(BeNil())
-			Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", findConditionType)))
+			Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionBar)))
 		})
 		It("should error when operatorCondition is nil", func() {
-			findConditionType = conditionFoo
-			c, err := FindOperatorCondition(nil, findConditionType)
+			c, err := FindOperatorCondition(nil, conditionFoo)
 			Expect(err).To(HaveOccurred())
 			Expect(c).To(BeNil())
 			Expect(err).Should(MatchError(ErrNoOperatorCondition))
 		})
 	})
 
 	Describe("Verfiy status of the condition", func() {
-		var (
-			conditionStatusFor string
-		)
-
 		It("should return correct value when condition exists", func() {
-			conditionStatusFor = conditionFoo
-
 			// IsConditionStatusTrue should return true
-			val, err := IsConditionStatusTrue(operatorCondition, conditionStatusFor)
+			val, err := IsConditionStatusTrue(operatorCondition, conditionFoo)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(val).To(BeTrue())
 
 			// IsConditionStatusFalse should return false
-			val, err = IsConditionStatusFalse(operatorCondition, conditionStatusFor)
+			val, err = IsConditionStatusFalse(operatorCondition, conditionFoo)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(val).To(BeFalse())
 
 			// IsConditionStatusUnknown should return false
-			val, err = IsConditionStatusUnknown(operatorCondition, conditionStatusFor)
+			val, err = IsConditionStatusUnknown(operatorCondition, conditionFoo)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(val).To(BeFalse())
 		})
 
 		It("should return false if condition status is not set to true", func() {
-			conditionStatusFor = conditionFoo
 			operatorCondition.Status.Conditions[0].Status = metav1.ConditionFalse
 
 			// IsConditionStatusTrue should return false
-			val, err := IsConditionStatusTrue(operatorCondition, conditionStatusFor)
+			val, err := IsConditionStatusTrue(operatorCondition, conditionFoo)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(val).To(BeFalse())
 
 			// IsConditionStatusFalse should return true
-			val, err = IsConditionStatusFalse(operatorCondition, conditionStatusFor)
+			val, err = IsConditionStatusFalse(operatorCondition, conditionFoo)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(val).To(BeTrue())
 
 			// IsConditionStatusUnknown should return false
-			val, err = IsConditionStatusUnknown(operatorCondition, conditionStatusFor)
+			val, err = IsConditionStatusUnknown(operatorCondition, conditionFoo)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(val).To(BeFalse())
 		})
 		It("should return false if condition status is unknown", func() {
-			conditionStatusFor = conditionFoo
 			operatorCondition.Status.Conditions[0].Status = metav1.ConditionUnknown
 
 			// IsConditionStatusTrue should return false
-			val, err := IsConditionStatusTrue(operatorCondition, conditionStatusFor)
+			val, err := IsConditionStatusTrue(operatorCondition, conditionFoo)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(val).To(BeFalse())
 
 			// IsConditionStatusFalse should return false
-			val, err = IsConditionStatusFalse(operatorCondition, conditionStatusFor)
+			val, err = IsConditionStatusFalse(operatorCondition, conditionFoo)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(val).To(BeFalse())
 
 			// IsConditionStatusUnknown should return true
-			val, err = IsConditionStatusUnknown(operatorCondition, conditionStatusFor)
+			val, err = IsConditionStatusUnknown(operatorCondition, conditionFoo)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(val).To(BeTrue())
 
 		})
 		It("should error when condition cannot be found", func() {
-			conditionStatusFor = conditionBar
-
 			// IsConditionStatusTrue should return error
-			val, err := IsConditionStatusTrue(operatorCondition, conditionStatusFor)
+			val, err := IsConditionStatusTrue(operatorCondition, conditionBar)
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionStatusFor)))
+			Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionBar)))
 			Expect(val).To(BeFalse())
 
 			// IsConditionStatusFalse should return error
-			val, err = IsConditionStatusFalse(operatorCondition, conditionStatusFor)
+			val, err = IsConditionStatusFalse(operatorCondition, conditionBar)
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionStatusFor)))
+			Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionBar)))
 			Expect(val).To(BeFalse())
 
 			// IsConditionStatusUnknown should return error
-			val, err = IsConditionStatusUnknown(operatorCondition, conditionStatusFor)
+			val, err = IsConditionStatusUnknown(operatorCondition, conditionBar)
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionStatusFor)))
+			Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionBar)))
 			Expect(val).To(BeFalse())
 		})
 	})
 
 	Describe("IsConditionStatusPresentAndEqual", func() {
-		var (
-			conditionType   string
-			conditionStatus metav1.ConditionStatus
-		)
 
 		It("should return true when condition is in the specified status", func() {
-			conditionType = conditionFoo
-			conditionStatus = metav1.ConditionTrue
-
-			val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionType, conditionStatus)
+			val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionFoo, metav1.ConditionTrue)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(val).To(BeTrue())
 		})
 
 		It("should return false when condition is not present in the specified status", func() {
-			conditionType = conditionFoo
-			conditionStatus = metav1.ConditionUnknown
-
-			val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionType, conditionStatus)
+			val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionFoo, metav1.ConditionUnknown)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(val).To(BeFalse())
 		})
 
 		It("should return error when condition is not present", func() {
-			conditionType = conditionBar
-			conditionStatus = metav1.ConditionTrue
-
-			val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionType, conditionStatus)
+			val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionBar, metav1.ConditionTrue)
 			Expect(err).To(HaveOccurred())
 			Expect(val).To(BeFalse())
 		})
 
 	})
-
-	Describe("GetNamespacedName", func() {
-		It("should error when name of the operator condition cannot be found", func() {
-			err := os.Unsetenv(operatorCondEnvVar)
-			Expect(err).NotTo(HaveOccurred())
-
-			objKey, err := GetNamespacedName()
-			Expect(err).To(HaveOccurred())
-			Expect(objKey).To(BeNil())
-			Expect(err.Error()).To(ContainSubstring("cannot find operator condition CR for the operator"))
-		})
-
-		It("should error when object namespace cannot be found", func() {
-			err := os.Setenv(operatorCondEnvVar, "test")
-			Expect(err).NotTo(HaveOccurred())
-
-			readNamespace = func() ([]byte, error) {
-				return nil, os.ErrNotExist
-			}
-
-			objKey, err := GetNamespacedName()
-			Expect(err).To(HaveOccurred())
-			Expect(objKey).To(BeNil())
-			Expect(err.Error()).To(ContainSubstring("cannot find namespace"))
-		})
-
-		It("should return the right namespaced name", func() {
-			err := os.Setenv(operatorCondEnvVar, "test")
-			Expect(err).NotTo(HaveOccurred())
-
-			readNamespace = func() ([]byte, error) {
-				return []byte("testns"), nil
-			}
-			objKey, err := GetNamespacedName()
-			Expect(err).NotTo(HaveOccurred())
-			Expect(objKey).NotTo(BeNil())
-			Expect(objKey.Name).To(BeEquivalentTo("test"))
-			Expect(objKey.Namespace).To(BeEquivalentTo("testns"))
-		})
-	})
-
 })
 
 func isConditionPresent(arr []metav1.Condition, con metav1.Condition) bool {