test/refactor: improve naming clarity and test implicit assumptions

pkg/cluster/k8sres.go

@@ -784,7 +784,7 @@ func patchSidecarContainers(in []v1.Container, volumeMounts []v1.VolumeMount, su
 				},
 			},
 		}
-		container.Env = appendEnvVars(env, container.Env...)
+		container.Env = appendIfNotPresent(env, container.Env...)
 		result = append(result, container)
 	}
 
@@ -1023,7 +1023,7 @@ func (c *Cluster) generateSpiloPodEnvVars(
 
 	// fetch cluster-specific variables that will override all subsequent global variables
 	if len(spec.Env) > 0 {
-		envVars = appendEnvVars(envVars, spec.Env...)
+		envVars = appendIfNotPresent(envVars, spec.Env...)
 	}
 
 	if spec.Clone != nil && spec.Clone.ClusterName != "" {
@@ -1040,7 +1040,7 @@ func (c *Cluster) generateSpiloPodEnvVars(
 	if err != nil {
 		return nil, err
 	}
-	envVars = appendEnvVars(envVars, secretEnvVarsList...)
+	envVars = appendIfNotPresent(envVars, secretEnvVarsList...)
 
 	// fetch variables from custom environment ConfigMap
 	// that will override all subsequent global variables
@@ -1048,7 +1048,7 @@ func (c *Cluster) generateSpiloPodEnvVars(
 	if err != nil {
 		return nil, err
 	}
-	envVars = appendEnvVars(envVars, configMapEnvVarsList...)
+	envVars = appendIfNotPresent(envVars, configMapEnvVarsList...)
 
 	// global variables derived from operator configuration
 	opConfigEnvVars := make([]v1.EnvVar, 0)
@@ -1080,12 +1080,12 @@ func (c *Cluster) generateSpiloPodEnvVars(
 		opConfigEnvVars = append(opConfigEnvVars, v1.EnvVar{Name: "LOG_BUCKET_SCOPE_PREFIX", Value: ""})
 	}
 
-	envVars = appendEnvVars(envVars, opConfigEnvVars...)
+	envVars = appendIfNotPresent(envVars, opConfigEnvVars...)
 
 	return envVars, nil
 }
 
-func appendEnvVars(envs []v1.EnvVar, appEnv ...v1.EnvVar) []v1.EnvVar {
+func appendIfNotPresent(envs []v1.EnvVar, appEnv ...v1.EnvVar) []v1.EnvVar {
 	collectedEnvs := envs
 	for _, env := range appEnv {
 		if !isEnvVarPresent(collectedEnvs, env.Name) {
@@ -1378,7 +1378,7 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef
 		}
 		tlsEnv, tlsVolumes := generateTlsMounts(spec, getSpiloTLSEnv)
 		for _, env := range tlsEnv {
-			spiloEnvVars = appendEnvVars(spiloEnvVars, env)
+			spiloEnvVars = appendIfNotPresent(spiloEnvVars, env)
 		}
 		additionalVolumes = append(additionalVolumes, tlsVolumes...)
 	}
@@ -2490,7 +2490,7 @@ func (c *Cluster) generateLogicalBackupPodEnvVars() []v1.EnvVar {
 
 	switch backupProvider {
 	case "s3":
-		envVars = appendEnvVars(envVars, []v1.EnvVar{
+		envVars = appendIfNotPresent(envVars, []v1.EnvVar{
 			{
 				Name:  "LOGICAL_BACKUP_S3_REGION",
 				Value: c.OpConfig.LogicalBackup.LogicalBackupS3Region,
@@ -2522,7 +2522,7 @@ func (c *Cluster) generateLogicalBackupPodEnvVars() []v1.EnvVar {
 		}
 
 	case "az":
-		envVars = appendEnvVars(envVars, []v1.EnvVar{
+		envVars = appendIfNotPresent(envVars, []v1.EnvVar{
 			{
 				Name:  "LOGICAL_BACKUP_AZURE_STORAGE_ACCOUNT_NAME",
 				Value: c.OpConfig.LogicalBackup.LogicalBackupAzureStorageAccountName,

pkg/cluster/k8sres_test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"reflect"
 	"sort"
+	"strings"
 	"testing"
 	"time"
 
@@ -1168,6 +1169,7 @@ func TestCloneEnv(t *testing.T) {
 		cloneOpts *acidv1.CloneDescription
 		env       v1.EnvVar
 		envPos    int
+		envLen    int
 	}{
 		{
 			subTest: "custom s3 path",
@@ -1181,6 +1183,7 @@ func TestCloneEnv(t *testing.T) {
 				Value: "s3://some/path/",
 			},
 			envPos: 1,
+			envLen: 5,
 		},
 		{
 			subTest: "generated s3 path, bucket",
@@ -1194,6 +1197,7 @@ func TestCloneEnv(t *testing.T) {
 				Value: "wale-bucket",
 			},
 			envPos: 1,
+			envLen: 6,
 		},
 		{
 			subTest: "generated s3 path, target time",
@@ -1207,6 +1211,7 @@ func TestCloneEnv(t *testing.T) {
 				Value: "somewhen",
 			},
 			envPos: 4,
+			envLen: 6,
 		},
 	}
 
@@ -1225,6 +1230,19 @@ func TestCloneEnv(t *testing.T) {
 	for _, tt := range tests {
 		envs := cluster.generateCloneEnvironment(tt.cloneOpts)
 
+		if len(envs) != tt.envLen {
+			t.Errorf("%s %s: Expected number of env variables %d, have %d instead",
+				t.Name(), tt.subTest, tt.envLen, len(envs))
+		}
+
+		// verify all env var names start with CLONE_
+		for _, env := range envs {
+			if !strings.HasPrefix(env.Name, "CLONE_") {
+				t.Errorf("%s %s: env var name %q does not start with CLONE_",
+					t.Name(), tt.subTest, env.Name)
+			}
+		}
+
 		env := envs[tt.envPos]
 
 		if env.Name != tt.env.Name {
@@ -1239,7 +1257,7 @@ func TestCloneEnv(t *testing.T) {
 	}
 }
 
-func TestAppendEnvVar(t *testing.T) {
+func TestAppendIfNotPresent(t *testing.T) {
 	tests := []struct {
 		subTest      string
 		envs         []v1.EnvVar
@@ -1291,7 +1309,7 @@ func TestAppendEnvVar(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		finalEnvs := appendEnvVars(tt.envs, tt.envsToAppend...)
+		finalEnvs := appendIfNotPresent(tt.envs, tt.envsToAppend...)
 
 		if len(finalEnvs) != tt.expectedSize {
 			t.Errorf("%s %s: expected %d env variables, got %d",
@@ -1390,6 +1408,19 @@ func TestStandbyEnv(t *testing.T) {
 	for _, tt := range tests {
 		envs := cluster.generateStandbyEnvironment(tt.standbyOpts)
 
+		if len(envs) != tt.envLen {
+			t.Errorf("%s %s: Expected number of env variables %d, have %d instead",
+				t.Name(), tt.subTest, tt.envLen, len(envs))
+		}
+
+		// verify all env var names start with STANDBY_
+		for _, env := range envs {
+			if !strings.HasPrefix(env.Name, "STANDBY_") {
+				t.Errorf("%s %s: env var name %q does not start with STANDBY_",
+					t.Name(), tt.subTest, env.Name)
+			}
+		}
+
 		env := envs[tt.envPos]
 
 		if env.Name != tt.env.Name {
@@ -1401,11 +1432,6 @@ func TestStandbyEnv(t *testing.T) {
 			t.Errorf("%s %s: Expected env value %s, have %s instead",
 				t.Name(), tt.subTest, tt.env.Value, env.Value)
 		}
-
-		if len(envs) != tt.envLen {
-			t.Errorf("%s %s: Expected number of env variables %d, have %d instead",
-				t.Name(), tt.subTest, tt.envLen, len(envs))
-		}
 	}
 }