Added "tpl" template function #2350
Conversation
| tplChart := &chart.Chart{ | ||
| Metadata: &chart.Metadata{Name: "TplFunction"}, | ||
| Templates: []*chart.Template{ | ||
| {Name: "templates/base", Data: []byte(`Evaluate tpl {{tpl "Value: {{ .Values.value}}" .}}`)}, |
There was a problem hiding this comment.
This looks great!
Can you also test a template that uses template functions (like quote) and defined templates ({{ define "foo" }})? Something like tpl "Value: {{ .Values.value | quote }}" and tpl "{{ include 'foo' }}".
It looks like those should both work. But I think it's worth having a safety net in the tests. We need to be sure that now and in the future those two items are available within a nested template.
There was a problem hiding this comment.
While testing the "include" function I ran into the following error since the other template is only available for the outer render context. Is there an global render context I could use to pass all registered templates to the rendering of the "tpl"-function?
tplChartWithInclude := &chart.Chart{
Metadata: &chart.Metadata{Name: "TplFunction"},
Templates: []*chart.Template{
{Name: "templates/base", Data: []byte(`{{ tpl "{{include `+"`"+`TplFunction/templates/_partial`+ "`" + ` . | indent 2}}" .}}`)},
{Name: "templates/_partial", Data: []byte(`{{.Release.Name}} - he`)},
},
Values: &chart.Config{Raw: ``},
Dependencies: []*chart.Chart{},
results in:
Expected "Evaluate tpl bar", got " template: no template \"TplFunction/templates/_partial\" associated with template \"gotpl\"" (map[TplFunction/templates/_partial:TestRelease - he TplFunction/templates/base: template: no template "TplFunction/templates/_partial" associated with template "gotpl"])
There was a problem hiding this comment.
I'll look into it. I was wondering about the scoping for that.
pkg/engine/engine.go
Outdated
| templates["template"] = r | ||
|
|
||
| result, err := e.render(templates) | ||
| if err != nil { |
There was a problem hiding this comment.
Instead of returning the error as the result string I would favour making the function return (string, error) and failing the template execution altogether if the provided string does not render.
Otherwise errors are very hard to catch and will most certainly endup being deployed to k8s causing failures further down the road.
See the recently merged change for include #2358 for some more details.
There was a problem hiding this comment.
Thanks for the input. I changed the function to return an error on failure instead of writing it into the template.
…template. Implementation for helm#1978
…e error in the template - Added tests for using a function inside a template that is evaluated using the "tpl" function
k8s-ci-robot
added
the
cncf-cla: yes
pkg/engine/engine.go
Outdated
| templates := map[string]renderable{} | ||
| templates["template"] = r | ||
|
|
||
| result, err := e.render(templates) |
There was a problem hiding this comment.
I wonder if doing a t.Parse() here instead might cause it to inherit scope. I'm not sure how best to work around the loss of context...
There was a problem hiding this comment.
When a chart is rendered I add the Chart's templates as the current templates in the engine. If something else is rendered it will add the current templates to the render context.
@technosophos What do you think about it?
|
Is this ready for another review, @eicnix ? |
|
@technosophos Yes, I am saving the current templates in the engine to reuse them in a |
|
Okay. It's on my docket for testing tomorrow for inclusion in 2.5.0. |
|
Okay, I have done a lot of testing of this, and it seems to be in working order. Do you feel okay with my documenting this as an experimental feature for now? I am still a little worried that some strange edge case will crop up in production. But I am confident enough in this PR that I think we should ship it for 2.5. |
|
This change is |
| } | ||
|
|
||
| templates := map[string]renderable{} | ||
| templates["aaa_template"] = r |
There was a problem hiding this comment.
I would not mind changing this to a randomly generated string just to discourage others from trying to access the template directly. But that's definitely not necessary for now.
|
@technosophos Ofc this can be marked experimental. I don't fully understand the whole impact the caching could have and some real world feedback would be nice. |
|
Okay! I'm merging this. Thanks again for the work on this @eicnix |
|
@eicnix Would it be possible for you to add some docs on what the |
|
Yes, that looks great, thanks! |
Added implementation for a tpl function that evaluates a string as a template.
Implementation for #1978