Skip to content
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

fix(dag): Running hooks should be able to refer to output of earlier tasks #13639

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chengjoey
Copy link
Contributor

Fixes #13619

Motivation

running task in a dag template don't have outputs, scope has earlier tasks outputs

resolveExitTmplArgument will check if outputs is empty, so we don't need to check it upstream of the function

func (woc *wfOperationCtx) resolveExitTmplArgument(args wfv1.Arguments, prefix string, outputs *wfv1.Outputs, scope *wfScope) (wfv1.Arguments, error) {
if scope == nil {
scope = createScope(nil)
}
if outputs != nil {
for _, param := range outputs.Parameters {
value := ""
if param.Value != nil {
value = param.Value.String()
}
scope.addParamToScope(fmt.Sprintf("%s.outputs.parameters.%s", prefix, param.Name), value)
}
for _, arts := range outputs.Artifacts {
scope.addArtifactToScope(fmt.Sprintf("%s.outputs.artifacts.%s", prefix, arts.Name), arts)
}
}
stepBytes, err := json.Marshal(args)

@agilgur5 agilgur5 changed the title fix(controller): running state hooks in a dag template can not refer to output of earlier tasks fix(dag): Running hooks should be able to refer to output of earlier tasks Sep 21, 2024
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find!

Can you add a test or two (negative and positive case) for this?

Also please note how I changed the title of your PR -- the PR commit should state what it does, not the problem. The issue states the problem, the PR states the fix

@agilgur5
Copy link
Member

agilgur5 commented Sep 21, 2024

Also @chengjoey would you be interested in becoming an Argoproj Member? I can sponsor you and I'm sure we can get another sponsor too. Appreciate your contributions and hope to see more!

@chengjoey
Copy link
Contributor Author

Can you add a test or two (negative and positive case) for this?

okay, i will add tests soon

@chengjoey
Copy link
Contributor Author

Also @chengjoey would you be interested in becoming an Argoproj Member? I can sponsor you and I'm sure we can get another sponsor too. Appreciate your contributions and hope to see more!

yeah, thanks! @agilgur5

@chengjoey
Copy link
Contributor Author

Can you add a test or two (negative and positive case) for this?

done~

@chengjoey chengjoey force-pushed the fix/running-hook-refer-outputs branch 4 times, most recently from 82a311e to de33ef6 Compare September 26, 2024 11:03
@chengjoey chengjoey force-pushed the fix/running-hook-refer-outputs branch from de33ef6 to 68e7982 Compare October 2, 2024 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hooks in DAG cannot refer to output of earlier tasks in some cases
2 participants