fix: populate optional artifacts with empty data. #14267 #14494
Conversation
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
isubasinghe
force-pushed
the
fix-optional-artifact
branch
from
6deae94 to
4a059e3
Compare
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
isubasinghe
changed the title
|
I was wondering if there should also be a |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where missing optional artifacts are now populated with empty data to ensure they can always be referenced.
- Modify the buildLocalScope logic to include default empty artifacts based on the template outputs.
- Update calls to addOutputsToLocalScope in both steps and DAG contexts with the new parameter for template artifacts.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| workflow/controller/steps.go | Updates call to addOutputsToLocalScope with an empty artifact parameter. |
| workflow/controller/scope.go | Adds hasArtifact helper to check for artifact presence in the scope. |
| workflow/controller/operator.go | Extends buildLocalScope and addOutputsToLocalScope to process template artifacts. |
| workflow/controller/dag.go | Modifies addOutputsToLocalScope call for DAG tasks by passing a nil tmplArts. |
Comments suppressed due to low confidence (1)
workflow/controller/operator.go:3294
- The loop resets several fields of tmplArts artifacts; please verify that this mutation is intended and does not inadvertently discard critical artifact data.
for _, art := range tmplArts {
| @@ -657,7 +657,7 @@ func (woc *wfOperationCtx) executeDAGTask(ctx context.Context, dagCtx *dagContex | |||
| func (woc *wfOperationCtx) buildLocalScopeFromTask(dagCtx *dagContext, task *wfv1.DAGTask) (*wfScope, error) { | |||
| // build up the scope | |||
| scope := createScope(dagCtx.tmpl) | |||
| woc.addOutputsToLocalScope("workflow", woc.wf.Status.Outputs, scope) | |||
| woc.addOutputsToLocalScope("workflow", woc.wf.Status.Outputs, scope, nil) | |||
There was a problem hiding this comment.
[nitpick] Passing nil for tmplArts is valid, but consider using an empty artifact container (e.g., wfv1.Artifacts{}) for improved clarity and consistency with other calls.
| woc.addOutputsToLocalScope("workflow", woc.wf.Status.Outputs, scope, nil) | |
| woc.addOutputsToLocalScope("workflow", woc.wf.Status.Outputs, scope, wfv1.Artifacts{}) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This whole thing still feels like it's the wrong fix. It's fixing outputs, which are fine, to mask a problem in inputs, which are broken.
We have a problem with input artifacts, marked as optional, causing the processing of those to cause an error. They're optional, so they shouldn't cause an error, and this doesn't fix that.
This feels like a hack, undoing some of #13678 which made sure that optional output artifacts which didn't exist really didn't exist.
You've suggested a flag exists on the artifact rather than an empty value as you've implemented here - but that's pretty much already determinable with the code prior to this - it just doesn't exist.
If you'd implemented exists where would you check and use that information - that's where I think this fix should be applied?
We're missing the test case as a regression test too.
| @@ -657,7 +657,7 @@ func (woc *wfOperationCtx) executeDAGTask(ctx context.Context, dagCtx *dagContex | |||
| func (woc *wfOperationCtx) buildLocalScopeFromTask(dagCtx *dagContext, task *wfv1.DAGTask) (*wfScope, error) { | |||
| // build up the scope | |||
| scope := createScope(dagCtx.tmpl) | |||
| woc.addOutputsToLocalScope("workflow", woc.wf.Status.Outputs, scope) | |||
| woc.addOutputsToLocalScope("workflow", woc.wf.Status.Outputs, scope, nil) | |||
| @@ -3222,12 +3223,22 @@ func (woc *wfOperationCtx) requeueIfTransientErr(err error, nodeName string) (*w | |||
| // buildLocalScope adds all of a nodes outputs to the local scope with the given prefix, as well | |||
| // as the global scope, if specified with a globalName | |||
| func (woc *wfOperationCtx) buildLocalScope(scope *wfScope, prefix string, node *wfv1.NodeStatus) { | |||
|
|
|||
Fixes #14267
Motivation
The current code allows for artifacts to be absent but allows for optional variables to be referenced on certain occasions. This code change adds all optional outputs to the scope if they do not exist.
Modifications
This change modifies the
buildLocalScopeto obtain the template, if an output fails to appear in the node outputs, we will populate these keys with empty artifacts.Verification
Tested manually using a debugger.
Documentation
While this does change how variables work, there isn't any documentation changes in this PR.