feat: include func support #1187
Conversation
test/integration/test-cases/include-template-func/input/helmfile.yaml
Outdated
Show resolved
Hide resolved
|
@mumoshu As a first-time implementation, do you think this is acceptable? |
|
@mumoshu I will try to implement the nested mode. |
|
@yxxhero See #1187 (comment). I think you need less code to achieve more! |
@mumoshu ok. got it. |
|
@mumoshu Another issue is that when loading the values of the environment or the values of the release, the basePath will change to the directory where the values file is located, which is also a problem. |
Yes! That's the expected behavior.
Why is that? That's also how each file locates colocated files with |
because it lost the helmfile state location information. @mumoshu |
|
@yxxhero Please see my updated reply! It is never lost. It must not depend on the "irrelevant" state. Each helmfile.yaml and values.yaml template need to be independently runnable and processable. |
|
Otherwise, you break helmfile's premise of "each helmfile.yaml is independently runnable and reusable". |
got it. |
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Co-authored-by: Yusuke Kuoka <ykuoka@gmail.com> Signed-off-by: yxxhero <aiopsclub@163.com>
yxxhero
force-pushed
the
feat--include-func-support
branch
from
26b40ab
to
be82ce8
Compare
|
@mumoshu all is ok. |
pkg/tmpl/context_tmpl.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get working directory: %v", err) | ||
| } | ||
| files, err := c.fs.Glob(filepath.Join(wd, "_*.tpl")) |
There was a problem hiding this comment.
Nit but this could be simpified:
| files, err := c.fs.Glob(filepath.Join(wd, "_*.tpl")) | |
| files, err := c.fs.Glob(filepath.Join(c.basePath, "_*.tpl")) |
There was a problem hiding this comment.
wd == c.basePath? I am not sure when to render the value file of release and the value file of env. @mumoshu
There was a problem hiding this comment.
see: pkg/state/envvals_loader.go 62
r := tmpl.NewFileRenderer(ld.fs, filepath.Dir(f), tmplData)
// f is the path of the env value file. not helm statefile file path.
There was a problem hiding this comment.
so I think wd will be better. @mumoshu
There was a problem hiding this comment.
Ah! Sorry for the confusion. The rule is that you shouldn't use wd at all.
Our readFile, for example, reads the file relative to the basePath, not wd, you know:
helmfile/pkg/tmpl/context_funcs.go
Line 222 in 0650447
The NewFileRenderer you pointed out takes basePath as the second arg, instead of wd, which aligns with the rule.
helmfile/pkg/tmpl/file_renderer.go
Line 17 in 0650447
If you used wd here, it ends up with that tpl files are loaded from the directory where helmfile.yaml is located, whereas readFile reads files from the directory where the (env) values.yaml template file is located, which is inconsistent.
| var buf strings.Builder | ||
| if v, ok := includedNames[name]; ok { | ||
| if v > recursionMaxNums { | ||
| return "", errors.Wrapf(fmt.Errorf("unable to execute template"), "rendering template has a nested reference name: %s", name) |
There was a problem hiding this comment.
Good job!
A test case for this error would make it perfect!
There was a problem hiding this comment.
@mumoshu that logic comes from helm. I just copied and changed a bit.
There was a problem hiding this comment.
It doesn't justify not having a test! We might change or refactor it in the future and test prevents breakage.
Signed-off-by: yxxhero <aiopsclub@163.com>
|
@mumoshu I added a test case for see: test/integration/test-cases/include-template-func/input/helmfile.yaml.gotmpl
|
Signed-off-by: yxxhero <aiopsclub@163.com>
* feat: include func support Signed-off-by: yxxhero <aiopsclub@163.com> Signed-off-by: Jade Hayes <jade.hayes@dominodatalab.com>
|
Eagerly awaiting this feature to be released. |
|
@plevart released. please enjoy it. thanks so much. |
#1175