From f172385ba363061435aa178123e8ee92fb6f8761 Mon Sep 17 00:00:00 2001 From: FogDong Date: Mon, 14 Nov 2022 16:29:34 +0800 Subject: [PATCH 1/2] Fix: fix fill array with array in inputs Signed-off-by: FogDong --- pkg/cue/model/value/value.go | 11 ++++++++++- pkg/cue/model/value/value_test.go | 29 ++++++++++++++++++++++++----- pkg/tasks/custom/task.go | 3 ++- pkg/tasks/custom/task_test.go | 10 ++++++++-- pkg/types/types.go | 2 ++ 5 files changed, 46 insertions(+), 9 deletions(-) diff --git a/pkg/cue/model/value/value.go b/pkg/cue/model/value/value.go index e0f5e3c..8c33e6d 100644 --- a/pkg/cue/model/value/value.go +++ b/pkg/cue/model/value/value.go @@ -319,7 +319,16 @@ func (val *Value) fillRawByScript(x string, path string) error { if err != nil { return err } - v, err := val.MakeValue(raw + "\n" + a.v) + assemblerVal, err := val.MakeValue(a.v) + if err != nil { + return errors.WithMessage(err, "remake assembler value") + } + // open the list for assembler value + assemblerRaw, err := assemblerVal.String(sets.ListOpen) + if err != nil { + return err + } + v, err := val.MakeValue(raw + "\n" + assemblerRaw) if err != nil { return errors.WithMessage(err, "remake value") } diff --git a/pkg/cue/model/value/value_test.go b/pkg/cue/model/value/value_test.go index af9d151..fc50379 100644 --- a/pkg/cue/model/value/value_test.go +++ b/pkg/cue/model/value/value_test.go @@ -952,7 +952,26 @@ func TestFillByScript(t *testing.T) { x: 100 }, { name: "foo" - }] + }, ...] +} +`}, + { + name: "insert array to array", + raw: ` +a: b: c: [{x: 100}, {x: 101}, {x: 102}, ...]`, + path: "a.b.c[0].value", + v: `"foo"`, + expected: `a: { + b: { + c: [{ + x: 100 + value: "foo" + }, { + x: 101 + }, { + x: 102 + }, ...] + } } `, }, @@ -967,9 +986,9 @@ func TestFillByScript(t *testing.T) { y: [{ name: "key" value: "foo" - }] + }, ...] } - }] + }, ...] } `, }, @@ -1052,7 +1071,7 @@ func TestFillByScript(t *testing.T) { raw: `a: b: [{x: y:[{name: "key"}]}]`, path: "a.b[0].x.y[0].value", v: `foo`, - err: "remake value: a.b.x.y.value: reference \"foo\" not found", + err: "remake assembler value: a.b.x.y.value: reference \"foo\" not found", }, { name: "conflict merge", @@ -1066,7 +1085,7 @@ func TestFillByScript(t *testing.T) { raw: `a: b: [{x: y:[{name: "key"}]}]`, path: "a.b[0].x.y[0].value", v: `*+-`, - err: "remake value: expected operand, found '}'", + err: "remake assembler value: expected operand, found '}'", }, } diff --git a/pkg/tasks/custom/task.go b/pkg/tasks/custom/task.go index 7653dbd..f53317b 100644 --- a/pkg/tasks/custom/task.go +++ b/pkg/tasks/custom/task.go @@ -194,7 +194,8 @@ func (t *TaskLoader) makeTaskGenerator(templ string) (types.TaskGenerator, error for _, hook := range options.PreStartHooks { if err := hook(ctx, basicVal, wfStep); err != nil { tracer.Error(err, "do preStartHook") - return v1alpha1.StepStatus{}, nil, errors.WithMessage(err, "do preStartHook") + exec.err(ctx, false, err, types.StatusReasonInput) + return exec.status(), exec.operation(), nil } } diff --git a/pkg/tasks/custom/task_test.go b/pkg/tasks/custom/task_test.go index 92c369a..775bff0 100644 --- a/pkg/tasks/custom/task_test.go +++ b/pkg/tasks/custom/task_test.go @@ -263,9 +263,15 @@ close({ status, operation, err := run.Run(wfCtx, &types.TaskRunOptions{}) switch step.Name { case "input-err": - r.Equal(err.Error(), "do preStartHook: parameter.score.x: conflicting values 100 and 101") + r.Equal(status.Message, "parameter.score.x: conflicting values 100 and 101") + r.Equal(operation.Waiting, false) + r.Equal(status.Phase, v1alpha1.WorkflowStepPhaseFailed) + r.Equal(status.Reason, types.StatusReasonInput) case "input": - r.Equal(err.Error(), "do preStartHook: get input from [podIP]: failed to lookup value: var(path=podIP) not exist") + r.Equal(status.Message, "get input from [podIP]: failed to lookup value: var(path=podIP) not exist") + r.Equal(operation.Waiting, false) + r.Equal(status.Phase, v1alpha1.WorkflowStepPhaseFailed) + r.Equal(status.Reason, types.StatusReasonInput) case "output-var-conflict": r.Contains(status.Message, "conflict") r.Equal(operation.Waiting, false) diff --git a/pkg/types/types.go b/pkg/types/types.go index 6d759b4..ffea954 100644 --- a/pkg/types/types.go +++ b/pkg/types/types.go @@ -258,6 +258,8 @@ const ( StatusReasonTerminate = "Terminate" // StatusReasonParameter is the reason of the workflow progress condition which is ProcessParameter. StatusReasonParameter = "ProcessParameter" + // StatusReasonInput is the reason of the workflow progress condition which is Input. + StatusReasonInput = "Input" // StatusReasonOutput is the reason of the workflow progress condition which is Output. StatusReasonOutput = "Output" // StatusReasonFailedAfterRetries is the reason of the workflow progress condition which is FailedAfterRetries. From e9a17404a2ed3e96cb3d3cb85f47d534e289943a Mon Sep 17 00:00:00 2001 From: FogDong Date: Mon, 14 Nov 2022 16:55:29 +0800 Subject: [PATCH 2/2] deprecate fill value by script Signed-off-by: FogDong --- charts/vela-workflow/README.md | 9 +-- pkg/cue/model/value/value.go | 110 +----------------------------- pkg/cue/model/value/value_test.go | 37 +++++----- pkg/tasks/custom/task_test.go | 2 +- 4 files changed, 25 insertions(+), 133 deletions(-) diff --git a/charts/vela-workflow/README.md b/charts/vela-workflow/README.md index 4382c56..031e7f9 100644 --- a/charts/vela-workflow/README.md +++ b/charts/vela-workflow/README.md @@ -29,10 +29,11 @@ helm install --create-namespace -n vela-system workflow kubevela/vela-workflow - ### Core parameters -| Name | Description | Value | -| --------------------------- | --------------------------------------------------------------------------------------------- | ----- | -| `systemDefinitionNamespace` | System definition namespace, if unspecified, will use built-in variable `.Release.Namespace`. | `nil` | -| `concurrentReconciles` | concurrentReconciles is the concurrent reconcile number of the controller | `4` | +| Name | Description | Value | +| -------------------------------------------- | --------------------------------------------------------------------------------------------------------------------- | ------- | +| `systemDefinitionNamespace` | System definition namespace, if unspecified, will use built-in variable `.Release.Namespace`. | `nil` | +| `concurrentReconciles` | concurrentReconciles is the concurrent reconcile number of the controller | `4` | +| `ignoreWorkflowWithoutControllerRequirement` | will determine whether to process the workflowrun without 'workflowrun.oam.dev/controller-version-require' annotation | `false` | ### KubeVela workflow parameters diff --git a/pkg/cue/model/value/value.go b/pkg/cue/model/value/value.go index 8c33e6d..4e9ddca 100644 --- a/pkg/cue/model/value/value.go +++ b/pkg/cue/model/value/value.go @@ -29,7 +29,6 @@ import ( "cuelang.org/go/cue/cuecontext" "cuelang.org/go/cue/literal" "cuelang.org/go/cue/parser" - "cuelang.org/go/cue/token" "github.com/cue-exp/kubevelafix" "github.com/pkg/errors" @@ -291,51 +290,11 @@ func (val *Value) FillRaw(x string, paths ...string) error { // FillValueByScript unify the value x at the given script path. func (val *Value) FillValueByScript(x *Value, path string) error { - if !strings.Contains(path, "[") { - newV := val.v.FillPath(FieldPath(path), x.v) - if err := newV.Err(); err != nil { - return err - } - val.v = newV - return nil - } - s, err := x.String() - if err != nil { + newV := val.v.FillPath(FieldPath(path), x.v) + if err := newV.Err(); err != nil { return err } - return val.fillRawByScript(s, path) -} - -func (val *Value) fillRawByScript(x string, path string) error { - a := newAssembler(x) - pathExpr, err := parser.ParseExpr("path", path) - if err != nil { - return errors.WithMessage(err, "parse path") - } - if err := a.installTo(pathExpr); err != nil { - return err - } - raw, err := val.String(sets.ListOpen) - if err != nil { - return err - } - assemblerVal, err := val.MakeValue(a.v) - if err != nil { - return errors.WithMessage(err, "remake assembler value") - } - // open the list for assembler value - assemblerRaw, err := assemblerVal.String(sets.ListOpen) - if err != nil { - return err - } - v, err := val.MakeValue(raw + "\n" + assemblerRaw) - if err != nil { - return errors.WithMessage(err, "remake value") - } - if err := v.Error(); err != nil { - return err - } - *val = *v + val.v = newV return nil } @@ -688,69 +647,6 @@ func isDef(s string) bool { return strings.HasPrefix(s, "#") } -// assembler put value under parsed expression as path. -type assembler struct { - v string -} - -func newAssembler(v string) *assembler { - return &assembler{v: v} -} - -func (a *assembler) fill2Path(p string) { - a.v = fmt.Sprintf("%s: %s", p, a.v) -} - -func (a *assembler) fill2Array(i int) { - s := "" - for j := 0; j < i; j++ { - s += "_," - } - if strings.Contains(a.v, ":") && !strings.HasPrefix(a.v, "{") { - a.v = fmt.Sprintf("{ %s }", a.v) - } - a.v = fmt.Sprintf("[%s%s]", s, strings.TrimSpace(a.v)) -} - -func (a *assembler) installTo(expr ast.Expr) error { - switch v := expr.(type) { - case *ast.IndexExpr: - if err := a.installTo(v.Index); err != nil { - return err - } - if err := a.installTo(v.X); err != nil { - return err - } - case *ast.SelectorExpr: - if ident, ok := v.Sel.(*ast.Ident); ok { - if err := a.installTo(ident); err != nil { - return err - } - } else { - return errors.New("invalid sel type in selector") - } - if err := a.installTo(v.X); err != nil { - return err - } - - case *ast.Ident: - a.fill2Path(v.String()) - case *ast.BasicLit: - switch v.Kind { - case token.STRING: - a.fill2Path(v.Value) - case token.INT: - idex, _ := strconv.Atoi(v.Value) - a.fill2Array(idex) - default: - return errors.New("invalid path") - } - default: - return errors.New("invalid path") - } - return nil -} - // makePath creates a Path from a sequence of string. func makePath(paths ...string) string { mergedPath := "" diff --git a/pkg/cue/model/value/value_test.go b/pkg/cue/model/value/value_test.go index fc50379..8aa547c 100644 --- a/pkg/cue/model/value/value_test.go +++ b/pkg/cue/model/value/value_test.go @@ -958,7 +958,7 @@ func TestFillByScript(t *testing.T) { { name: "insert array to array", raw: ` -a: b: c: [{x: 100}, {x: 101}, {x: 102}, ...]`, +a: b: c: [{x: 100}, {x: 101}, {x: 102}]`, path: "a.b.c[0].value", v: `"foo"`, expected: `a: { @@ -970,7 +970,7 @@ a: b: c: [{x: 100}, {x: 101}, {x: 102}, ...]`, x: 101 }, { x: 102 - }, ...] + }] } } `, @@ -986,9 +986,9 @@ a: b: c: [{x: 100}, {x: 101}, {x: 102}, ...]`, y: [{ name: "key" value: "foo" - }, ...] + }] } - }, ...] + }] } `, }, @@ -1021,9 +1021,9 @@ a: b: c: [{x: 100}, {x: 101}, {x: 102}, ...]`, x: { y: [{ name: "key" - }, ...] + }] } - }, ...] + }] c: { x: "foo" } @@ -1057,35 +1057,28 @@ a: b: c: [{x: 100}, {x: 101}, {x: 102}, ...]`, raw: `a: b: [{x: 100},...]`, path: "a.b[1]+1", v: `{name: "foo"}`, - err: "invalid path", + err: "invalid path: invalid label a.b[1]+1 ", }, { name: "invalid path [float]", raw: `a: b: [{x: 100},...]`, path: "a.b[0.1]", v: `{name: "foo"}`, - err: "invalid path", - }, - { - name: "invalid value", - raw: `a: b: [{x: y:[{name: "key"}]}]`, - path: "a.b[0].x.y[0].value", - v: `foo`, - err: "remake assembler value: a.b.x.y.value: reference \"foo\" not found", + err: "invalid path: invalid literal 0.1", }, { name: "conflict merge", raw: `a: b: [{x: y:[{name: "key"}]}]`, path: "a.b[0].x.y[0].name", v: `"foo"`, - err: "remake value: a.b.0.x.y.0.name: conflicting values \"foo\" and \"key\"", + err: "a.b.0.x.y.0.name: conflicting values \"foo\" and \"key\"", }, { - name: "filled value with wrong cue format", + name: "filled value with incompatible list lengths", raw: `a: b: [{x: y:[{name: "key"}]}]`, - path: "a.b[0].x.y[0].value", - v: `*+-`, - err: "remake assembler value: expected operand, found '}'", + path: "a.b[3].x.y[0].value", + v: `"foo"`, + err: "a.b: incompatible list lengths (1 and 5)", }, } @@ -1093,7 +1086,9 @@ a: b: c: [{x: 100}, {x: 101}, {x: 102}, ...]`, r := require.New(t) v, err := NewValue(errCase.raw, nil, "") r.NoError(err) - err = v.fillRawByScript(errCase.v, errCase.path) + errV, err := v.MakeValue(errCase.v) + r.NoError(err) + err = v.FillValueByScript(errV, errCase.path) r.Equal(errCase.err, err.Error()) } diff --git a/pkg/tasks/custom/task_test.go b/pkg/tasks/custom/task_test.go index 775bff0..361caad 100644 --- a/pkg/tasks/custom/task_test.go +++ b/pkg/tasks/custom/task_test.go @@ -260,7 +260,7 @@ close({ r.NoError(err) run, err := gen(step, &types.TaskGeneratorOptions{}) r.NoError(err) - status, operation, err := run.Run(wfCtx, &types.TaskRunOptions{}) + status, operation, _ := run.Run(wfCtx, &types.TaskRunOptions{}) switch step.Name { case "input-err": r.Equal(status.Message, "parameter.score.x: conflicting values 100 and 101")