-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: populate scope with empty values for outputs of skipped/omitted … #15932
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
cae806c
399a91f
5cc2576
421e737
b2dd26f
8395ecd
7ad6ea6
ad343c6
3e3742d
9ca90af
59a08b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| apiVersion: argoproj.io/v1alpha1 | ||
| kind: Workflow | ||
| metadata: | ||
| generateName: steps-skipped-output-ref- | ||
| spec: | ||
| entrypoint: main | ||
| templates: | ||
| - name: main | ||
| steps: | ||
| - - name: job1 | ||
| template: run-job | ||
| - - name: job2 | ||
| template: run-job | ||
| when: "\"{{steps.job1.outputs.parameters.status}}\" == \"FAILED\"" | ||
| - - name: job2-error-handler | ||
| template: run-job | ||
| when: "\"{{steps.job2.outputs.parameters.status}}\" != \"SUCCESS\" && \"{{steps.job1.outputs.parameters.status}}\" == \"SUCCESS\"" | ||
| - name: run-job | ||
| outputs: | ||
| parameters: | ||
| - name: status | ||
| valueFrom: | ||
| path: /tmp/status.txt | ||
| container: | ||
| image: argoproj/argosay:v2 | ||
| args: ["echo", "SUCCESS", "/tmp/status.txt"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,6 +163,28 @@ func (woc *wfOperationCtx) executeSteps(ctx context.Context, nodeName string, tm | |
| woc.buildLocalScope(stepsCtx.scope, prefix, sgNode) | ||
| } else { | ||
| woc.buildLocalScope(stepsCtx.scope, prefix, childNode) | ||
|
|
||
| if (childNode.Phase == wfv1.NodeSkipped || childNode.Phase == wfv1.NodeOmitted) && childNode.Outputs == nil { | ||
| _, stepTmpl, _, resolveErr := stepsCtx.tmplCtx.ResolveTemplate(ctx, &step) | ||
|
|
||
| if resolveErr != nil { | ||
| woc.log.WithError(resolveErr).Debug(ctx, "failed to resolve template for skipped step, outputs will not be populated in scope") | ||
| } | ||
|
|
||
| if resolveErr == nil && stepTmpl != nil { | ||
| for _, param := range stepTmpl.Outputs.Parameters { | ||
| key := fmt.Sprintf("%s.outputs.parameters.%s", prefix, param.Name) | ||
| stepsCtx.scope.addSkippedParamToScope(key) | ||
| } | ||
| for _, artifact := range stepTmpl.Outputs.Artifacts { | ||
| key := fmt.Sprintf("%s.outputs.artifacts.%s", prefix, artifact.Name) | ||
| stepsCtx.scope.addArtifactToScope(key, wfv1.Artifact{}) | ||
| } | ||
| if stepTmpl.Outputs.Result != nil { | ||
| stepsCtx.scope.addSkippedParamToScope(fmt.Sprintf("%s.outputs.result", prefix)) | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fix handles for _, artifact := range tmpl.Outputs.Artifacts {
key := fmt.Sprintf("%s.outputs.artifacts.%s", prefix, artifact.Name)
stepsCtx.scope.addArtifactToScope(key, wfv1.Artifact{})
}This mirrors how the parameter case is handled above.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. applied |
||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test exercises
NodeSkippedbut neverNodeOmitted. Both phases are handled in the fix. SinceOmittedis a distinct node phase that arises in DAG templates (notsteps), it may require a separate workflow fixture, but at least a comment explaining why it isn't tested would clarify the intent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry for that, but honestly its my first time I wrote golang and did something in argo -
honestly - I just fixed the case which blocks us, that's the reason, I can try to fix other cases as well but this can take some time.
If its enough to write comment, can you please sketch some comment I can paste ? I will probably not create anything meaningful :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@utafrali I pushed following comment on that test