From 15af8d0e82d0d796a2e1a3c4d04b61efe946de74 Mon Sep 17 00:00:00 2001 From: Saravanan Balasubramanian Date: Tue, 14 Jul 2020 15:22:15 -0700 Subject: [PATCH 1/9] fix: WorkflowTemplateRef with defaults workflow spec on Configmap --- workflow/controller/controller_test.go | 2 +- workflow/controller/operator.go | 13 +++--- .../operator_workflow_template_ref_test.go | 3 +- workflow/util/util.go | 25 +++++++++++ workflow/util/util_test.go | 43 +++++++++++++++++++ workflow/validate/validate.go | 7 +++ 6 files changed, 85 insertions(+), 8 deletions(-) diff --git a/workflow/controller/controller_test.go b/workflow/controller/controller_test.go index 22c5dd460e6e..03cb025cc63b 100644 --- a/workflow/controller/controller_test.go +++ b/workflow/controller/controller_test.go @@ -395,7 +395,7 @@ func TestCheckAndInitWorkflowTmplRef(t *testing.T) { wf: wf} _, _, err := woc.loadExecutionSpec() assert.NoError(t, err) - assert.Equal(t, &wftmpl.Spec.WorkflowSpec, woc.wfSpec) + assert.Equal(t, wftmpl.Spec.WorkflowSpec.Templates, woc.wfSpec.Templates) } func TestIsArchivable(t *testing.T) { diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go index 2226b4363cd9..cbc9cf7d0ae1 100644 --- a/workflow/controller/operator.go +++ b/workflow/controller/operator.go @@ -2829,16 +2829,17 @@ func (woc *wfOperationCtx) loadExecutionSpec() (wfv1.TemplateReferenceHolder, wf } } - woc.wfSpec = woc.wf.Status.StoredWorkflowSpec - - entrypoint := woc.wf.Spec.Entrypoint - if entrypoint == "" { - entrypoint = woc.wfSpec.Entrypoint + // Set the workflow properties to execution spec. + mergedWf, err := wfutil.MergeWorkflows(wfv1.Workflow{Spec: *woc.wf.Status.StoredWorkflowSpec}, wfv1.Workflow{Spec: woc.wf.Spec}) + if err != nil { + return nil, executionParameters, err } + woc.wfSpec = &mergedWf.Spec + woc.volumes = woc.wfSpec.DeepCopy().Volumes - tmplRef := &wfv1.WorkflowStep{TemplateRef: woc.wf.Spec.WorkflowTemplateRef.ToTemplateRef(entrypoint)} + tmplRef := &wfv1.WorkflowStep{TemplateRef: woc.wf.Spec.WorkflowTemplateRef.ToTemplateRef(woc.wfSpec.Entrypoint)} if len(woc.wfSpec.Arguments.Parameters) > 0 { executionParameters.Parameters = util.MergeParameters(executionParameters.Parameters, woc.wfSpec.Arguments.Parameters) diff --git a/workflow/controller/operator_workflow_template_ref_test.go b/workflow/controller/operator_workflow_template_ref_test.go index c88a23a60925..5976a17a9e8c 100644 --- a/workflow/controller/operator_workflow_template_ref_test.go +++ b/workflow/controller/operator_workflow_template_ref_test.go @@ -15,7 +15,8 @@ func TestWorkflowTemplateRef(t *testing.T) { defer cancel() woc := newWorkflowOperationCtx(unmarshalWF(wfWithTmplRef), controller) woc.operate() - assert.Equal(t, &unmarshalWFTmpl(wfTmpl).Spec.WorkflowSpec, woc.wfSpec) + assert.Equal(t, unmarshalWFTmpl(wfTmpl).Spec.WorkflowSpec.Templates, woc.wfSpec.Templates) + assert.Equal(t, woc.wf.Spec.Entrypoint, woc.wfSpec.Entrypoint) // verify we copy these values assert.Len(t, woc.volumes, 1, "volumes from workflow template") // and these diff --git a/workflow/util/util.go b/workflow/util/util.go index 69e05236989c..badfc93e2d36 100644 --- a/workflow/util/util.go +++ b/workflow/util/util.go @@ -907,3 +907,28 @@ func GetNodeType(tmpl *wfv1.Template) wfv1.NodeType { } return "" } +// MergeWorkflows will do strategic merge the workflows +func MergeWorkflows(originalWf, patchWf wfv1.Workflow) (*wfv1.Workflow, error) { + workflowBytes, err := json.Marshal(wfv1.Workflow{Spec: patchWf.Spec}) + if err != nil { + return nil, err + } + + storedWFByte, err := json.Marshal(wfv1.Workflow{Spec: originalWf.Spec}) + if err != nil { + return nil, err + } + + mergedWFByte, err := strategicpatch.StrategicMergePatch(storedWFByte, workflowBytes, wfv1.Workflow{}) + if err != nil { + return nil, err + } + + var mergedWf wfv1.Workflow + err = json.Unmarshal(mergedWFByte, &mergedWf) + if err != nil { + return nil, err + } + return &mergedWf, nil + +} \ No newline at end of file diff --git a/workflow/util/util_test.go b/workflow/util/util_test.go index fa20f10ec88a..c8d7a3030083 100644 --- a/workflow/util/util_test.go +++ b/workflow/util/util_test.go @@ -396,3 +396,46 @@ func TestApplySubmitOpts(t *testing.T) { } }) } + +var origWF = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: workflow-template-hello-world- +spec: + arguments: + parameters: + - name: message + value: original + entrypoint: start + onExit: end + serviceAccountName: argo + workflowTemplateRef: + name: workflow-template-submittable +` +var patchWF = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: workflow-template-hello-world- +spec: + arguments: + parameters: + - name: message + value: patch + serviceAccountName: argo1 + podGC: + strategy: OnPodSuccess +` + +func TestMergeWorkflows(t *testing.T) { + origWf := unmarshalWF(origWF) + patchWf := unmarshalWF(patchWF) + + mergedWf, err := MergeWorkflows(*origWf, *patchWf) + assert.NoError(t, err) + assert.Equal(t, origWf.Spec.Entrypoint, mergedWf.Spec.Entrypoint) + assert.Equal(t, patchWf.Spec.ServiceAccountName, mergedWf.Spec.ServiceAccountName) + assert.Equal(t, patchWf.Spec.Arguments.Parameters[0].Name, mergedWf.Spec.Arguments.Parameters[0].Name) + assert.Equal(t, patchWf.Spec.Arguments.Parameters[0].Value, mergedWf.Spec.Arguments.Parameters[0].Value) +} diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go index a8c5f8412d6e..598f5a9f7742 100644 --- a/workflow/validate/validate.go +++ b/workflow/validate/validate.go @@ -98,6 +98,13 @@ var wfTmplRefAllowedWfSpecValidFields = map[string]bool{ "Arguments": true, "WorkflowTemplateRef": true, "TTLStrategy": true, + "Parallelism": true, + "Volumes": true, + "VolumeClaimTemplates": true, + "NodeSelector": true, + "OnExit": true, + "PodGC": true, + "ServiceAccountName": true, } type FakeArguments struct{} From 75ad501ed57e5d4eb1ccc877a46d6ba76a1cea42 Mon Sep 17 00:00:00 2001 From: Saravanan Balasubramanian Date: Tue, 14 Jul 2020 15:30:08 -0700 Subject: [PATCH 2/9] Update util.go --- workflow/util/util.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/workflow/util/util.go b/workflow/util/util.go index badfc93e2d36..95cc930a3072 100644 --- a/workflow/util/util.go +++ b/workflow/util/util.go @@ -907,6 +907,7 @@ func GetNodeType(tmpl *wfv1.Template) wfv1.NodeType { } return "" } + // MergeWorkflows will do strategic merge the workflows func MergeWorkflows(originalWf, patchWf wfv1.Workflow) (*wfv1.Workflow, error) { workflowBytes, err := json.Marshal(wfv1.Workflow{Spec: patchWf.Spec}) @@ -931,4 +932,4 @@ func MergeWorkflows(originalWf, patchWf wfv1.Workflow) (*wfv1.Workflow, error) { } return &mergedWf, nil -} \ No newline at end of file +} From ec2d918eb72efc8e64fc65a395ff99958837d22c Mon Sep 17 00:00:00 2001 From: Saravanan Balasubramanian Date: Tue, 14 Jul 2020 16:20:30 -0700 Subject: [PATCH 3/9] Update operator.go --- workflow/controller/operator.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go index cbc9cf7d0ae1..20de1ef0e838 100644 --- a/workflow/controller/operator.go +++ b/workflow/controller/operator.go @@ -2836,11 +2836,8 @@ func (woc *wfOperationCtx) loadExecutionSpec() (wfv1.TemplateReferenceHolder, wf } woc.wfSpec = &mergedWf.Spec - woc.volumes = woc.wfSpec.DeepCopy().Volumes - tmplRef := &wfv1.WorkflowStep{TemplateRef: woc.wf.Spec.WorkflowTemplateRef.ToTemplateRef(woc.wfSpec.Entrypoint)} - if len(woc.wfSpec.Arguments.Parameters) > 0 { executionParameters.Parameters = util.MergeParameters(executionParameters.Parameters, woc.wfSpec.Arguments.Parameters) } From 63cc572f43d44b61a23f5dfbe803aa80f3b265dc Mon Sep 17 00:00:00 2001 From: Saravanan Balasubramanian Date: Tue, 14 Jul 2020 17:38:55 -0700 Subject: [PATCH 4/9] Update argo_list.md --- docs/cli/argo_list.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/cli/argo_list.md b/docs/cli/argo_list.md index 251be1ad52b1..24ecb27f624e 100644 --- a/docs/cli/argo_list.md +++ b/docs/cli/argo_list.md @@ -22,6 +22,7 @@ argo list [flags] --older string List completed workflows finished before the specified duration (e.g. 10m, 3h, 1d) -o, --output string Output format. One of: wide|name --prefix string Filter workflows by prefix + --resubmitted Show only resubmitted workflows --running Show only running workflows -l, --selector string Selector (label query) to filter on, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2) --since string Show only workflows created after than a relative duration From 39b576ce35b8405a1a11447064fb2548e12f19f9 Mon Sep 17 00:00:00 2001 From: Saravanan Balasubramanian Date: Wed, 15 Jul 2020 09:34:26 -0700 Subject: [PATCH 5/9] refactor mergto function --- workflow/controller/controller.go | 16 +--------------- workflow/controller/operator.go | 7 ++++--- workflow/util/util.go | 30 ++++++++++++++++-------------- workflow/util/util_test.go | 12 ++++++------ 4 files changed, 27 insertions(+), 38 deletions(-) diff --git a/workflow/controller/controller.go b/workflow/controller/controller.go index 99a2aaa54aca..1a24d9084fc8 100644 --- a/workflow/controller/controller.go +++ b/workflow/controller/controller.go @@ -2,7 +2,6 @@ package controller import ( "context" - "encoding/json" "fmt" "os" "strconv" @@ -20,7 +19,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes" @@ -765,19 +763,7 @@ func (wfc *WorkflowController) newPodInformer() cache.SharedIndexInformer { // The defaults for the workflow controller are set in the workflow-controller config map func (wfc *WorkflowController) setWorkflowDefaults(wf *wfv1.Workflow) error { if wfc.Config.WorkflowDefaults != nil { - defaultsSpec, err := json.Marshal(*wfc.Config.WorkflowDefaults) - if err != nil { - return err - } - workflowBytes, err := json.Marshal(wf) - if err != nil { - return err - } - mergedWf, err := strategicpatch.StrategicMergePatch(defaultsSpec, workflowBytes, wfv1.Workflow{}) - if err != nil { - return err - } - err = json.Unmarshal(mergedWf, &wf) + err := util.MergeTo(wfc.Config.WorkflowDefaults, wf) if err != nil { return err } diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go index 9cddc42c43c9..23ab168a32a9 100644 --- a/workflow/controller/operator.go +++ b/workflow/controller/operator.go @@ -2829,13 +2829,14 @@ func (woc *wfOperationCtx) loadExecutionSpec() (wfv1.TemplateReferenceHolder, wf } } - // Set the workflow properties to execution spec. - mergedWf, err := wfutil.MergeWorkflows(wfv1.Workflow{Spec: *woc.wf.Status.StoredWorkflowSpec}, wfv1.Workflow{Spec: woc.wf.Spec}) + // Merge the workflow spec and storedWorkflowspec. + targetWf := wfv1.Workflow{Spec: *woc.wf.Status.StoredWorkflowSpec} + err := wfutil.MergeTo(woc.wf, &targetWf) if err != nil { return nil, executionParameters, err } + woc.wfSpec = &targetWf.Spec - woc.wfSpec = &mergedWf.Spec woc.volumes = woc.wfSpec.DeepCopy().Volumes tmplRef := &wfv1.WorkflowStep{TemplateRef: woc.wf.Spec.WorkflowTemplateRef.ToTemplateRef(woc.wfSpec.Entrypoint)} if len(woc.wfSpec.Arguments.Parameters) > 0 { diff --git a/workflow/util/util.go b/workflow/util/util.go index 95cc930a3072..e6560719e4a7 100644 --- a/workflow/util/util.go +++ b/workflow/util/util.go @@ -908,28 +908,30 @@ func GetNodeType(tmpl *wfv1.Template) wfv1.NodeType { return "" } -// MergeWorkflows will do strategic merge the workflows -func MergeWorkflows(originalWf, patchWf wfv1.Workflow) (*wfv1.Workflow, error) { - workflowBytes, err := json.Marshal(wfv1.Workflow{Spec: patchWf.Spec}) - if err != nil { - return nil, err +// MergeTo will do strategic merge the workflows +// patch workflow will be merged into target workflow. +func MergeTo(patch, target *wfv1.Workflow) error { + if target == nil || patch == nil { + return nil } - storedWFByte, err := json.Marshal(wfv1.Workflow{Spec: originalWf.Spec}) + patchWfBytes, err := json.Marshal(patch) if err != nil { - return nil, err + return err } - mergedWFByte, err := strategicpatch.StrategicMergePatch(storedWFByte, workflowBytes, wfv1.Workflow{}) + targetWfByte, err := json.Marshal(target) if err != nil { - return nil, err + return err } - var mergedWf wfv1.Workflow - err = json.Unmarshal(mergedWFByte, &mergedWf) + mergedWFByte, err := strategicpatch.StrategicMergePatch(targetWfByte, patchWfBytes, wfv1.Workflow{}) if err != nil { - return nil, err + return err } - return &mergedWf, nil - + err = json.Unmarshal(mergedWFByte, &target) + if err != nil { + return err + } + return nil } diff --git a/workflow/util/util_test.go b/workflow/util/util_test.go index c8d7a3030083..849a3896a4e7 100644 --- a/workflow/util/util_test.go +++ b/workflow/util/util_test.go @@ -429,13 +429,13 @@ spec: ` func TestMergeWorkflows(t *testing.T) { - origWf := unmarshalWF(origWF) + targetWf := unmarshalWF(origWF) patchWf := unmarshalWF(patchWF) - mergedWf, err := MergeWorkflows(*origWf, *patchWf) + err := MergeTo(patchWf, targetWf) assert.NoError(t, err) - assert.Equal(t, origWf.Spec.Entrypoint, mergedWf.Spec.Entrypoint) - assert.Equal(t, patchWf.Spec.ServiceAccountName, mergedWf.Spec.ServiceAccountName) - assert.Equal(t, patchWf.Spec.Arguments.Parameters[0].Name, mergedWf.Spec.Arguments.Parameters[0].Name) - assert.Equal(t, patchWf.Spec.Arguments.Parameters[0].Value, mergedWf.Spec.Arguments.Parameters[0].Value) + assert.Equal(t, "start", targetWf.Spec.Entrypoint) + assert.Equal(t, "argo1", targetWf.Spec.ServiceAccountName) + assert.Equal(t, "message", targetWf.Spec.Arguments.Parameters[0].Name) + assert.Equal(t, "patch", targetWf.Spec.Arguments.Parameters[0].Value) } From b30c55161feae705de98ea98535a0fdd667aec03 Mon Sep 17 00:00:00 2001 From: Saravanan Balasubramanian Date: Wed, 15 Jul 2020 12:14:48 -0700 Subject: [PATCH 6/9] addressed comments --- workflow/controller/controller.go | 2 +- workflow/controller/operator.go | 2 +- workflow/util/util.go | 13 +++++++++---- workflow/util/util_test.go | 4 ++-- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/workflow/controller/controller.go b/workflow/controller/controller.go index 1a24d9084fc8..0c49f5fba2fa 100644 --- a/workflow/controller/controller.go +++ b/workflow/controller/controller.go @@ -763,7 +763,7 @@ func (wfc *WorkflowController) newPodInformer() cache.SharedIndexInformer { // The defaults for the workflow controller are set in the workflow-controller config map func (wfc *WorkflowController) setWorkflowDefaults(wf *wfv1.Workflow) error { if wfc.Config.WorkflowDefaults != nil { - err := util.MergeTo(wfc.Config.WorkflowDefaults, wf) + err := util.MergeTo(wfc.Config.WorkflowDefaults, wf, false) if err != nil { return err } diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go index 23ab168a32a9..551d522b5902 100644 --- a/workflow/controller/operator.go +++ b/workflow/controller/operator.go @@ -2831,7 +2831,7 @@ func (woc *wfOperationCtx) loadExecutionSpec() (wfv1.TemplateReferenceHolder, wf // Merge the workflow spec and storedWorkflowspec. targetWf := wfv1.Workflow{Spec: *woc.wf.Status.StoredWorkflowSpec} - err := wfutil.MergeTo(woc.wf, &targetWf) + err := wfutil.MergeTo(woc.wf, &targetWf, true) if err != nil { return nil, executionParameters, err } diff --git a/workflow/util/util.go b/workflow/util/util.go index e6560719e4a7..ae561eb99132 100644 --- a/workflow/util/util.go +++ b/workflow/util/util.go @@ -910,7 +910,8 @@ func GetNodeType(tmpl *wfv1.Template) wfv1.NodeType { // MergeTo will do strategic merge the workflows // patch workflow will be merged into target workflow. -func MergeTo(patch, target *wfv1.Workflow) error { +// Overwrite flag indicates whether merge needs to overwrite the target value from patch +func MergeTo(patch, target *wfv1.Workflow, overwrite bool) error { if target == nil || patch == nil { return nil } @@ -924,12 +925,16 @@ func MergeTo(patch, target *wfv1.Workflow) error { if err != nil { return err } - - mergedWFByte, err := strategicpatch.StrategicMergePatch(targetWfByte, patchWfBytes, wfv1.Workflow{}) + var mergedWfByte []byte + if overwrite { + mergedWfByte, err = strategicpatch.StrategicMergePatch(targetWfByte, patchWfBytes, wfv1.Workflow{}) + } else { + mergedWfByte, err = strategicpatch.StrategicMergePatch(patchWfBytes, targetWfByte, wfv1.Workflow{}) + } if err != nil { return err } - err = json.Unmarshal(mergedWFByte, &target) + err = json.Unmarshal(mergedWfByte, target) if err != nil { return err } diff --git a/workflow/util/util_test.go b/workflow/util/util_test.go index 849a3896a4e7..7c29864e9675 100644 --- a/workflow/util/util_test.go +++ b/workflow/util/util_test.go @@ -432,10 +432,10 @@ func TestMergeWorkflows(t *testing.T) { targetWf := unmarshalWF(origWF) patchWf := unmarshalWF(patchWF) - err := MergeTo(patchWf, targetWf) + err := MergeTo(patchWf, targetWf, true) assert.NoError(t, err) assert.Equal(t, "start", targetWf.Spec.Entrypoint) assert.Equal(t, "argo1", targetWf.Spec.ServiceAccountName) assert.Equal(t, "message", targetWf.Spec.Arguments.Parameters[0].Name) - assert.Equal(t, "patch", targetWf.Spec.Arguments.Parameters[0].Value) + assert.Equal(t, "patch", targetWf.Spec.Arguments.Parameters[0].Value.StrVal) } From 57da2544c37414d53baf833714fd816d376ed429 Mon Sep 17 00:00:00 2001 From: Saravanan Balasubramanian Date: Wed, 15 Jul 2020 13:26:09 -0700 Subject: [PATCH 7/9] Added Test --- workflow/validate/validate.go | 1 - workflow/validate/validate_test.go | 9 +++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go index 598f5a9f7742..eeb4bf2be37e 100644 --- a/workflow/validate/validate.go +++ b/workflow/validate/validate.go @@ -102,7 +102,6 @@ var wfTmplRefAllowedWfSpecValidFields = map[string]bool{ "Volumes": true, "VolumeClaimTemplates": true, "NodeSelector": true, - "OnExit": true, "PodGC": true, "ServiceAccountName": true, } diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go index d74c043e8272..e70b71dd2cfc 100644 --- a/workflow/validate/validate_test.go +++ b/workflow/validate/validate_test.go @@ -2413,6 +2413,15 @@ metadata: generateName: hello-world- spec: entrypoint: A + serviceAccountName: argo + parallelism: 1 + volumes: + - name: workdir + emptyDir: {} + podGC: + strategy: OnPodSuccess + nodeSelector: + beta.kubernetes.io/arch: "{{inputs.parameters.arch}}" arguments: parameters: - name: lines-count From 544ad75fa714660519de6f770067510c256dfa36 Mon Sep 17 00:00:00 2001 From: Saravanan Balasubramanian Date: Wed, 15 Jul 2020 17:54:33 -0700 Subject: [PATCH 8/9] addressed comments --- workflow/controller/controller.go | 2 +- workflow/controller/operator.go | 6 ++-- workflow/util/merge.go | 40 +++++++++++++++++++++++++ workflow/util/merge_test.go | 50 +++++++++++++++++++++++++++++++ workflow/util/util.go | 33 -------------------- workflow/util/util_test.go | 43 -------------------------- 6 files changed, 95 insertions(+), 79 deletions(-) create mode 100644 workflow/util/merge.go create mode 100644 workflow/util/merge_test.go diff --git a/workflow/controller/controller.go b/workflow/controller/controller.go index 0c49f5fba2fa..1a24d9084fc8 100644 --- a/workflow/controller/controller.go +++ b/workflow/controller/controller.go @@ -763,7 +763,7 @@ func (wfc *WorkflowController) newPodInformer() cache.SharedIndexInformer { // The defaults for the workflow controller are set in the workflow-controller config map func (wfc *WorkflowController) setWorkflowDefaults(wf *wfv1.Workflow) error { if wfc.Config.WorkflowDefaults != nil { - err := util.MergeTo(wfc.Config.WorkflowDefaults, wf, false) + err := util.MergeTo(wfc.Config.WorkflowDefaults, wf) if err != nil { return err } diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go index 551d522b5902..092267f17755 100644 --- a/workflow/controller/operator.go +++ b/workflow/controller/operator.go @@ -2830,8 +2830,10 @@ func (woc *wfOperationCtx) loadExecutionSpec() (wfv1.TemplateReferenceHolder, wf } // Merge the workflow spec and storedWorkflowspec. - targetWf := wfv1.Workflow{Spec: *woc.wf.Status.StoredWorkflowSpec} - err := wfutil.MergeTo(woc.wf, &targetWf, true) + targetWf := wfv1.Workflow{Spec: woc.wf.Spec} + patchWf := wfv1.Workflow{Spec: *woc.wf.Status.StoredWorkflowSpec} + + err := wfutil.MergeTo(&patchWf, &targetWf) if err != nil { return nil, executionParameters, err } diff --git a/workflow/util/merge.go b/workflow/util/merge.go new file mode 100644 index 000000000000..4b30cdbfd4ef --- /dev/null +++ b/workflow/util/merge.go @@ -0,0 +1,40 @@ +package util + +import ( + "encoding/json" + + "k8s.io/apimachinery/pkg/util/strategicpatch" + + wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1" +) + +// MergeTo will do strategic merge the workflows +// patch workflow will be merged into target workflow. +// Target value will not be overwrite if it is already present +func MergeTo(patch, target *wfv1.Workflow) error { + if target == nil || patch == nil { + return nil + } + + patchWfBytes, err := json.Marshal(patch) + if err != nil { + return err + } + + targetWfByte, err := json.Marshal(target) + if err != nil { + return err + } + var mergedWfByte []byte + + mergedWfByte, err = strategicpatch.StrategicMergePatch(patchWfBytes, targetWfByte, wfv1.Workflow{}) + + if err != nil { + return err + } + err = json.Unmarshal(mergedWfByte, target) + if err != nil { + return err + } + return nil +} diff --git a/workflow/util/merge_test.go b/workflow/util/merge_test.go new file mode 100644 index 000000000000..f6f4b20a4b93 --- /dev/null +++ b/workflow/util/merge_test.go @@ -0,0 +1,50 @@ +package util + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +var origWF = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: workflow-template-hello-world- +spec: + arguments: + parameters: + - name: message + value: original + entrypoint: start + onExit: end + serviceAccountName: argo + workflowTemplateRef: + name: workflow-template-submittable +` +var patchWF = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: workflow-template-hello-world- +spec: + arguments: + parameters: + - name: message + value: patch + serviceAccountName: argo1 + podGC: + strategy: OnPodSuccess +` + +func TestMergeWorkflows(t *testing.T) { + patchWf := unmarshalWF(origWF) + targetWf := unmarshalWF(patchWF) + + err := MergeTo(patchWf, targetWf) + assert.NoError(t, err) + assert.Equal(t, "start", targetWf.Spec.Entrypoint) + assert.Equal(t, "argo1", targetWf.Spec.ServiceAccountName) + assert.Equal(t, "message", targetWf.Spec.Arguments.Parameters[0].Name) + assert.Equal(t, "patch", targetWf.Spec.Arguments.Parameters[0].Value.StrVal) +} diff --git a/workflow/util/util.go b/workflow/util/util.go index ae561eb99132..69e05236989c 100644 --- a/workflow/util/util.go +++ b/workflow/util/util.go @@ -907,36 +907,3 @@ func GetNodeType(tmpl *wfv1.Template) wfv1.NodeType { } return "" } - -// MergeTo will do strategic merge the workflows -// patch workflow will be merged into target workflow. -// Overwrite flag indicates whether merge needs to overwrite the target value from patch -func MergeTo(patch, target *wfv1.Workflow, overwrite bool) error { - if target == nil || patch == nil { - return nil - } - - patchWfBytes, err := json.Marshal(patch) - if err != nil { - return err - } - - targetWfByte, err := json.Marshal(target) - if err != nil { - return err - } - var mergedWfByte []byte - if overwrite { - mergedWfByte, err = strategicpatch.StrategicMergePatch(targetWfByte, patchWfBytes, wfv1.Workflow{}) - } else { - mergedWfByte, err = strategicpatch.StrategicMergePatch(patchWfBytes, targetWfByte, wfv1.Workflow{}) - } - if err != nil { - return err - } - err = json.Unmarshal(mergedWfByte, target) - if err != nil { - return err - } - return nil -} diff --git a/workflow/util/util_test.go b/workflow/util/util_test.go index 7c29864e9675..fa20f10ec88a 100644 --- a/workflow/util/util_test.go +++ b/workflow/util/util_test.go @@ -396,46 +396,3 @@ func TestApplySubmitOpts(t *testing.T) { } }) } - -var origWF = ` -apiVersion: argoproj.io/v1alpha1 -kind: Workflow -metadata: - generateName: workflow-template-hello-world- -spec: - arguments: - parameters: - - name: message - value: original - entrypoint: start - onExit: end - serviceAccountName: argo - workflowTemplateRef: - name: workflow-template-submittable -` -var patchWF = ` -apiVersion: argoproj.io/v1alpha1 -kind: Workflow -metadata: - generateName: workflow-template-hello-world- -spec: - arguments: - parameters: - - name: message - value: patch - serviceAccountName: argo1 - podGC: - strategy: OnPodSuccess -` - -func TestMergeWorkflows(t *testing.T) { - targetWf := unmarshalWF(origWF) - patchWf := unmarshalWF(patchWF) - - err := MergeTo(patchWf, targetWf, true) - assert.NoError(t, err) - assert.Equal(t, "start", targetWf.Spec.Entrypoint) - assert.Equal(t, "argo1", targetWf.Spec.ServiceAccountName) - assert.Equal(t, "message", targetWf.Spec.Arguments.Parameters[0].Name) - assert.Equal(t, "patch", targetWf.Spec.Arguments.Parameters[0].Value.StrVal) -} From bf6c5421bca0d270ff2e1233ac3fb1a5608fd0cd Mon Sep 17 00:00:00 2001 From: Saravanan Balasubramanian Date: Thu, 16 Jul 2020 10:49:08 -0700 Subject: [PATCH 9/9] addressed minor comments --- workflow/controller/operator.go | 4 +--- workflow/util/merge.go | 5 ++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go index 092267f17755..49f75507e596 100644 --- a/workflow/controller/operator.go +++ b/workflow/controller/operator.go @@ -2831,9 +2831,7 @@ func (woc *wfOperationCtx) loadExecutionSpec() (wfv1.TemplateReferenceHolder, wf // Merge the workflow spec and storedWorkflowspec. targetWf := wfv1.Workflow{Spec: woc.wf.Spec} - patchWf := wfv1.Workflow{Spec: *woc.wf.Status.StoredWorkflowSpec} - - err := wfutil.MergeTo(&patchWf, &targetWf) + err := wfutil.MergeTo(&wfv1.Workflow{Spec: *woc.wf.Status.StoredWorkflowSpec}, &targetWf) if err != nil { return nil, executionParameters, err } diff --git a/workflow/util/merge.go b/workflow/util/merge.go index 4b30cdbfd4ef..5371132dba16 100644 --- a/workflow/util/merge.go +++ b/workflow/util/merge.go @@ -8,9 +8,8 @@ import ( wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1" ) -// MergeTo will do strategic merge the workflows -// patch workflow will be merged into target workflow. -// Target value will not be overwrite if it is already present +// MergeTo will merge one workflow (the "patch" workflow) into another (the "target" workflow. +// If the target workflow defines a field, this take precedence over the patch. func MergeTo(patch, target *wfv1.Workflow) error { if target == nil || patch == nil { return nil