fixup! fix: add integration tests for NAB

Signed-off-by: Mateus Oliveira <[email protected]>

Makefile

@@ -225,14 +225,15 @@ editorconfig: $(LOCALBIN) ## Download editorconfig locally if necessary.
 	}
 
 # TODO increase to 60?
-COVERAGE_THRESHOLD=40
+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

@@ -99,7 +99,8 @@ func main() {
 		TLSOpts: tlsOpts,
 	})
 
-	if len(function.GetOADPNamespace()) == 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)
 	}
@@ -133,10 +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???
-		// add env var here?? so it is only called once still and is easy to test
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "NonAdminBackup")
 		os.Exit(1)

internal/common/constant/constant.go

@@ -38,9 +38,6 @@ const (
 // 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

@@ -102,6 +102,7 @@ func GetBackupSpecFromNonAdminBackup(nonAdminBackup *nacv1alpha1.NonAdminBackup)
 	}
 
 	if nonAdminBackup.Spec.BackupSpec == nil {
+		// this should be Kubernetes API validation
 		return nil, fmt.Errorf("BackupSpec is not defined")
 	}
 
@@ -267,6 +268,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 {
@@ -283,9 +285,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"
@@ -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")
 }

internal/controller/nonadminbackup_controller.go

@@ -44,7 +44,8 @@ import (
 // NonAdminBackupReconciler reconciles a NonAdminBackup object
 type NonAdminBackupReconciler struct {
 	client.Client
-	Scheme *runtime.Scheme
+	Scheme        *runtime.Scheme
+	OADPNamespace string
 	// needed???
 	Context context.Context
 }
@@ -70,8 +71,7 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	err := r.Get(ctx, req.NamespacedName, &nab)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
-			// Delete event triggered this reconcile
-			logger.V(1).Info("Non existing NonAdminBackup")
+			logger.V(1).Info("NonAdminBackup was deleted")
 			return ctrl.Result{}, nil
 		}
 		logger.Error(err, "Unable to fetch NonAdminBackup")
@@ -148,7 +148,9 @@ func (r *NonAdminBackupReconciler) Init(ctx context.Context, logrLogger logr.Log
 	logger := logrLogger.WithValues("Init NonAdminBackup", types.NamespacedName{Name: nab.Name, Namespace: nab.Namespace})
 
 	if nab.Status.Phase == constant.EmptyString {
-		// // Set initial Phase to New
+		// Set initial Phase to New
+		// can this function be simplified to return just an error?
+		// can it return false, nil?
 		updatedStatus, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseNew)
 		if errUpdate != nil {
 			logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: New")
@@ -243,15 +245,13 @@ func (r *NonAdminBackupReconciler) UpdateSpecStatus(ctx context.Context, logrLog
 	logger := logrLogger.WithValues("UpdateSpecStatus NonAdminBackup", types.NamespacedName{Name: nab.Name, Namespace: nab.Namespace})
 
 	veleroBackupName := function.GenerateVeleroBackupName(nab.Namespace, nab.Name)
-
 	if veleroBackupName == constant.EmptyString {
 		return true, false, errors.New("unable to generate Velero Backup name")
 	}
 
-	oadpNamespace := function.GetOADPNamespace()
 	veleroBackup := velerov1api.Backup{}
-	veleroBackupLogger := logger.WithValues("VeleroBackup", types.NamespacedName{Name: veleroBackupName, Namespace: oadpNamespace})
-	err := r.Get(ctx, client.ObjectKey{Namespace: oadpNamespace, Name: veleroBackupName}, &veleroBackup)
+	veleroBackupLogger := logger.WithValues("VeleroBackup", types.NamespacedName{Name: veleroBackupName, Namespace: r.OADPNamespace})
+	err := r.Get(ctx, client.ObjectKey{Namespace: r.OADPNamespace, Name: veleroBackupName}, &veleroBackup)
 	if err != nil {
 		if !apierrors.IsNotFound(err) {
 			veleroBackupLogger.Error(err, "Unable to fetch VeleroBackup")
@@ -274,7 +274,7 @@ func (r *NonAdminBackupReconciler) UpdateSpecStatus(ctx context.Context, logrLog
 		veleroBackup = velerov1api.Backup{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      veleroBackupName,
-				Namespace: oadpNamespace,
+				Namespace: r.OADPNamespace,
 			},
 			Spec: *backupSpec,
 		}
@@ -322,7 +322,7 @@ func (r *NonAdminBackupReconciler) UpdateSpecStatus(ctx context.Context, logrLog
 	veleroBackupLogger.Info("VeleroBackup already exists, updating NonAdminBackup status")
 	updatedNab, errBackupUpdate := function.UpdateNonAdminBackupFromVeleroBackup(ctx, r.Client, logger, nab, &veleroBackup)
 	// Regardless if the status was updated or not, we should not
-	// requeue here as it was only status update.
+	// requeue here as it was only status update. AND SPEC???
 	if errBackupUpdate != nil {
 		return true, false, errBackupUpdate
 	} else if updatedNab {
@@ -342,7 +342,7 @@ func (r *NonAdminBackupReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		WithEventFilter(predicate.CompositePredicate{
 			NonAdminBackupPredicate: predicate.NonAdminBackupPredicate{},
 			VeleroBackupPredicate: predicate.VeleroBackupPredicate{
-				OadpVeleroNamespace: function.GetOADPNamespace(),
+				OadpVeleroNamespace: r.OADPNamespace,
 			},
 			Context: r.Context,
 		}).