fix: argument of PodName function (fixes #7315) #7316
Conversation
mikutas
force-pushed
the
fix_podname_input
branch
from
af39a8e to
4b552a3
Compare
node has TemplateRef.Template field rather than TemplateName field if using WorkflowTemplate Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
workflow/util/util.go
Outdated
| @@ -814,7 +814,13 @@ func retryWorkflow(ctx context.Context, kubeClient kubernetes.Interface, hydrato | |||
| return nil, errors.InternalErrorf("Workflow cannot be retried with node %s in %s phase", node.Name, node.Phase) | |||
| } | |||
| if node.Type == wfv1.NodeTypePod { | |||
| podName := PodName(wf.Name, node.Name, node.TemplateName, node.ID) | |||
| var templateName string | |||
There was a problem hiding this comment.
I see similar code here, but the template name format is different in the case of TemplateRef. Are we sure this is correct?
argo-workflows/cmd/argo/commands/get.go
Lines 532 to 537 in 7739256
There was a problem hiding this comment.
v2 pod name doesn't contain WorkflowTemplate's name (=TemplateRef.name).
It consists of [workflow name]-[template name]-[hash] .
Workflow
WorkflowTemplates
each full manifest is in #7315
There was a problem hiding this comment.
Hey @mikutas - I'd recommend using a utility function for getting the template name like: https://github.com/argoproj/argo-workflows/blob/master/ui/src/app/shared/pod-name.ts#L49
That utility function could use a test though. Feel free to add one in this PR
address review feedback Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
| } else { | ||
| templateName = node.TemplateName | ||
| } | ||
| podName := PodName(wf.Name, node.Name, templateName, node.ID) |
|
@JPZ13 could I ask you to own the review? |
Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
| @@ -866,6 +867,13 @@ func retryWorkflow(ctx context.Context, kubeClient kubernetes.Interface, hydrato | |||
| return wfClient.Update(ctx, newWF, metav1.UpdateOptions{}) | |||
| } | |||
|
|
|||
| func getTemplateFromNode(node wfv1.NodeStatus) string { | |||
There was a problem hiding this comment.
Looks good! Could you add a test? In the meantime, I'll run it locally to test it out
|
I've confirmed this fix works when running locally. I'll approve once there's a unit test for |
|
This will need back porting to v3.2 |
node has TemplateRef.Template field rather than TemplateName field
fixes #7315
Signed-off-by: mikutas 23391543+mikutas@users.noreply.github.com
Don't bother creating a PR until you've done this:
make pre-commit -Bto fix codegen, lint, and commit message problems.Create your PR as a draft.
does not need to pass.
Tips: