Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move merging of workflow section into webhook #168

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+113 −16
Draft

Move merging of workflow section into webhook #168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions api/v1beta1/common_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
package v1beta1

import (
kstrenkova marked this conversation as resolved.
Show resolved Hide resolved
"fmt"
"reflect"
)

const (
// ErrPrivilegedModeRequired
ErrPrivilegedModeRequired = "%s.Spec.Privileged is requied in order to successfully " +
Expand Down Expand Up @@ -28,3 +33,58 @@ const (
"ensures that the copying of the logs to the PV is completed without any " +
"complications."
)

// merge non-workflow section into workflow
func mergeSectionIntoWorkflow(instance interface{}, workflowStepNum int) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would modify the interface of this function like this:

Suggested change
func mergeSectionIntoWorkflow(instance interface{}, workflowStepNum int) {
func mergeSectionIntoWorkflow(instance interface{}, instanceWorkflowSection {}interface) {

It would be a responsibility of the webhook to pass the correct values. Wdyt?:)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into it and I don't understand why this would be needed 😄 Could you please explain your thought behind it a bit more?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so the idea for this function is to be generic because it is inside of the commonw_webhook.go . It should work for Tempest, Tobiko, HorizonTest and AnsibleTest CR. Yet you have a code which is specific to Tempest CR inside the function [1].

The idea is that inside the webhook you would call this function like this:

if len(spec.Workflow) > 0 {
	for _, workflowStepSpec := range spec.Workflow {
		mergeSectionIntoWorkflow(spec, workflowStepSpec)
	}
}

This would allow you to avoid the need to cast the instance variable to TempestSpec before you can grab the workflow section.

[1] here

// TODO add other resources
switch spec := instance.(type) {
case *TempestSpec:
fmt.Println("Instance is type *TempestSpec")

tRun := spec.TempestRun
wtRun := &spec.Workflow[workflowStepNum].TempestRun

tRunReflect := reflect.ValueOf(tRun)
wtRunReflect := reflect.ValueOf(wtRun).Elem()

setNonZeroValues(tRunReflect, wtRunReflect, false)
default:
fmt.Println("Error, instance of unknown type.")
}
}

func isEmpty(value interface{}) bool {
v := reflect.ValueOf(value)

switch v.Kind() {
case reflect.String:
return v.Len() == 0
case reflect.Ptr, reflect.Interface, reflect.Slice:
return v.IsNil()
default:
zero := reflect.Zero(v.Type()).Interface()
return reflect.DeepEqual(value, zero)
}
}

func setNonZeroValues(src reflect.Value, dest reflect.Value, isStruct bool) {
for i := 0; i < src.NumField(); i++ {
tRunName := src.Type().Field(i).Name
tRunValue := src.Field(i)
wtRunValue := dest.FieldByName(tRunName)

if isEmpty(wtRunValue) && !isEmpty(tRunValue.IsNil()) {
if tRunValue.Kind() == reflect.Struct {
setNonZeroValues(tRunValue, wtRunValue, true)
} else {
if isStruct {
wtRunValue.Set(tRunValue)
} else {
tRunPtr := reflect.New(tRunValue.Type())
tRunPtr.Elem().Set(tRunValue)
wtRunValue.Set(tRunPtr)
}
}
}
}
}
6 changes: 6 additions & 0 deletions api/v1beta1/tempest_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,12 @@ func (spec *TempestSpec) Default() {
if spec.TempestconfRun == (TempestconfRunSpec{}) {
spec.TempestconfRun.Create = true
}

if len(spec.Workflow) > 0 {
for key, _ := range spec.Workflow {
mergeSectionIntoWorkflow(spec, key)
}
}
}

func (r *Tempest) PrivilegedRequired() bool {
Expand Down
35 changes: 35 additions & 0 deletions controllers/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,3 +693,38 @@ func EnsureCloudsConfigMapExists(

return ctrl.Result{}, nil
}

// TODO make general for all resources
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

func getResourceRun(instance *v1beta1.Tempest, workflowStepNum int) v1beta1.TempestRunSpec {
if workflowStepNum < len(instance.Spec.Workflow) {
newInstance := changeType(instance, workflowStepNum).(*v1beta1.Tempest)
return newInstance.Spec.TempestRun
}
return instance.Spec.TempestRun
}

func changeType(instance interface{}, workflowStepNum int) interface{} {
// TODO other types; else if typedInstance, ok := instance.(*v1beta1.HorizonTest);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree:)


typedInstance, _ := instance.(*v1beta1.Tempest)
wtRun := typedInstance.Spec.Workflow[workflowStepNum].TempestRun

var tRun v1beta1.TempestRunSpec

wtReflected := reflect.ValueOf(wtRun)
tReflected := reflect.ValueOf(&tRun).Elem()

for i := 0; i < wtReflected.NumField(); i++ {
tName := tReflected.Type().Field(i).Name
tValue := tReflected.Field(i)

wtValue := wtReflected.FieldByName(tName)
if !wtValue.IsNil() {
wtValue = wtValue.Elem()
tValue.Set(wtValue)
}
}
typedInstance.Spec.TempestRun = tRun

return typedInstance
}
28 changes: 12 additions & 16 deletions controllers/tempest_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,37 +436,33 @@ func (r *TempestReconciler) setTempestConfigVars(envVars map[string]string,
instance *testv1beta1.Tempest,
workflowStepNum int,
) {
tRun := instance.Spec.TempestRun
wtRun := testv1beta1.WorkflowTempestRunSpec{}
if workflowStepNum < len(instance.Spec.Workflow) {
wtRun = instance.Spec.Workflow[workflowStepNum].TempestRun
}
tRun := getResourceRun(instance, workflowStepNum)

testOperatorDir := "/etc/test_operator/"

// Files
value := mergeWithWorkflow(tRun.WorkerFile, wtRun.WorkerFile)
value := tRun.WorkerFile
if len(value) != 0 {
workerFile := "worker_file.yaml"
customData[workerFile] = value
envVars["TEMPEST_WORKER_FILE"] = testOperatorDir + workerFile
}

value = mergeWithWorkflow(tRun.IncludeList, wtRun.IncludeList)
value = tRun.IncludeList
if len(value) != 0 {
includeListFile := "include.txt"
customData[includeListFile] = value
envVars["TEMPEST_INCLUDE_LIST"] = testOperatorDir + includeListFile
}

value = mergeWithWorkflow(tRun.ExcludeList, wtRun.ExcludeList)
value = tRun.ExcludeList
if len(value) != 0 {
excludeListFile := "exclude.txt"
customData[excludeListFile] = value
envVars["TEMPEST_EXCLUDE_LIST"] = testOperatorDir + excludeListFile
}

value = mergeWithWorkflow(tRun.ExpectedFailuresList, wtRun.ExpectedFailuresList)
value = tRun.ExpectedFailuresList
if len(value) != 0 {
expectedFailuresListFile := "expected_failures.txt"
customData[expectedFailuresListFile] = value
Expand All @@ -475,9 +471,9 @@ func (r *TempestReconciler) setTempestConfigVars(envVars map[string]string,

// Bool
tempestBoolEnvVars := map[string]bool{
"TEMPEST_SERIAL": mergeWithWorkflow(tRun.Serial, wtRun.Serial),
"TEMPEST_PARALLEL": mergeWithWorkflow(tRun.Parallel, wtRun.Parallel),
"TEMPEST_SMOKE": mergeWithWorkflow(tRun.Smoke, wtRun.Smoke),
"TEMPEST_SERIAL": tRun.Serial,
"TEMPEST_PARALLEL": tRun.Parallel,
"TEMPEST_SMOKE": tRun.Smoke,
"USE_EXTERNAL_FILES": true,
}

Expand All @@ -486,11 +482,11 @@ func (r *TempestReconciler) setTempestConfigVars(envVars map[string]string,
}

// Int
numValue := mergeWithWorkflow(tRun.Concurrency, wtRun.Concurrency)
numValue := tRun.Concurrency
envVars["TEMPEST_CONCURRENCY"] = r.GetDefaultInt(numValue)

// Dictionary
dictValue := mergeWithWorkflow(tRun.ExternalPlugin, wtRun.ExternalPlugin)
dictValue := tRun.ExternalPlugin
for _, externalPluginDictionary := range dictValue {
envVars["TEMPEST_EXTERNAL_PLUGIN_GIT_URL"] += externalPluginDictionary.Repository + ","

Expand All @@ -506,7 +502,7 @@ func (r *TempestReconciler) setTempestConfigVars(envVars map[string]string,

envVars["TEMPEST_WORKFLOW_STEP_DIR_NAME"] = r.GetJobName(instance, workflowStepNum)

extraImages := mergeWithWorkflow(tRun.ExtraImages, wtRun.ExtraImages)
extraImages := tRun.ExtraImages
for _, extraImageDict := range extraImages {
envVars["TEMPEST_EXTRA_IMAGES_URL"] += extraImageDict.URL + ","
envVars["TEMPEST_EXTRA_IMAGES_OS_CLOUD"] += extraImageDict.OsCloud + ","
Expand All @@ -524,7 +520,7 @@ func (r *TempestReconciler) setTempestConfigVars(envVars map[string]string,
envVars["TEMPEST_EXTRA_IMAGES_FLAVOR_VCPUS"] += r.GetDefaultInt(extraImageDict.Flavor.Vcpus, "-") + ","
}

extraRPMs := mergeWithWorkflow(tRun.ExtraRPMs, wtRun.ExtraRPMs)
extraRPMs := tRun.ExtraRPMs
for _, extraRPMURL := range extraRPMs {
envVars["TEMPEST_EXTRA_RPMS"] += extraRPMURL + ","
}
Expand Down
Loading