Fix: fix fill array with array in inputs (#86)

* Fix: fix fill array with array in inputs Signed-off-by: FogDong <dongtianxin.tx@alibaba-inc.com> * deprecate fill value by script Signed-off-by: FogDong <dongtianxin.tx@alibaba-inc.com> Signed-off-by: FogDong <dongtianxin.tx@alibaba-inc.com>
kubevela · Jan 3, 2023 · 6b7697b · 6b7697b
1 parent 420b2ea
commit 6b7697b
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 124 deletions.
diff --git 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
@@ -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,42 +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 {
-		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 {
+	newV := val.v.FillPath(FieldPath(path), x.v)
+	if err := newV.Err(); err != nil {
 		return err
 	}
-	raw, err := val.String(sets.ListOpen)
-	if err != nil {
-		return err
-	}
-	v, err := val.MakeValue(raw + "\n" + a.v)
-	if err != nil {
-		return errors.WithMessage(err, "remake value")
-	}
-	if err := v.Error(); err != nil {
-		return err
-	}
-	*val = *v
+	val.v = newV
 	return nil
 }
 
@@ -679,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
@@ -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
+		}]
+	}
 }
 `,
 		},
@@ -1002,9 +1021,9 @@ func TestFillByScript(t *testing.T) {
 		x: {
 			y: [{
 				name: "key"
-			}, ...]
+			}]
 		}
-	}, ...]
+	}]
 	c: {
 		x: "foo"
 	}
@@ -1038,43 +1057,38 @@ func TestFillByScript(t *testing.T) {
 			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 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 value: expected operand, found '}'",
+			path: "a.b[3].x.y[0].value",
+			v:    `"foo"`,
+			err:  "a.b: incompatible list lengths (1 and 5)",
 		},
 	}
 
 	for _, errCase := range errCases {
 		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.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
@@ -260,12 +260,18 @@ 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(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
@@ -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.