Removes test helper constant NoS3StoreAvailable

removes test helper constant NoS3StoreAvailable

Removes the NoS3StoreAvailable constant ("NoS3") that was marked with
FIXME as a test helper. Replace all usages with empty string checks
and refactor S3Profiles slice handling to filter empty strings upfront
before processing.

Changes:
- Remove NoS3StoreAvailable constant from ramenconfig.go
- Replace constant comparisons with empty string checks for single
  S3ProfileName fields
- Refactor S3Profiles slice handling to filter empty strings upfront
  instead of checking during iteration
- Remove skipIfS3ProfileIsForTest() function as it's no longer needed
- Update test files to use empty strings instead of the constant

This improves code quality by:
- Removing test-specific constants from production code
- Using more efficient upfront filtering for slices
- Maintaining consistent behavior while simplifying the codebase

Resolves #1710

Signed-off-by: raaizik <[email protected]>

Author: raaizik

internal/controller/drcluster_controller.go

@@ -319,7 +319,7 @@ func filterDRClusterSecret(ctx context.Context, reader client.Reader, secret *co
 
 		s3ProfileName := drcluster.Spec.S3ProfileName
 
-		if s3ProfileName == NoS3StoreAvailable {
+		if s3ProfileName == "" {
 			continue
 		}
 
@@ -499,7 +499,7 @@ func validateS3Profile(ctx context.Context, apiReader client.Reader,
 	objectStoreGetter ObjectStoreGetter,
 	drcluster *ramen.DRCluster, listKeyPrefix string, log logr.Logger,
 ) (string, error) {
-	if drcluster.Spec.S3ProfileName != NoS3StoreAvailable {
+	if drcluster.Spec.S3ProfileName != "" {
 		if reason, err := s3ProfileValidate(ctx, apiReader, objectStoreGetter,
 			drcluster.Spec.S3ProfileName, listKeyPrefix, log); err != nil {
 			return reason, err

internal/controller/drcluster_drcconfig_test.go

@@ -149,7 +149,7 @@ var _ = Describe("DRCluster-DRClusterConfigTests", Ordered, func() {
 		// Initialize --- DRCluster
 		drCluster1 = &ramen.DRCluster{
 			ObjectMeta: metav1.ObjectMeta{Name: drCluster1Name},
-			Spec:       ramen.DRClusterSpec{S3ProfileName: "NoS3", Region: "east"},
+			Spec:       ramen.DRClusterSpec{S3ProfileName: "", Region: "east"},
 		}
 
 		Expect(k8sClient.Create(context.TODO(), drCluster1)).To(Succeed())

internal/controller/ramenconfig.go

@@ -43,9 +43,6 @@ const (
 	DefaultVolSyncCopyMethod                          = "Snapshot"
 )
 
-// FIXME
-const NoS3StoreAvailable = "NoS3"
-
 var ControllerType ramendrv1alpha1.ControllerType
 
 var cachedRamenConfigFileName string

internal/controller/s3_store_accessors.go

@@ -21,14 +21,23 @@ func s3StoreAccessorsGet(
 	objectStorerGet func(string) (ObjectStorer, ramen.S3StoreProfile, error),
 	log logr.Logger,
 ) []s3StoreAccessor {
-	s3StoreAccessors := make([]s3StoreAccessor, 0, len(s3ProfileNames))
+	// Filter out empty profile names (no S3 store configured)
+	s3Profiles := make([]string, 0, len(s3ProfileNames))
+	for _, profile := range s3ProfileNames {
+		if profile != "" {
+			s3Profiles = append(s3Profiles, profile)
+		}
+	}
 
-	for _, s3ProfileName := range s3ProfileNames {
-		if s3ProfileName == NoS3StoreAvailable {
-			log.Info("Kube object protection store dummy")
+	if len(s3Profiles) == 0 {
+		log.Info("Kube object protection store dummy")
 
-			continue
-		}
+		return []s3StoreAccessor{}
+	}
+
+	s3StoreAccessors := make([]s3StoreAccessor, 0, len(s3Profiles))
+
+	for _, s3ProfileName := range s3Profiles {
 
 		objectStorer, s3StoreProfile, err := objectStorerGet(s3ProfileName)
 		if err != nil {

internal/controller/vrg_kubeobjects.go

@@ -557,18 +557,6 @@ func (v *VRGInstance) getVRGFromS3Profile(s3ProfileName string) (*ramen.VolumeRe
 	return vrg, nil
 }
 
-func (v *VRGInstance) skipIfS3ProfileIsForTest() bool {
-	for _, s3ProfileName := range v.instance.Spec.S3Profiles {
-		if s3ProfileName == NoS3StoreAvailable {
-			v.log.Info("NoS3 available to fetch")
-
-			return true
-		}
-	}
-
-	return false
-}
-
 func (v *VRGInstance) kubeObjectsRecoverFromS3(result *ctrl.Result, accessor s3StoreAccessor) error {
 	s3ProfileName := accessor.S3ProfileName
 
@@ -600,14 +588,8 @@ func (v *VRGInstance) kubeObjectsRecover(result *ctrl.Result) error {
 	}
 
 	if len(v.s3StoreAccessors) == 0 {
-		v.log.Info("No S3 profiles configured")
-
-		result.Requeue = true
-
-		return fmt.Errorf("no S3Profiles configured")
-	}
+		v.log.Info("No S3 profiles configured, skipping kube objects recovery")
 
-	if v.skipIfS3ProfileIsForTest() {
 		return nil
 	}
 

internal/controller/vrg_recipe_test.go

@@ -221,7 +221,7 @@ var _ = Describe("VolumeReplicationGroupRecipe", func() {
 		vrg = &ramen.VolumeReplicationGroup{
 			ObjectMeta: metav1.ObjectMeta{Namespace: namespaceName, Name: "a"},
 			Spec: ramen.VolumeReplicationGroupSpec{
-				S3Profiles:       []string{controllers.NoS3StoreAvailable},
+				S3Profiles:       []string{""},
 				ReplicationState: ramen.Primary,
 				Sync: &ramen.VRGSyncSpec{
 					PeerClasses: vrgSyncPeerClasses(),

internal/controller/vrg_volgrouprep.go

@@ -910,17 +910,23 @@ func (v *VRGInstance) restoreVGRsAndVGRCsForVolRep(result *ctrl.Result) error {
 }
 
 func (v *VRGInstance) restoreVGRsAndVGRCsFromS3(result *ctrl.Result) error {
-	err := errors.New("s3Profiles empty")
-	NoS3 := false
+	// Filter out empty profile names (no S3 store configured)
+	s3Profiles := make([]string, 0, len(v.instance.Spec.S3Profiles))
+	for _, profile := range v.instance.Spec.S3Profiles {
+		if profile != "" {
+			s3Profiles = append(s3Profiles, profile)
+		}
+	}
 
-	for _, s3ProfileName := range v.instance.Spec.S3Profiles {
-		if s3ProfileName == NoS3StoreAvailable {
-			v.log.Info("NoS3 available to fetch")
+	if len(s3Profiles) == 0 {
+		v.log.Info("NoS3 available to fetch")
 
-			NoS3 = true
+		return nil
+	}
 
-			continue
-		}
+	err := errors.New("s3Profiles empty")
+
+	for _, s3ProfileName := range s3Profiles {
 
 		var objectStore ObjectStorer
 
@@ -953,10 +959,6 @@ func (v *VRGInstance) restoreVGRsAndVGRCsFromS3(result *ctrl.Result) error {
 		return nil
 	}
 
-	if NoS3 {
-		return nil
-	}
-
 	result.Requeue = true
 
 	return err

internal/controller/vrg_volrep.go

@@ -1106,13 +1106,21 @@ func (v *VRGInstance) pvAndPvcObjectReplicasDelete(pvc corev1.PersistentVolumeCl
 }
 
 func (v *VRGInstance) s3StoresDo(do func(ObjectStorer) error, msg string) error {
-	for _, s3ProfileName := range v.instance.Spec.S3Profiles {
-		if s3ProfileName == NoS3StoreAvailable {
-			v.log.Info("NoS3 available to clean")
-
-			continue
+	// Filter out empty profile names (no S3 store configured)
+	s3Profiles := make([]string, 0, len(v.instance.Spec.S3Profiles))
+	for _, profile := range v.instance.Spec.S3Profiles {
+		if profile != "" {
+			s3Profiles = append(s3Profiles, profile)
 		}
+	}
+
+	if len(s3Profiles) == 0 {
+		v.log.Info("NoS3 available to clean")
 
+		return nil
+	}
+
+	for _, s3ProfileName := range s3Profiles {
 		if err := v.s3StoreDo(do, msg, s3ProfileName); err != nil {
 			return fmt.Errorf("%s using profile %s, err %w", msg, s3ProfileName, err)
 		}
@@ -2151,17 +2159,23 @@ func (v *VRGInstance) restorePVsAndPVCsForVolRep(result *ctrl.Result) (int, erro
 }
 
 func (v *VRGInstance) restorePVsAndPVCsFromS3(result *ctrl.Result) (int, error) {
-	err := errors.New("s3Profiles empty")
-	NoS3 := false
+	// Filter out empty profile names (no S3 store configured)
+	s3Profiles := make([]string, 0, len(v.instance.Spec.S3Profiles))
+	for _, profile := range v.instance.Spec.S3Profiles {
+		if profile != "" {
+			s3Profiles = append(s3Profiles, profile)
+		}
+	}
 
-	for _, s3ProfileName := range v.instance.Spec.S3Profiles {
-		if s3ProfileName == NoS3StoreAvailable {
-			v.log.Info("NoS3 available to fetch")
+	if len(s3Profiles) == 0 {
+		v.log.Info("NoS3 available to fetch")
 
-			NoS3 = true
+		return 0, nil
+	}
 
-			continue
-		}
+	err := errors.New("s3Profiles empty")
+
+	for _, s3ProfileName := range s3Profiles {
 
 		var objectStore ObjectStorer
 
@@ -2201,10 +2215,6 @@ func (v *VRGInstance) restorePVsAndPVCsFromS3(result *ctrl.Result) (int, error)
 		return pvCount + pvcCount, nil
 	}
 
-	if NoS3 {
-		return 0, nil
-	}
-
 	result.Requeue = true
 
 	return 0, err

internal/controller/vrg_volrep_test.go

@@ -616,7 +616,7 @@ var _ = Describe("VolumeReplicationGroupVolRepController", func() {
 			vrgS3UploadTestCase.verifyCachedUploadError()
 		})
 		Specify("set VRG's S3 profile names to empty", func() {
-			vrgS3UploadTestCase.vrgS3ProfilesSet([]string{vrgController.NoS3StoreAvailable})
+			vrgS3UploadTestCase.vrgS3ProfilesSet([]string{""})
 		})
 		It("cleans up after testing", func() {
 			vrgS3UploadTestCase.cleanupStatusUnprotected()