Skip to content

Commit 0bd3d12

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #425 from kstrenkova/small-refactor-changes
Refactor minor inconsistencies
2 parents 81981b2 + 5796943 commit 0bd3d12

5 files changed

Lines changed: 91 additions & 81 deletions

File tree

‎internal/controller/ansibletest_controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ func (r *AnsibleTestReconciler) Reconcile(ctx context.Context, req ctrl.Request)
174174
// another instance. This is considered to be an error state.
175175
lockAcquired, err := r.AcquireLock(ctx, instance, helper, false)
176176
if !lockAcquired {
177-
Log.Error(err, ErrConfirmLockOwnership, testOperatorLockName)
177+
Log.Error(err, fmt.Sprintf(ErrConfirmLockOwnership, testOperatorLockName))
178178
return ctrl.Result{RequeueAfter: RequeueAfterValue}, err
179179
}
180180

‎internal/controller/common.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ const (
4848

4949
const (
5050
// ErrConfirmLockOwnership is the error message for lock ownership confirmation failures
51-
ErrConfirmLockOwnership = "can not confirm ownership of %s lock"
51+
ErrConfirmLockOwnership = "Can not confirm ownership of %s lock."
5252
)
5353

5454
const (
@@ -727,7 +727,7 @@ func EnsureCloudsConfigMapExists(
727727
helper *helper.Helper,
728728
labels map[string]string,
729729
openstackConfigMapName string,
730-
) (ctrl.Result, error) {
730+
) error {
731731
const testOperatorCloudsConfigMapName = "test-operator-clouds-config"
732732

733733
cm, _, _ := configmap.GetConfigMap(
@@ -738,7 +738,7 @@ func EnsureCloudsConfigMapExists(
738738
time.Second*10,
739739
)
740740
if cm.Name == testOperatorCloudsConfigMapName {
741-
return ctrl.Result{}, nil
741+
return nil
742742
}
743743

744744
cm, _, _ = configmap.GetConfigMap(
@@ -753,7 +753,7 @@ func EnsureCloudsConfigMapExists(
753753

754754
err := yaml.Unmarshal([]byte(cm.Data["clouds.yaml"]), &result)
755755
if err != nil {
756-
return ctrl.Result{}, err
756+
return err
757757
}
758758

759759
clouds := result["clouds"].(map[string]interface{})
@@ -766,7 +766,7 @@ func EnsureCloudsConfigMapExists(
766766

767767
yamlString, err := yaml.Marshal(result)
768768
if err != nil {
769-
return ctrl.Result{}, err
769+
return err
770770
}
771771

772772
cms := []util.Template{
@@ -781,10 +781,10 @@ func EnsureCloudsConfigMapExists(
781781
}
782782
err = configmap.EnsureConfigMaps(ctx, helper, instance, cms, nil)
783783
if err != nil {
784-
return ctrl.Result{}, err
784+
return err
785785
}
786786

787-
return ctrl.Result{}, nil
787+
return nil
788788
}
789789

790790
// Int64OrPlaceholder converts int64 to string, returns placeholder if 0

‎internal/controller/horizontest_controller.go‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ func (r *HorizonTestReconciler) Reconcile(ctx context.Context, req ctrl.Request)
167167
// another instance. This is considered to be an error state.
168168
lockAcquired, err := r.AcquireLock(ctx, instance, helper, instance.Spec.Parallel)
169169
if !lockAcquired {
170-
Log.Error(err, ErrConfirmLockOwnership, testOperatorLockName)
170+
Log.Error(err, fmt.Sprintf(ErrConfirmLockOwnership, testOperatorLockName))
171171
return ctrl.Result{RequeueAfter: RequeueAfterValue}, err
172172
}
173173

@@ -212,23 +212,19 @@ func (r *HorizonTestReconciler) Reconcile(ctx context.Context, req ctrl.Request)
212212
mountKubeconfig := len(instance.Spec.KubeconfigSecretName) != 0
213213
instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage)
214214

215-
yamlResult, err := EnsureCloudsConfigMapExists(
216-
ctx,
217-
instance,
218-
helper,
219-
serviceLabels,
220-
instance.Spec.OpenStackConfigMap,
221-
)
215+
// Generate ConfigMaps
216+
err = r.generateServiceConfigMaps(ctx, helper, serviceLabels, instance)
222217
if err != nil {
223218
instance.Status.Conditions.Set(condition.FalseCondition(
224219
condition.ServiceConfigReadyCondition,
225220
condition.ErrorReason,
226221
condition.SeverityWarning,
227222
condition.ServiceConfigReadyErrorMessage,
228223
err.Error()))
229-
return yamlResult, err
224+
return ctrl.Result{}, err
230225
}
231226
instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage)
227+
// Generate ConfigMaps - end
232228

233229
// Create PersistentVolumeClaim
234230
ctrlResult, err := r.EnsureLogsPVCExists(
@@ -306,6 +302,22 @@ func (r *HorizonTestReconciler) SetupWithManager(mgr ctrl.Manager) error {
306302
Complete(r)
307303
}
308304

305+
func (r *HorizonTestReconciler) generateServiceConfigMaps(
306+
ctx context.Context,
307+
h *helper.Helper,
308+
labels map[string]string,
309+
instance *testv1beta1.HorizonTest,
310+
) error {
311+
err := EnsureCloudsConfigMapExists(
312+
ctx,
313+
instance,
314+
h,
315+
labels,
316+
instance.Spec.OpenStackConfigMap,
317+
)
318+
return err
319+
}
320+
309321
// PrepareHorizonTestEnvVars prepares environment variables for HorizonTest execution
310322
func (r *HorizonTestReconciler) PrepareHorizonTestEnvVars(
311323
instance *testv1beta1.HorizonTest,

‎internal/controller/tempest_controller.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
193193
// another instance. This is considered to be an error state.
194194
lockAcquired, err := r.AcquireLock(ctx, instance, helper, instance.Spec.Parallel)
195195
if !lockAcquired {
196-
Log.Error(err, ErrConfirmLockOwnership, testOperatorLockName)
196+
Log.Error(err, fmt.Sprintf(ErrConfirmLockOwnership, testOperatorLockName))
197197
return ctrl.Result{RequeueAfter: RequeueAfterValue}, err
198198
}
199199

@@ -287,7 +287,7 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
287287
// Create a new pod
288288
mountCerts := r.CheckSecretExists(ctx, instance, "combined-ca-bundle")
289289
customDataConfigMapName := GetCustomDataConfigMapName(instance, workflowStepIndex)
290-
EnvVarsConfigMapName := GetEnvVarsConfigMapName(instance, workflowStepIndex)
290+
envVarsConfigMapName := GetEnvVarsConfigMapName(instance, workflowStepIndex)
291291
podName := r.GetPodName(instance, workflowStepIndex)
292292
logsPVCName := r.GetPVCLogsName(instance, pvcIndex)
293293
containerImage, err := r.GetContainerImage(ctx, instance)
@@ -300,7 +300,7 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
300300
serviceLabels,
301301
serviceAnnotations,
302302
podName,
303-
EnvVarsConfigMapName,
303+
envVarsConfigMapName,
304304
customDataConfigMapName,
305305
logsPVCName,
306306
mountCerts,
@@ -313,7 +313,7 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
313313
// Creation of the tempest pod was not successful.
314314
// Release the lock and allow other controllers to spawn
315315
// a pod.
316-
if lockReleased, lockErr := r.ReleaseLock(ctx, instance); lockReleased {
316+
if lockReleased, lockErr := r.ReleaseLock(ctx, instance); !lockReleased {
317317
return ctrl.Result{RequeueAfter: RequeueAfterValue}, lockErr
318318
}
319319

‎internal/controller/tobiko_controller.go‎

Lines changed: 58 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ func (r *TobikoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
179179
// got claimed by another instance.
180180
lockAcquired, err := r.AcquireLock(ctx, instance, helper, instance.Spec.Parallel)
181181
if !lockAcquired {
182-
Log.Error(err, ErrConfirmLockOwnership, testOperatorLockName)
182+
Log.Error(err, fmt.Sprintf(ErrConfirmLockOwnership, testOperatorLockName))
183183
return ctrl.Result{RequeueAfter: RequeueAfterValue}, err
184184
}
185185

@@ -207,39 +207,35 @@ func (r *TobikoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
207207
return ctrl.Result{RequeueAfter: RequeueAfterValue}, err
208208
}
209209

210-
yamlResult, err := EnsureCloudsConfigMapExists(
211-
ctx,
212-
instance,
213-
helper,
214-
serviceLabels,
215-
instance.Spec.OpenStackConfigMap,
216-
)
210+
err = r.ValidateSecretWithKeys(ctx, instance, instance.Spec.KubeconfigSecretName, []string{})
217211
if err != nil {
218212
instance.Status.Conditions.Set(condition.FalseCondition(
219213
condition.InputReadyCondition,
220214
condition.ErrorReason,
221215
condition.SeverityWarning,
222216
condition.InputReadyErrorMessage,
223217
err.Error()))
224-
return yamlResult, err
218+
return ctrl.Result{}, err
225219
}
220+
mountKubeconfig := len(instance.Spec.KubeconfigSecretName) != 0
226221

227-
err = r.ValidateSecretWithKeys(ctx, instance, instance.Spec.KubeconfigSecretName, []string{})
222+
instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage)
223+
224+
// Generate ConfigMaps
225+
err = r.generateServiceConfigMaps(ctx, helper, serviceLabels, instance, workflowStepIndex)
228226
if err != nil {
229227
instance.Status.Conditions.Set(condition.FalseCondition(
230-
condition.InputReadyCondition,
228+
condition.ServiceConfigReadyCondition,
231229
condition.ErrorReason,
232230
condition.SeverityWarning,
233-
condition.InputReadyErrorMessage,
231+
condition.ServiceConfigReadyErrorMessage,
234232
err.Error()))
235233
return ctrl.Result{}, err
236234
}
237-
mountKubeconfig := len(instance.Spec.KubeconfigSecretName) != 0
238-
239-
instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage)
235+
instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage)
236+
// Generate ConfigMaps - end
240237

241238
pvcIndex := 0
242-
243239
// Create multiple PVCs for parallel execution
244240
if instance.Spec.Parallel && workflowStepIndex < len(instance.Spec.Workflow) {
245241
pvcIndex = workflowStepIndex
@@ -290,16 +286,13 @@ func (r *TobikoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
290286

291287
// Create Pod
292288
mountCerts := r.CheckSecretExists(ctx, instance, "combined-ca-bundle")
293-
294-
mountKeys := false
295-
if (len(instance.Spec.PublicKey) == 0) || (len(instance.Spec.PrivateKey) == 0) {
296-
Log.Info("Both values privateKey and publicKey need to be specified. Keys not mounted.")
297-
} else {
298-
mountKeys = true
289+
mountKeys := len(instance.Spec.PublicKey) > 0 && len(instance.Spec.PrivateKey) > 0
290+
if !mountKeys {
291+
Log.Info("Both values 'privateKey' and 'publicKey' need to be specified. Keys not mounted.")
299292
}
300293

301294
// Prepare Tobiko env vars
302-
envVars := r.PrepareTobikoEnvVars(ctx, serviceLabels, instance, helper, workflowStepIndex)
295+
envVars := r.PrepareTobikoEnvVars(instance, workflowStepIndex)
303296
podName := r.GetPodName(instance, workflowStepIndex)
304297
logsPVCName := r.GetPVCLogsName(instance, pvcIndex)
305298
containerImage, err := r.GetContainerImage(ctx, instance)
@@ -358,12 +351,51 @@ func (r *TobikoReconciler) SetupWithManager(mgr ctrl.Manager) error {
358351
Complete(r)
359352
}
360353

361-
// PrepareTobikoEnvVars prepares environment variables for a single workflow step
362-
func (r *TobikoReconciler) PrepareTobikoEnvVars(
354+
func (r *TobikoReconciler) generateServiceConfigMaps(
363355
ctx context.Context,
356+
h *helper.Helper,
364357
labels map[string]string,
365358
instance *testv1beta1.Tobiko,
366-
helper *helper.Helper,
359+
workflowStepIndex int,
360+
) error {
361+
err := EnsureCloudsConfigMapExists(
362+
ctx,
363+
instance,
364+
h,
365+
labels,
366+
instance.Spec.OpenStackConfigMap,
367+
)
368+
if err != nil {
369+
return err
370+
}
371+
372+
templateSpecs := []struct {
373+
infix string
374+
key string
375+
value string
376+
}{
377+
{tobiko.ConfigMapInfixConfig, tobiko.ConfigFileName, instance.Spec.Config},
378+
{tobiko.ConfigMapInfixPrivateKey, tobiko.PrivateKeyFileName, instance.Spec.PrivateKey},
379+
{tobiko.ConfigMapInfixPublicKey, tobiko.PublicKeyFileName, instance.Spec.PublicKey},
380+
}
381+
382+
cms := make([]util.Template, 0, len(templateSpecs))
383+
for _, spec := range templateSpecs {
384+
cms = append(cms, util.Template{
385+
Name: tobiko.GetConfigMapName(instance, spec.infix, workflowStepIndex),
386+
Namespace: instance.Namespace,
387+
InstanceType: instance.Kind,
388+
Labels: labels,
389+
CustomData: map[string]string{spec.key: spec.value},
390+
})
391+
}
392+
393+
return configmap.EnsureConfigMaps(ctx, h, instance, cms, nil)
394+
}
395+
396+
// PrepareTobikoEnvVars prepares environment variables for a single workflow step
397+
func (r *TobikoReconciler) PrepareTobikoEnvVars(
398+
instance *testv1beta1.Tobiko,
367399
workflowStepIndex int,
368400
) map[string]env.Setter {
369401
// Prepare env vars
@@ -404,39 +436,5 @@ func (r *TobikoReconciler) PrepareTobikoEnvVars(
404436
})
405437
}
406438

407-
// Prepare custom data
408-
templateSpecs := []struct {
409-
infix string
410-
key string
411-
value string
412-
}{
413-
{tobiko.ConfigMapInfixConfig, tobiko.ConfigFileName, instance.Spec.Config},
414-
{tobiko.ConfigMapInfixPrivateKey, tobiko.PrivateKeyFileName, instance.Spec.PrivateKey},
415-
{tobiko.ConfigMapInfixPublicKey, tobiko.PublicKeyFileName, instance.Spec.PublicKey},
416-
}
417-
418-
cms := make([]util.Template, 0, len(templateSpecs))
419-
for _, spec := range templateSpecs {
420-
cms = append(cms, util.Template{
421-
Name: tobiko.GetConfigMapName(instance, spec.infix, workflowStepIndex),
422-
Namespace: instance.Namespace,
423-
InstanceType: instance.Kind,
424-
Labels: labels,
425-
CustomData: map[string]string{spec.key: spec.value},
426-
})
427-
}
428-
429-
err := configmap.EnsureConfigMaps(ctx, helper, instance, cms, nil)
430-
if err != nil {
431-
instance.Status.Conditions.Set(condition.FalseCondition(
432-
condition.ServiceConfigReadyCondition,
433-
condition.ErrorReason,
434-
condition.SeverityWarning,
435-
condition.ServiceConfigReadyErrorMessage,
436-
err.Error()))
437-
return map[string]env.Setter{}
438-
}
439-
instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage)
440-
441439
return envVars
442440
}

0 commit comments

Comments
 (0)