fix: add integration tests for NAB

Makefile

@@ -1,9 +1,9 @@
 
 # Image URL to use all building/pushing image targets
 IMG ?= quay.io/konveyor/oadp-non-admin:latest
-# Kubernetes version from OpenShift 4.15.x https://openshift-release.apps.ci.l2s4.p1.openshiftapps.com/#4-stable
+# Kubernetes version from OpenShift 4.16.x https://openshift-release.apps.ci.l2s4.p1.openshiftapps.com/#4-stable
 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
-ENVTEST_K8S_VERSION = 1.28
+ENVTEST_K8S_VERSION = 1.29
 
 # Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
 ifeq (,$(shell go env GOBIN))
@@ -224,15 +224,16 @@ editorconfig: $(LOCALBIN) ## Download editorconfig locally if necessary.
 	mv $(LOCALBIN)/$${ec_binary} $(EC) ;\
 	}
 
-# TODO increase!!!
-COVERAGE_THRESHOLD=10
+# TODO increase to 60?
+COVERAGE_THRESHOLD=50
 
 .PHONY: ci
 ci: simulation-test lint docker-build hadolint check-generate check-manifests ec check-images ## Run all project continuous integration (CI) checks locally.
 
 .PHONY: simulation-test
+# TODO coverage is not in sync in what is being actually done...
 simulation-test: envtest ## Run unit and integration tests.
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test $$(go list ./... | grep -v /e2e) -coverprofile cover.out
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test $(shell go list ./... | grep -v oadp-non-admin/test) -test.coverprofile cover.out -test.v -ginkgo.vv
 	@make check-coverage
 
 .PHONY: check-coverage

cmd/main.go

@@ -38,6 +38,7 @@ import (
 
 	nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1"
 	"github.com/migtools/oadp-non-admin/internal/common/constant"
+	"github.com/migtools/oadp-non-admin/internal/common/function"
 	"github.com/migtools/oadp-non-admin/internal/controller"
 )
 
@@ -98,7 +99,8 @@ func main() {
 		TLSOpts: tlsOpts,
 	})
 
-	if len(constant.OadpNamespace) == 0 {
+	oadpNamespace := function.GetOADPNamespace()
+	if len(oadpNamespace) == 0 {
 		setupLog.Error(fmt.Errorf("%v environment variable is empty", constant.NamespaceEnvVar), "environment variable must be set")
 		os.Exit(1)
 	}
@@ -132,8 +134,10 @@ func main() {
 	}
 
 	if err = (&controller.NonAdminBackupReconciler{
-		Client: mgr.GetClient(),
-		Scheme: mgr.GetScheme(),
+		Client:        mgr.GetClient(),
+		Scheme:        mgr.GetScheme(),
+		OADPNamespace: oadpNamespace,
+		// TODO context does not need to be set here???
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "NonAdminBackup")
 		os.Exit(1)

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/onsi/gomega v1.30.0
 	github.com/stretchr/testify v1.8.4
 	github.com/vmware-tanzu/velero v1.12.0
+	k8s.io/api v0.29.0
 	k8s.io/apimachinery v0.29.0
 	k8s.io/client-go v0.29.0
 	sigs.k8s.io/controller-runtime v0.17.0
@@ -65,7 +66,6 @@ require (
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
-	k8s.io/api v0.29.0 // indirect
 	k8s.io/apiextensions-apiserver v0.29.0 // indirect
 	k8s.io/component-base v0.29.0 // indirect
 	k8s.io/klog/v2 v2.110.1 // indirect
@@ -76,4 +76,5 @@ require (
 	sigs.k8s.io/yaml v1.4.0 // indirect
 )
 
+// need update?
 replace github.com/vmware-tanzu/velero => github.com/openshift/velero v0.10.2-0.20231024175012-d8101a298016

internal/common/constant/constant.go

@@ -17,8 +17,6 @@ limitations under the License.
 // Package constant contains all common constants used in the project
 package constant
 
-import "os"
-
 // Common labels for objects manipulated by the Non Admin Controller
 // Labels should be used to identify the NAC object
 // Annotations on the other hand should be used to define ownership
@@ -37,15 +35,9 @@ const (
 	NamespaceEnvVar = "WATCH_NAMESPACE"
 )
 
-// OadpNamespace is the namespace OADP operator is installed
-var OadpNamespace = os.Getenv(NamespaceEnvVar)
-
 // EmptyString defines a constant for the empty string
 const EmptyString = ""
 
-// NameSpaceString k8s Namespace string
-const NameSpaceString = "Namespace"
-
 // MaxKubernetesNameLength represents maximum length of the name in k8s
 const MaxKubernetesNameLength = 253
 

internal/common/function/function.go

@@ -23,6 +23,7 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
+	"os"
 	"reflect"
 
 	"github.com/go-logr/logr"
@@ -39,6 +40,11 @@ import (
 
 const requiredAnnotationError = "backup does not have the required annotation '%s'"
 
+// GetOADPNamespace get the namespace OADP operator is installed
+func GetOADPNamespace() string {
+	return os.Getenv(constant.NamespaceEnvVar)
+}
+
 // AddNonAdminLabels return a map with both the object labels and with the default Non Admin labels.
 // If error occurs, a map with only the default Non Admin labels is returned
 func AddNonAdminLabels(labels map[string]string) map[string]string {
@@ -89,11 +95,14 @@ func containsOnlyNamespace(namespaces []string, namespace string) bool {
 
 // GetBackupSpecFromNonAdminBackup return BackupSpec object from NonAdminBackup spec, if no error occurs
 func GetBackupSpecFromNonAdminBackup(nonAdminBackup *nacv1alpha1.NonAdminBackup) (*velerov1api.BackupSpec, error) {
+	// TODO https://github.com/migtools/oadp-non-admin/issues/60
+	// unnecessary?
 	if nonAdminBackup == nil {
 		return nil, fmt.Errorf("nonAdminBackup is nil")
 	}
 
 	if nonAdminBackup.Spec.BackupSpec == nil {
+		// this should be Kubernetes API validation
 		return nil, fmt.Errorf("BackupSpec is not defined")
 	}
 
@@ -105,7 +114,7 @@ func GetBackupSpecFromNonAdminBackup(nonAdminBackup *nacv1alpha1.NonAdminBackup)
 		veleroBackupSpec.IncludedNamespaces = []string{nonAdminBackup.Namespace}
 	} else {
 		if !containsOnlyNamespace(veleroBackupSpec.IncludedNamespaces, nonAdminBackup.Namespace) {
-			return nil, fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other then: %s", nonAdminBackup.Namespace)
+			return nil, fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other than: %s", nonAdminBackup.Namespace)
 		}
 	}
 
@@ -146,6 +155,7 @@ func GenerateVeleroBackupName(namespace, nabName string) string {
 
 // UpdateNonAdminPhase updates the phase of a NonAdminBackup object with the provided phase.
 func UpdateNonAdminPhase(ctx context.Context, r client.Client, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup, phase nacv1alpha1.NonAdminBackupPhase) (bool, error) {
+	// unnecessary?
 	if nab == nil {
 		return false, errors.New("NonAdminBackup object is nil")
 	}
@@ -169,7 +179,6 @@ func UpdateNonAdminPhase(ctx context.Context, r client.Client, logger logr.Logge
 	}
 
 	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Phase set to: %s", phase))
-
 	return true, nil
 }
 
@@ -178,50 +187,37 @@ func UpdateNonAdminPhase(ctx context.Context, r client.Client, logger logr.Logge
 // that the condition is set to the desired status only if it differs from the current status.
 // If the condition is already set to the desired status, no update is performed.
 func UpdateNonAdminBackupCondition(ctx context.Context, r client.Client, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup, condition nacv1alpha1.NonAdminCondition, conditionStatus metav1.ConditionStatus, reason string, message string) (bool, error) {
+	// log should be parent responsibility?
+	// unnecessary?
 	if nab == nil {
 		return false, errors.New("NonAdminBackup object is nil")
 	}
 
-	// Ensure phase and condition are valid
-	if condition == constant.EmptyString {
-		return false, errors.New("NonAdminBackup Condition cannot be empty")
-	}
-
-	if conditionStatus == constant.EmptyString {
-		return false, errors.New("NonAdminBackup Condition Status cannot be empty")
-	} else if conditionStatus != metav1.ConditionTrue && conditionStatus != metav1.ConditionFalse && conditionStatus != metav1.ConditionUnknown {
-		return false, errors.New("NonAdminBackup Condition Status must be valid metav1.ConditionStatus")
-	}
-
-	if reason == constant.EmptyString {
-		return false, errors.New("NonAdminBackup Condition Reason cannot be empty")
-	}
-
+	// is not this metav1 responsibility?
 	if message == constant.EmptyString {
 		return false, errors.New("NonAdminBackup Condition Message cannot be empty")
 	}
 
-	// Check if the condition is already set to the desired status
-	currentCondition := apimeta.FindStatusCondition(nab.Status.Conditions, string(condition))
-	if currentCondition != nil && currentCondition.Status == conditionStatus && currentCondition.Reason == reason && currentCondition.Message == message {
-		// Condition is already set to the desired status, no need to update
-		logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition is already set to: %s", condition))
-		return false, nil
-	}
-
+	// move this if outside func?
 	// Update NAB status condition
-	apimeta.SetStatusCondition(&nab.Status.Conditions,
+	update := apimeta.SetStatusCondition(&nab.Status.Conditions,
 		metav1.Condition{
 			Type:    string(condition),
 			Status:  conditionStatus,
 			Reason:  reason,
 			Message: message,
 		},
 	)
+	if !update {
+		// would remove log
+		logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition is already set to: %s", condition))
+		return false, nil
+	}
 
-	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition to: %s", condition))
-	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition Reason to: %s", reason))
-	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition Message to: %s", message))
+	// TODO these logs should be after err check, no?
+	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition set to: %s", condition))
+	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition Reason set to: %s", reason))
+	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition Message set to: %s", message))
 
 	// Update NAB status
 	if err := r.Status().Update(ctx, nab); err != nil {
@@ -257,6 +253,7 @@ func UpdateNonAdminBackupFromVeleroBackup(ctx context.Context, r client.Client,
 	}
 
 	// Check if BackupSpec needs to be updated
+	// avoid spec change?
 	if !reflect.DeepEqual(nab.Spec.BackupSpec, &veleroBackup.Spec) {
 		nab.Spec.BackupSpec = veleroBackup.Spec.DeepCopy()
 		if err := r.Update(ctx, nab); err != nil {
@@ -273,9 +270,8 @@ func UpdateNonAdminBackupFromVeleroBackup(ctx context.Context, r client.Client,
 }
 
 // CheckVeleroBackupLabels return true if Velero Backup object has required Non Admin labels, false otherwise
-func CheckVeleroBackupLabels(backup *velerov1api.Backup) bool {
+func CheckVeleroBackupLabels(labels map[string]string) bool {
 	// TODO also need to check for constant.OadpLabel label?
-	labels := backup.GetLabels()
 	value, exists := labels[constant.ManagedByLabel]
 	return exists && value == constant.ManagedByLabelValue
 }

internal/common/function/function_test.go

@@ -22,6 +22,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/onsi/ginkgo/v2"
 	"github.com/stretchr/testify/assert"
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -35,6 +36,8 @@ import (
 	"github.com/migtools/oadp-non-admin/internal/common/constant"
 )
 
+var _ = ginkgo.Describe("PLACEHOLDER", func() {})
+
 func TestMergeMaps(t *testing.T) {
 	const (
 		d     = "d"
@@ -219,7 +222,7 @@ func TestGetBackupSpecFromNonAdminBackup(t *testing.T) {
 
 	assert.Error(t, err)
 	assert.Nil(t, backupSpec)
-	assert.Equal(t, "spec.backupSpec.IncludedNamespaces can not contain namespaces other then: namespace2", err.Error())
+	assert.Equal(t, "spec.backupSpec.IncludedNamespaces can not contain namespaces other than: namespace2", err.Error())
 
 	backupSpecInput = &velerov1api.BackupSpec{
 		IncludedNamespaces: []string{"namespace3"},
@@ -237,7 +240,7 @@ func TestGetBackupSpecFromNonAdminBackup(t *testing.T) {
 
 	assert.Error(t, err)
 	assert.Nil(t, backupSpec)
-	assert.Equal(t, "spec.backupSpec.IncludedNamespaces can not contain namespaces other then: namespace4", err.Error())
+	assert.Equal(t, "spec.backupSpec.IncludedNamespaces can not contain namespaces other than: namespace4", err.Error())
 }
 
 func TestGenerateVeleroBackupName(t *testing.T) {
@@ -330,15 +333,15 @@ func TestCheckVeleroBackupLabels(t *testing.T) {
 			},
 		},
 	}
-	assert.True(t, CheckVeleroBackupLabels(backupWithLabel), "Expected backup to have required label")
+	assert.True(t, CheckVeleroBackupLabels(backupWithLabel.GetLabels()), "Expected backup to have required label")
 
 	// Backup does not have the required label
 	backupWithoutLabel := &velerov1api.Backup{
 		ObjectMeta: metav1.ObjectMeta{
 			Labels: map[string]string{},
 		},
 	}
-	assert.False(t, CheckVeleroBackupLabels(backupWithoutLabel), "Expected backup to not have required label")
+	assert.False(t, CheckVeleroBackupLabels(backupWithoutLabel.GetLabels()), "Expected backup to not have required label")
 
 	// Backup has the required label with incorrect value
 	backupWithIncorrectValue := &velerov1api.Backup{
@@ -348,5 +351,5 @@ func TestCheckVeleroBackupLabels(t *testing.T) {
 			},
 		},
 	}
-	assert.False(t, CheckVeleroBackupLabels(backupWithIncorrectValue), "Expected backup to not have required label")
+	assert.False(t, CheckVeleroBackupLabels(backupWithIncorrectValue.GetLabels()), "Expected backup to not have required label")
 }