-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix: skip validate dynamic templateref #14850
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: skip validate dynamic templateref #14850
Conversation
Signed-off-by: Tianchu Zhao <[email protected]>
Signed-off-by: Tianchu Zhao <[email protected]>
Signed-off-by: Tianchu Zhao <[email protected]>
Joibel
left a comment
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.
Minor suggestion
| tmplCtx, resolvedTmpl, _, err := tmplCtx.ResolveTemplate(ctx, tmplHolder) | ||
| if err != nil { | ||
| if argoerr, ok := err.(errors.ArgoError); ok && argoerr.Code() == errors.CodeNotFound { | ||
| if tmplRef != nil && strings.Contains(tmplRef.Template, "placeholder") { |
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 we make it less likely to mismatch on non-placeholder templates with
| if tmplRef != nil && strings.Contains(tmplRef.Template, "placeholder") { | |
| if tmplRef != nil && strings.HasPrefix(tmplRef.Template, "placeholder") { |
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 dynamic template parameter can be partial, using prefix matching won't cover the following use case.
e.g.
"prod-{{ inputs.parameters.message }}" this will translate to prod-placeholder-x.
If a user uses string:placeholder in templateRef, the worst-case scenario is that they experience an issue during runtime instead of catch it at the validation phase.
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.
Thanks for that, understand
| tmplCtx, resolvedTmpl, _, err := tmplCtx.ResolveTemplate(ctx, tmplHolder) | ||
| if err != nil { | ||
| if argoerr, ok := err.(errors.ArgoError); ok && argoerr.Code() == errors.CodeNotFound { | ||
| if tmplRef != nil && strings.Contains(tmplRef.Template, "placeholder") { |
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.
Thanks for that, understand
Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: Sebastien Dionne <[email protected]>
Signed-off-by: Tianchu Zhao <[email protected]>
|
❌ Cherry-pick failed for 3.6. Please check the workflow logs for details. |
|
❌ Cherry-pick failed for 3.7. Please check the workflow logs for details. |
(cherry picked from commit d1b280e) Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: Alan Clucas <[email protected]>
(cherry picked from commit d1b280e) Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: Alan Clucas <[email protected]>
(cherry picked from commit d1b280e) Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: Alan Clucas <[email protected]>
(cherry picked from commit d1b280e) Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: Alan Clucas <[email protected]>
…15082) Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Co-authored-by: Tianchu Zhao <[email protected]>
…15081) Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Co-authored-by: Tianchu Zhao <[email protected]>
Signed-off-by: Tianchu Zhao <[email protected]>
Fixes #14042 #13773
Before this fix, the validation will failed if workflow contains dynamic templateRef.
Decide to skip validate dynamic templateRef instead of find a way to validate them due to the complexity and cases where its impossible to validate
Motivation
Modifications
Verification
Added test for dynamic templateRef validation. copy from #14053
Added test to validate the regression introduced by previous templateRef validation implementation attempt
Documentation