-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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: correct pod names for inline templates. Fixes #12895 #12928
Conversation
Signed-off-by: AlbeeSo <suyashi1321@163.com>
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.
Can you add a test?
I see the UI logic for pod names has diverged a bit -- #7605 added a This logic could perhaps be simplified to just |
if !strings.Contains(nodeName, ".inline") && templateName != "" { | ||
prefix = fmt.Sprintf("%s-%s", workflowName, templateName) |
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.
if !strings.Contains(nodeName, ".inline") && templateName != "" { | |
prefix = fmt.Sprintf("%s-%s", workflowName, templateName) | |
if templateName != "" { | |
prefix = fmt.Sprintf("%s-%s", prefix, templateName) |
confirmed that this works and will match the UI as well -- see also #12964 .
an inline template will have no templateName
, so it is equivalent to the previous logic.
also changed the prefix
assignment a bit to simplify it
This PR has been automatically marked as stale because it has not had recent activity and needs further changes. It will be closed if no further activity occurs. |
This PR has been closed due to inactivity and lack of changes. If you would like to still work on this PR, please address the review comments and re-open. |
Fixes #12895
Motivation
argo-workflows/workflow/util/pod_name.go
Line 60 in 87a2041
Incorrect pod names of inline templates will lead to empty UI logs problem.
Modifications
Verification
Test with the workflow mentioned in issue, got result like image post below: