diff --git a/README.md b/README.md index 6d7d7f3c..e79b8d0f 100644 --- a/README.md +++ b/README.md @@ -697,6 +697,52 @@ It can be declared in the template like this: foreach_strategy: "sequence" ``` +When writing `skip` conditions on loops, an additional property `foreach` can be added. It can have two values: +- `children`: default value. If no value is set, this value is used. The condition will be run on every iteration of the foreach loop; +- `parent`: the condition will be run on the step itself before creating its children. + +For example: + +```yaml +foreach: '{{.step.aPreviousStep.output.ids | toJson}}' +action: + type: echo + configuration: + output: + url: '{{ .iterator }}' +conditions: + - type: skip + foreach: children # <- this line can be omitted + if: + - value: '{{ .iterator }}' + operator: EQ + expected: '{{ .step.something.output.dontTouchId }}' + then: + this: PRUNE +``` + +will be run on every children and skip the child by pruning it the condition is true. And + +```yaml +foreach: '{{.step.aPreviousStep.output.ids | toJson}}' +action: + type: echo + configuration: + output: + url: '{{ .iterator }}' +conditions: + - type: skip + foreach: parent + if: + - value: '{{ step.previousCheck.output.result }}' + operator: EQ + expected: 'already_done' + then: + this: PRUNE +``` + +will be run before creating any children, by pruning the parent. + #### Resources Resources are a way to restrict the concurrency factor of operations, to control the throughput and avoid dangerous behavior (e.g. flooding the targets). diff --git a/engine/engine.go b/engine/engine.go index f54ad161..302529e0 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -660,6 +660,13 @@ func runAvailableSteps(dbp zesty.DBProvider, modifiedSteps map[string]bool, res // prepare step s.Name = name if s.ForEach != "" { // loop step + // run "skip" step conditions in step in in todo or to_retry + switch s.State { + case step.StateTODO, step.StateToRetry: + step.PreRun(s, res.Values, resolutionStateSetter(res, preRunModifiedSteps), executedSteps) + _ = commit(dbp, res, nil) + } + switch s.State { case step.StateTODO: expanded++ @@ -751,12 +758,19 @@ func expandStep(s *step.Step, res *resolution.Resolution) { // to be shared between multiple steps dependencies := make([]string, len(s.Dependencies)) customStates := make([]string, len(s.CustomStates)) - conditions := make([]*condition.Condition, len(s.Conditions)) resources := make([]string, len(s.Resources)) + conditions := []*condition.Condition{} + copy(dependencies, s.Dependencies) copy(customStates, s.CustomStates) - copy(conditions, s.Conditions) copy(resources, s.Resources) + for _, c := range s.Conditions { + // Only copy skip conditions that are flagged with foreach: children + if c.Type == condition.SKIP && c.ForEach != condition.ForEachChildren { + continue + } + conditions = append(conditions, c) + } res.Steps[childStepName] = &step.Step{ Name: childStepName, diff --git a/engine/engine_test.go b/engine/engine_test.go index 70646365..45d137f7 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -194,11 +194,6 @@ func templateFromYAML(dbp zesty.DBProvider, filename string) (*tasktemplate.Task return tasktemplate.LoadFromName(dbp, tmpl.Name) } -type lintingAndValidationTest struct { - NilResolution bool - NilError bool -} - func TestSimpleTemplate(t *testing.T) { input := map[string]interface{}{ "foo": "bar", @@ -333,41 +328,48 @@ func TestStepMaxRetries(t *testing.T) { } func TestLintingAndValidation(t *testing.T) { - expectedResult := map[string]lintingAndValidationTest{ - "lintingError.yaml": {true, false}, - "lintingRootKey.yaml": {true, false}, - "lintingReservedStep.yaml": {true, false}, - "customStates.yaml": {true, false}, - "forbiddenStateImpact.yaml": {true, false}, - "stepDetailsLintingError.yaml": {true, false}, - "circularDependencies.yaml": {true, false}, - "selfDependency.yaml": {true, false}, - "orphanDependencies.yaml": {true, false}, - "functionEchoHelloWorldError.yaml": {true, false}, - - "lintingInfiniteOk.yaml": {false, true}, - "lintingObject.yaml": {false, true}, - "allowedStateImpact.yaml": {false, true}, - "functionEchoHelloWorld.yaml": {false, true}, - "functionCustomState.yaml": {false, true}, - "functionPreHook.yaml": {false, true}, - "functionEchoTemplatedOutput.yaml": {false, true}, + expectedResult := map[string]struct { + nilResolution bool + errstr string + }{ + "lintingError.yaml": {true, `Variable notfound does not exist`}, + "lintingRootKey.yaml": {true, `Variable grault does not exist`}, + "lintingReservedStep.yaml": {true, `'this' step name is reserved`}, + "customStates.yaml": {true, `Custom state "SERVER_ERROR" is not allowed as it's a reserved state`}, + "forbiddenStateImpact.yaml": {true, `Step condition cannot impact the state of step stepTwo, only those who belong to the dependency chain are allowed`}, + "stepDetailsLintingError.yaml": {true, `Wrong step key: stepNotFound`}, + "circularDependencies.yaml": {true, `Invalid: circular dependency [stepOne stepThree stepTwo] <-> step`}, // Last step name is random + "selfDependency.yaml": {true, `Invalid: circular dependency [stepOne] <-> stepOne`}, + "orphanDependencies.yaml": {true, `Invalid dependency, no step with that name: "stepTwo"`}, + "functionEchoHelloWorldError.yaml": {true, `Invalid executor action: missing function_args "name"`}, + "conditionForeachSkipOnly.yaml": {true, `Step condition can set foreach on a skip condition`}, + "conditionForeachInvalid.yaml": {true, `Unknown condition foreach: invalid`}, + "conditionForeachStepNotForeach.yaml": {true, `Step condition cannot set foreach on a non-foreach step`}, + + "lintingInfiniteOk.yaml": {false, ""}, + "lintingObject.yaml": {false, ""}, + "allowedStateImpact.yaml": {false, ""}, + "functionEchoHelloWorld.yaml": {false, ""}, + "functionCustomState.yaml": {false, ""}, + "functionPreHook.yaml": {false, ""}, + "functionEchoTemplatedOutput.yaml": {false, ""}, } for template, testCase := range expectedResult { t.Run(template, func(t *testing.T) { res, err := createResolution(template, map[string]interface{}{}, nil) - if testCase.NilResolution { + if testCase.nilResolution { assert.Nil(t, res) } else { assert.NotNil(t, res) } - if testCase.NilError { + if testCase.errstr == "" { assert.Nil(t, err) } else { - assert.NotNil(t, err) + require.NotNil(t, err) + assert.Contains(t, err.Error(), testCase.errstr) } }) } @@ -711,7 +713,7 @@ func TestForeach(t *testing.T) { } func TestForeachWithChainedIterations(t *testing.T) { - _, require := td.AssertRequire(t) + assert, require := td.AssertRequire(t) res, err := createResolution("foreach.yaml", map[string]interface{}{ "list": []interface{}{"a", "b", "c", "d", "e"}, }, nil) @@ -733,6 +735,7 @@ func TestForeachWithChainedIterations(t *testing.T) { Then: map[string]string{ "this": "SERVER_ERROR", }, + ForEach: condition.ForEachChildren, }, ) res.Steps["generateItems"].ForEachStrategy = "sequence" @@ -744,24 +747,24 @@ func TestForeachWithChainedIterations(t *testing.T) { require.Nil(err) require.Cmp(res.State, resolution.StateError) - td.Cmp(t, res.Steps["emptyLoop"].State, step.StateDone) // running on empty collection is ok - td.Cmp(t, res.Steps["concatItems"].State, step.StateTODO) - td.Cmp(t, res.Steps["finalStep"].State, step.StateTODO) - td.Cmp(t, res.Steps["bStep"].State, "B") - td.Cmp(t, res.Steps["generateItems-0"].State, step.StateDone) - td.Cmp(t, res.Steps["generateItems-1"].State, step.StateDone) - td.Cmp(t, res.Steps["generateItems-2"].State, step.StateDone) - td.Cmp(t, res.Steps["generateItems-3"].State, step.StateServerError) - td.Cmp(t, res.Steps["generateItems-4"].State, step.StateTODO) - td.CmpLen(t, res.Steps["generateItems-0"].Dependencies, 0) - td.Cmp(t, res.Steps["generateItems-1"].Dependencies, []string{"generateItems-0"}) - td.Cmp(t, res.Steps["generateItems-2"].Dependencies, []string{"generateItems-1"}) - td.Cmp(t, res.Steps["generateItems-3"].Dependencies, []string{"generateItems-2"}) - td.Cmp(t, res.Steps["generateItems-4"].Dependencies, []string{"generateItems-3"}) + assert.Cmp(res.Steps["emptyLoop"].State, step.StateDone) // running on empty collection is ok + assert.Cmp(res.Steps["concatItems"].State, step.StateTODO) + assert.Cmp(res.Steps["finalStep"].State, step.StateTODO) + assert.Cmp(res.Steps["bStep"].State, "B") + assert.Cmp(res.Steps["generateItems-0"].State, step.StateDone) + assert.Cmp(res.Steps["generateItems-1"].State, step.StateDone) + assert.Cmp(res.Steps["generateItems-2"].State, step.StateDone) + assert.Cmp(res.Steps["generateItems-3"].State, step.StateServerError) + assert.Cmp(res.Steps["generateItems-4"].State, step.StateTODO) + assert.Len(res.Steps["generateItems-0"].Dependencies, 0) + assert.Cmp(res.Steps["generateItems-1"].Dependencies, []string{"generateItems-0"}) + assert.Cmp(res.Steps["generateItems-2"].Dependencies, []string{"generateItems-1"}) + assert.Cmp(res.Steps["generateItems-3"].Dependencies, []string{"generateItems-2"}) + assert.Cmp(res.Steps["generateItems-4"].Dependencies, []string{"generateItems-3"}) } func TestForeachWithChainedIterationsWithDepOnParent(t *testing.T) { - _, require := td.AssertRequire(t) + assert, require := td.AssertRequire(t) res, err := createResolution("foreach.yaml", map[string]interface{}{ "list": []interface{}{"a", "b", "c", "d", "e"}, }, nil) @@ -784,6 +787,7 @@ func TestForeachWithChainedIterationsWithDepOnParent(t *testing.T) { Then: map[string]string{ "this": "SERVER_ERROR", }, + ForEach: condition.ForEachChildren, }, ) res.Steps["generateItems"].ForEachStrategy = "sequence" @@ -795,39 +799,64 @@ func TestForeachWithChainedIterationsWithDepOnParent(t *testing.T) { require.Nil(err) require.Cmp(res.State, resolution.StateError) - td.Cmp(t, res.Steps["emptyLoop"].State, step.StateDone) // running on empty collection is ok - td.Cmp(t, res.Steps["concatItems"].State, step.StateTODO) - td.Cmp(t, res.Steps["finalStep"].State, step.StateTODO) - td.Cmp(t, res.Steps["bStep"].State, "B") - td.Cmp(t, res.Steps["generateItems-0"].State, step.StateDone) - td.Cmp(t, res.Steps["generateItems-1"].State, step.StateDone) - td.Cmp(t, res.Steps["generateItems-2"].State, step.StateDone) - td.Cmp(t, res.Steps["generateItems-3"].State, step.StateServerError) - td.Cmp(t, res.Steps["generateItems-4"].State, step.StateTODO) - td.Cmp(t, res.Steps["generateItems"].Dependencies, []string{"emptyLoop", "generateItems-0:ANY", "generateItems-1:ANY", "generateItems-2:ANY", "generateItems-3:ANY", "generateItems-4:ANY"}) - td.Cmp(t, res.Steps["generateItems-0"].Dependencies, []string{"emptyLoop"}) - td.Cmp(t, res.Steps["generateItems-1"].Dependencies, []string{"emptyLoop", "generateItems-0"}) - td.Cmp(t, res.Steps["generateItems-2"].Dependencies, []string{"emptyLoop", "generateItems-1"}) - td.Cmp(t, res.Steps["generateItems-3"].Dependencies, []string{"emptyLoop", "generateItems-2"}) - td.Cmp(t, res.Steps["generateItems-4"].Dependencies, []string{"emptyLoop", "generateItems-3"}) + assert.Cmp(res.Steps["emptyLoop"].State, step.StateDone) // running on empty collection is ok + assert.Cmp(res.Steps["concatItems"].State, step.StateTODO) + assert.Cmp(res.Steps["finalStep"].State, step.StateTODO) + assert.Cmp(res.Steps["bStep"].State, "B") + assert.Cmp(res.Steps["generateItems-0"].State, step.StateDone) + assert.Cmp(res.Steps["generateItems-1"].State, step.StateDone) + assert.Cmp(res.Steps["generateItems-2"].State, step.StateDone) + assert.Cmp(res.Steps["generateItems-3"].State, step.StateServerError) + assert.Cmp(res.Steps["generateItems-4"].State, step.StateTODO) + assert.Cmp(res.Steps["generateItems"].Dependencies, []string{"emptyLoop", "generateItems-0:ANY", "generateItems-1:ANY", "generateItems-2:ANY", "generateItems-3:ANY", "generateItems-4:ANY"}) + assert.Cmp(res.Steps["generateItems-0"].Dependencies, []string{"emptyLoop"}) + assert.Cmp(res.Steps["generateItems-1"].Dependencies, []string{"emptyLoop", "generateItems-0"}) + assert.Cmp(res.Steps["generateItems-2"].Dependencies, []string{"emptyLoop", "generateItems-1"}) + assert.Cmp(res.Steps["generateItems-3"].Dependencies, []string{"emptyLoop", "generateItems-2"}) + assert.Cmp(res.Steps["generateItems-4"].Dependencies, []string{"emptyLoop", "generateItems-3"}) } func TestForeachWithPreRun(t *testing.T) { - input := map[string]interface{}{} - res, err := createResolution("foreachAndPreRun.yaml", input, nil) - require.Nilf(t, err, "expecting nil error, got %s", err) - require.NotNil(t, res) + for _, switchToToRetry := range []bool{false, true} { + t.Run(fmt.Sprintf("%s-%t", t.Name(), switchToToRetry), func(t *testing.T) { + input := map[string]interface{}{} + res, err := createResolution("foreachAndPreRun.yaml", input, nil) + require.Nilf(t, err, "expecting nil error, got %s", err) + require.NotNil(t, res) + + if switchToToRetry { + for _, st := range []string{"stepForeachPrune", "stepDepOnForeachPrune", "stepForeachPruneParentTask", "stepDepOnForeachPruneParentTask"} { + res.Steps[st].State = step.StateToRetry + } + require.NoError(t, updateResolution(res)) + } - res, err = runResolution(res) + res, err = runResolution(res) - require.Nilf(t, err, "got error %s", err) - require.NotNil(t, res) - assert.Equal(t, resolution.StateDone, res.State) - for _, st := range []string{"stepForeachNoDep", "stepSkippedNoDep", "stepNoDep", "stepForeachWithDep", "stepSkippedWithDep"} { - assert.Equal(t, step.StateDone, res.Steps[st].State) - } - for _, st := range []string{"stepDep", "stepDep2"} { - assert.Equal(t, step.StatePrune, res.Steps[st].State) + require.Nilf(t, err, "got error %s", err) + require.NotNil(t, res) + assert.Equal(t, resolution.StateDone, res.State) + for _, st := range []string{"stepForeachNoDep", "stepSkippedNoDep", "stepNoDep", "stepForeachWithDep", "stepSkippedWithDep"} { + assert.Equal(t, step.StateDone, res.Steps[st].State) + } + for _, st := range []string{"stepDep", "stepDep2"} { + assert.Equal(t, step.StatePrune, res.Steps[st].State) + } + + // skip prune on a foreach step's children means: + // - foreach children are set to prune + // - the foreach step itself is set to done + // - the dependencies are not pruned + assert.Equal(t, step.StateDone, res.Steps["stepForeachPrune"].State) + assert.Equal(t, step.StateDone, res.Steps["stepDepOnForeachPrune"].State) + + // skip prune on a foreach step itself means: + // - foreach children are not generated + // - the foreach step itself is set to prune + // - the dependencies are pruned + assert.Equal(t, step.StatePrune, res.Steps["stepForeachPruneParentTask"].State) + assert.Equal(t, step.StatePrune, res.Steps["stepDepOnForeachPruneParentTask"].State) + }) } } diff --git a/engine/step/condition/stepcondition.go b/engine/step/condition/stepcondition.go index f4ef387e..2f705031 100644 --- a/engine/step/condition/stepcondition.go +++ b/engine/step/condition/stepcondition.go @@ -3,6 +3,7 @@ package condition import ( "fmt" + "github.com/juju/errors" "github.com/ovh/utask/engine/values" ) @@ -13,15 +14,47 @@ const ( CHECK = "check" ) +const ( + // ForEachChildren executes the condition on the children of a foreach step. + // The children are created, the condition is copied in them, then run. + // This is the default value. + ForEachChildren = "children" + // ForEachParent executes the condition on the foreach step itself. + ForEachParent = "parent" +) + // Condition defines a condition to be evaluated before or after a step's action type Condition struct { Type string `json:"type"` If []*Assert `json:"if"` Then map[string]string `json:"then"` Final bool `json:"final"` + ForEach string `json:"foreach"` Message string `json:"message"` } +// Valid asserts that a condition's definition is valid +// ie. the type and foreach are among the accepted values listed above +func (c *Condition) Valid() error { + if c == nil { + return nil + } + + switch c.Type { + case SKIP, CHECK: + default: + return errors.BadRequestf("Unknown condition type: %s", c.Type) + } + + switch c.ForEach { + case "", ForEachChildren, ForEachParent: + default: + return errors.BadRequestf("Unknown condition foreach: %s", c.ForEach) + } + + return nil +} + // Eval runs the condition against a set of values, evaluating the underlying Condition func (sc *Condition) Eval(v *values.Values, item interface{}, stepName string) error { for _, c := range sc.If { diff --git a/engine/step/step.go b/engine/step/step.go index 08fa3fb9..7a0f96d6 100644 --- a/engine/step/step.go +++ b/engine/step/step.go @@ -489,6 +489,10 @@ func PreRun(st *Step, values *values.Values, ss StateSetter, executedSteps map[s if sc.Type != condition.SKIP { continue } + if st.ForEach != "" && sc.ForEach != condition.ForEachParent { + continue + } + if err := sc.Eval(values, st.Item, st.Name); err != nil { if _, ok := err.(condition.ErrConditionNotMet); ok { logrus.Debugf("PreRun: Step [%s] condition eval: %s", st.Name, err) @@ -576,7 +580,6 @@ func AfterRun(st *Step, values *values.Values, ss StateSetter) { // - validates the provided json schema for result validation // - checks dependency declaration against the task's execution tree func (st *Step) ValidAndNormalize(name string, baseConfigs map[string]json.RawMessage, steps map[string]*Step) error { - if name == stepRefThis { return errors.BadRequestf("'%s' step name is reserved", stepRefThis) } @@ -639,6 +642,9 @@ func (st *Step) ValidAndNormalize(name string, baseConfigs map[string]json.RawMe // valid step conditions for _, sc := range st.Conditions { + if st.ForEach != "" && sc.Type == condition.SKIP && sc.ForEach == "" { + sc.ForEach = condition.ForEachChildren + } if err := ValidCondition(sc, name, steps); err != nil { return err } @@ -685,7 +691,8 @@ func (st *Step) ValidAndNormalize(name string, baseConfigs map[string]json.RawMe // no circular dependencies, sourceChain := dependenciesChain(steps, st.Dependencies) if utils.ListContainsString(sourceChain, name) { - return errors.BadRequestf("Invalid: circular dependency %v <-> %s", sourceChain, st.Name) + sort.Strings(sourceChain) + return errors.BadRequestf("Invalid: circular dependency %v <-> %s", sourceChain, name) } return nil diff --git a/engine/step/stepcondition.go b/engine/step/stepcondition.go index ca452418..e5e11466 100644 --- a/engine/step/stepcondition.go +++ b/engine/step/stepcondition.go @@ -8,11 +8,16 @@ import ( // ValidCondition asserts that the definition for a StepCondition is valid func ValidCondition(sc *condition.Condition, stepName string, steps map[string]*Step) error { + if err := sc.Valid(); err != nil { + return err + } + for _, c := range sc.If { if err := c.Valid(); err != nil { return err } } + for thenStep, thenState := range sc.Then { // force the use of "this" for single steps // except in the case of "loop" steps: the condition will belong to its children, @@ -45,6 +50,17 @@ func ValidCondition(sc *condition.Condition, stepName string, steps map[string]* return errors.BadRequestf("Step condition implies invalid state for step %s: %s", thenStep, thenState) } } + + if sc.ForEach != "" { + if steps[stepName].ForEach == "" { + return errors.BadRequestf("Step condition cannot set foreach on a non-foreach step") + } + + if sc.Type != condition.SKIP { + return errors.BadRequestf("Step condition can set foreach on a skip condition") + } + } + return nil } diff --git a/engine/templates_tests/conditionForeachInvalid.yaml b/engine/templates_tests/conditionForeachInvalid.yaml new file mode 100644 index 00000000..05d78784 --- /dev/null +++ b/engine/templates_tests/conditionForeachInvalid.yaml @@ -0,0 +1,21 @@ +name: conditionForeachInvalid +description: Invalid value inside foreach on a condition +title_format: "[test] invalid value in condition foreach" +steps: + stepOne: + foreach: '[1, 2]' + description: "step one" + action: + type: echo + configuration: + output: + foo: "bar: {{.iterator}}" + conditions: + - type: skip + foreach: invalid + if: + - value: 1 + operator: EQ + expected: 1 + then: + this: CLIENT_ERROR diff --git a/engine/templates_tests/conditionForeachSkipOnly.yaml b/engine/templates_tests/conditionForeachSkipOnly.yaml new file mode 100644 index 00000000..a11997a0 --- /dev/null +++ b/engine/templates_tests/conditionForeachSkipOnly.yaml @@ -0,0 +1,21 @@ +name: conditionForeachSkipOnly +description: Foreach on condition can only be set on skip confitoins +title_format: "[test] foreach condition only on skip" +steps: + stepOne: + foreach: '[1, 2]' + description: "step one" + action: + type: echo + configuration: + output: + foo: "bar: {{.iterator}}" + conditions: + - type: check + foreach: children + if: + - value: 1 + operator: EQ + expected: 1 + then: + this: CLIENT_ERROR diff --git a/engine/templates_tests/conditionForeachStepNotForeach.yaml b/engine/templates_tests/conditionForeachStepNotForeach.yaml new file mode 100644 index 00000000..0600cdd6 --- /dev/null +++ b/engine/templates_tests/conditionForeachStepNotForeach.yaml @@ -0,0 +1,20 @@ +name: conditionForeachStepNotForeach +description: Cannot put a foreach condition on a non foreach step +title_format: "[test] foreach condition on a non foreach step" +steps: + stepOne: + description: "step one" + action: + type: echo + configuration: + output: + foo: "bar: {{.iterator}}" + conditions: + - type: skip + foreach: children + if: + - value: 1 + operator: EQ + expected: 1 + then: + this: CLIENT_ERROR diff --git a/engine/templates_tests/foreachAndPreRun.yaml b/engine/templates_tests/foreachAndPreRun.yaml index f246efc6..29b4ee79 100644 --- a/engine/templates_tests/foreachAndPreRun.yaml +++ b/engine/templates_tests/foreachAndPreRun.yaml @@ -88,3 +88,52 @@ steps: configuration: output: url: 'https://eu.httpbin.org/delay/1' + stepForeachPrune: + description: a foreach step that prune its children + foreach: '["1","2"]' + action: + type: echo + configuration: + output: + url: 'https://eu.httpbin.org/delay/{{ .iterator }}' + conditions: + - type: skip + if: + - value: 'foo' + operator: EQ + expected: 'foo' + then: + this: PRUNE + stepDepOnForeachPrune: + dependencies: [stepForeachPrune] + description: a dep on a foreach which pruned its children + action: + type: echo + configuration: + output: + ok: "ok" + stepForeachPruneParentTask: + description: a foreach step that prune its children + foreach: '["1","2"]' + action: + type: echo + configuration: + output: + url: 'https://eu.httpbin.org/delay/{{ .iterator }}' + conditions: + - type: skip + foreach: parent # This is the difference between this one and stepForeachPrune + if: + - value: 'foo' + operator: EQ + expected: 'foo' + then: + this: PRUNE + stepDepOnForeachPruneParentTask: + dependencies: [stepForeachPruneParentTask] + description: a dep on a foreach which was pruned + action: + type: echo + configuration: + output: + ok: "ok"