Refactor minor inconsistencies

internal/controller/ansibletest_controller.go

@@ -174,7 +174,7 @@ func (r *AnsibleTestReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		// another instance. This is considered to be an error state.
 		lockAcquired, err := r.AcquireLock(ctx, instance, helper, false)
 		if !lockAcquired {
-			Log.Error(err, ErrConfirmLockOwnership, testOperatorLockName)
+			Log.Error(err, fmt.Sprintf(ErrConfirmLockOwnership, testOperatorLockName))
 			return ctrl.Result{RequeueAfter: RequeueAfterValue}, err
 		}
 

internal/controller/common.go

@@ -48,7 +48,7 @@ const (
 
 const (
 	// ErrConfirmLockOwnership is the error message for lock ownership confirmation failures
-	ErrConfirmLockOwnership = "can not confirm ownership of %s lock"
+	ErrConfirmLockOwnership = "Can not confirm ownership of %s lock."
 )
 
 const (
@@ -727,7 +727,7 @@ func EnsureCloudsConfigMapExists(
 	helper *helper.Helper,
 	labels map[string]string,
 	openstackConfigMapName string,
-) (ctrl.Result, error) {
+) error {
 	const testOperatorCloudsConfigMapName = "test-operator-clouds-config"
 
 	cm, _, _ := configmap.GetConfigMap(
@@ -738,7 +738,7 @@ func EnsureCloudsConfigMapExists(
 		time.Second*10,
 	)
 	if cm.Name == testOperatorCloudsConfigMapName {
-		return ctrl.Result{}, nil
+		return nil
 	}
 
 	cm, _, _ = configmap.GetConfigMap(
@@ -753,7 +753,7 @@ func EnsureCloudsConfigMapExists(
 
 	err := yaml.Unmarshal([]byte(cm.Data["clouds.yaml"]), &result)
 	if err != nil {
-		return ctrl.Result{}, err
+		return err
 	}
 
 	clouds := result["clouds"].(map[string]interface{})
@@ -766,7 +766,7 @@ func EnsureCloudsConfigMapExists(
 
 	yamlString, err := yaml.Marshal(result)
 	if err != nil {
-		return ctrl.Result{}, err
+		return err
 	}
 
 	cms := []util.Template{
@@ -781,10 +781,10 @@ func EnsureCloudsConfigMapExists(
 	}
 	err = configmap.EnsureConfigMaps(ctx, helper, instance, cms, nil)
 	if err != nil {
-		return ctrl.Result{}, err
+		return err
 	}
 
-	return ctrl.Result{}, nil
+	return nil
 }
 
 // Int64OrPlaceholder converts int64 to string, returns placeholder if 0

internal/controller/horizontest_controller.go

@@ -167,7 +167,7 @@ func (r *HorizonTestReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		// another instance. This is considered to be an error state.
 		lockAcquired, err := r.AcquireLock(ctx, instance, helper, instance.Spec.Parallel)
 		if !lockAcquired {
-			Log.Error(err, ErrConfirmLockOwnership, testOperatorLockName)
+			Log.Error(err, fmt.Sprintf(ErrConfirmLockOwnership, testOperatorLockName))
 			return ctrl.Result{RequeueAfter: RequeueAfterValue}, err
 		}
 
@@ -212,23 +212,19 @@ func (r *HorizonTestReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	mountKubeconfig := len(instance.Spec.KubeconfigSecretName) != 0
 	instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage)
 
-	yamlResult, err := EnsureCloudsConfigMapExists(
-		ctx,
-		instance,
-		helper,
-		serviceLabels,
-		instance.Spec.OpenStackConfigMap,
-	)
+	// Generate ConfigMaps
+	err = r.generateServiceConfigMaps(ctx, helper, serviceLabels, instance)
 	if err != nil {
 		instance.Status.Conditions.Set(condition.FalseCondition(
 			condition.ServiceConfigReadyCondition,
 			condition.ErrorReason,
 			condition.SeverityWarning,
 			condition.ServiceConfigReadyErrorMessage,
 			err.Error()))
-		return yamlResult, err
+		return ctrl.Result{}, err
 	}
 	instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage)
+	// Generate ConfigMaps - end
 
 	// Create PersistentVolumeClaim
 	ctrlResult, err := r.EnsureLogsPVCExists(
@@ -306,6 +302,22 @@ func (r *HorizonTestReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
+func (r *HorizonTestReconciler) generateServiceConfigMaps(
+	ctx context.Context,
+	h *helper.Helper,
+	labels map[string]string,
+	instance *testv1beta1.HorizonTest,
+) error {
+	err := EnsureCloudsConfigMapExists(
+		ctx,
+		instance,
+		h,
+		labels,
+		instance.Spec.OpenStackConfigMap,
+	)
+	return err
+}
+
 // PrepareHorizonTestEnvVars prepares environment variables for HorizonTest execution
 func (r *HorizonTestReconciler) PrepareHorizonTestEnvVars(
 	instance *testv1beta1.HorizonTest,

internal/controller/tempest_controller.go

@@ -193,7 +193,7 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
 		// another instance. This is considered to be an error state.
 		lockAcquired, err := r.AcquireLock(ctx, instance, helper, instance.Spec.Parallel)
 		if !lockAcquired {
-			Log.Error(err, ErrConfirmLockOwnership, testOperatorLockName)
+			Log.Error(err, fmt.Sprintf(ErrConfirmLockOwnership, testOperatorLockName))
 			return ctrl.Result{RequeueAfter: RequeueAfterValue}, err
 		}
 
@@ -287,7 +287,7 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
 	// Create a new pod
 	mountCerts := r.CheckSecretExists(ctx, instance, "combined-ca-bundle")
 	customDataConfigMapName := GetCustomDataConfigMapName(instance, workflowStepIndex)
-	EnvVarsConfigMapName := GetEnvVarsConfigMapName(instance, workflowStepIndex)
+	envVarsConfigMapName := GetEnvVarsConfigMapName(instance, workflowStepIndex)
 	podName := r.GetPodName(instance, workflowStepIndex)
 	logsPVCName := r.GetPVCLogsName(instance, pvcIndex)
 	containerImage, err := r.GetContainerImage(ctx, instance)
@@ -300,7 +300,7 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
 		serviceLabels,
 		serviceAnnotations,
 		podName,
-		EnvVarsConfigMapName,
+		envVarsConfigMapName,
 		customDataConfigMapName,
 		logsPVCName,
 		mountCerts,
@@ -313,7 +313,7 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
 		// Creation of the tempest pod was not successful.
 		// Release the lock and allow other controllers to spawn
 		// a pod.
-		if lockReleased, lockErr := r.ReleaseLock(ctx, instance); lockReleased {
+		if lockReleased, lockErr := r.ReleaseLock(ctx, instance); !lockReleased {
 			return ctrl.Result{RequeueAfter: RequeueAfterValue}, lockErr
 		}
 

internal/controller/tobiko_controller.go

@@ -179,7 +179,7 @@ func (r *TobikoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 		// got claimed by another instance.
 		lockAcquired, err := r.AcquireLock(ctx, instance, helper, instance.Spec.Parallel)
 		if !lockAcquired {
-			Log.Error(err, ErrConfirmLockOwnership, testOperatorLockName)
+			Log.Error(err, fmt.Sprintf(ErrConfirmLockOwnership, testOperatorLockName))
 			return ctrl.Result{RequeueAfter: RequeueAfterValue}, err
 		}
 
@@ -207,39 +207,35 @@ func (r *TobikoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 		return ctrl.Result{RequeueAfter: RequeueAfterValue}, err
 	}
 
-	yamlResult, err := EnsureCloudsConfigMapExists(
-		ctx,
-		instance,
-		helper,
-		serviceLabels,
-		instance.Spec.OpenStackConfigMap,
-	)
+	err = r.ValidateSecretWithKeys(ctx, instance, instance.Spec.KubeconfigSecretName, []string{})
 	if err != nil {
 		instance.Status.Conditions.Set(condition.FalseCondition(
 			condition.InputReadyCondition,
 			condition.ErrorReason,
 			condition.SeverityWarning,
 			condition.InputReadyErrorMessage,
 			err.Error()))
-		return yamlResult, err
+		return ctrl.Result{}, err
 	}
+	mountKubeconfig := len(instance.Spec.KubeconfigSecretName) != 0
 
-	err = r.ValidateSecretWithKeys(ctx, instance, instance.Spec.KubeconfigSecretName, []string{})
+	instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage)
+
+	// Generate ConfigMaps
+	err = r.generateServiceConfigMaps(ctx, helper, serviceLabels, instance, workflowStepIndex)
 	if err != nil {
 		instance.Status.Conditions.Set(condition.FalseCondition(
-			condition.InputReadyCondition,
+			condition.ServiceConfigReadyCondition,
 			condition.ErrorReason,
 			condition.SeverityWarning,
-			condition.InputReadyErrorMessage,
+			condition.ServiceConfigReadyErrorMessage,
 			err.Error()))
 		return ctrl.Result{}, err
 	}
-	mountKubeconfig := len(instance.Spec.KubeconfigSecretName) != 0
-
-	instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage)
+	instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage)
+	// Generate ConfigMaps - end
 
 	pvcIndex := 0
-
 	// Create multiple PVCs for parallel execution
 	if instance.Spec.Parallel && workflowStepIndex < len(instance.Spec.Workflow) {
 		pvcIndex = workflowStepIndex
@@ -290,16 +286,13 @@ func (r *TobikoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 
 	// Create Pod
 	mountCerts := r.CheckSecretExists(ctx, instance, "combined-ca-bundle")
-
-	mountKeys := false
-	if (len(instance.Spec.PublicKey) == 0) || (len(instance.Spec.PrivateKey) == 0) {
-		Log.Info("Both values privateKey and publicKey need to be specified. Keys not mounted.")
-	} else {
-		mountKeys = true
+	mountKeys := len(instance.Spec.PublicKey) > 0 && len(instance.Spec.PrivateKey) > 0
+	if !mountKeys {
+		Log.Info("Both values 'privateKey' and 'publicKey' need to be specified. Keys not mounted.")
 	}
 
 	// Prepare Tobiko env vars
-	envVars := r.PrepareTobikoEnvVars(ctx, serviceLabels, instance, helper, workflowStepIndex)
+	envVars := r.PrepareTobikoEnvVars(instance, workflowStepIndex)
 	podName := r.GetPodName(instance, workflowStepIndex)
 	logsPVCName := r.GetPVCLogsName(instance, pvcIndex)
 	containerImage, err := r.GetContainerImage(ctx, instance)
@@ -358,12 +351,51 @@ func (r *TobikoReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-// PrepareTobikoEnvVars prepares environment variables for a single workflow step
-func (r *TobikoReconciler) PrepareTobikoEnvVars(
+func (r *TobikoReconciler) generateServiceConfigMaps(
 	ctx context.Context,
+	h *helper.Helper,
 	labels map[string]string,
 	instance *testv1beta1.Tobiko,
-	helper *helper.Helper,
+	workflowStepIndex int,
+) error {
+	err := EnsureCloudsConfigMapExists(
+		ctx,
+		instance,
+		h,
+		labels,
+		instance.Spec.OpenStackConfigMap,
+	)
+	if err != nil {
+		return err
+	}
+
+	templateSpecs := []struct {
+		infix string
+		key   string
+		value string
+	}{
+		{tobiko.ConfigMapInfixConfig, tobiko.ConfigFileName, instance.Spec.Config},
+		{tobiko.ConfigMapInfixPrivateKey, tobiko.PrivateKeyFileName, instance.Spec.PrivateKey},
+		{tobiko.ConfigMapInfixPublicKey, tobiko.PublicKeyFileName, instance.Spec.PublicKey},
+	}
+
+	cms := make([]util.Template, 0, len(templateSpecs))
+	for _, spec := range templateSpecs {
+		cms = append(cms, util.Template{
+			Name:         tobiko.GetConfigMapName(instance, spec.infix, workflowStepIndex),
+			Namespace:    instance.Namespace,
+			InstanceType: instance.Kind,
+			Labels:       labels,
+			CustomData:   map[string]string{spec.key: spec.value},
+		})
+	}
+
+	return configmap.EnsureConfigMaps(ctx, h, instance, cms, nil)
+}
+
+// PrepareTobikoEnvVars prepares environment variables for a single workflow step
+func (r *TobikoReconciler) PrepareTobikoEnvVars(
+	instance *testv1beta1.Tobiko,
 	workflowStepIndex int,
 ) map[string]env.Setter {
 	// Prepare env vars
@@ -404,39 +436,5 @@ func (r *TobikoReconciler) PrepareTobikoEnvVars(
 		})
 	}
 
-	// Prepare custom data
-	templateSpecs := []struct {
-		infix string
-		key   string
-		value string
-	}{
-		{tobiko.ConfigMapInfixConfig, tobiko.ConfigFileName, instance.Spec.Config},
-		{tobiko.ConfigMapInfixPrivateKey, tobiko.PrivateKeyFileName, instance.Spec.PrivateKey},
-		{tobiko.ConfigMapInfixPublicKey, tobiko.PublicKeyFileName, instance.Spec.PublicKey},
-	}
-
-	cms := make([]util.Template, 0, len(templateSpecs))
-	for _, spec := range templateSpecs {
-		cms = append(cms, util.Template{
-			Name:         tobiko.GetConfigMapName(instance, spec.infix, workflowStepIndex),
-			Namespace:    instance.Namespace,
-			InstanceType: instance.Kind,
-			Labels:       labels,
-			CustomData:   map[string]string{spec.key: spec.value},
-		})
-	}
-
-	err := configmap.EnsureConfigMaps(ctx, helper, instance, cms, nil)
-	if err != nil {
-		instance.Status.Conditions.Set(condition.FalseCondition(
-			condition.ServiceConfigReadyCondition,
-			condition.ErrorReason,
-			condition.SeverityWarning,
-			condition.ServiceConfigReadyErrorMessage,
-			err.Error()))
-		return map[string]env.Setter{}
-	}
-	instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage)
-
 	return envVars
 }