From dc335428158c6f5a516fe0b7299648ba9c61b7ab Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Thu, 10 Sep 2026 10:42:33 +0200 Subject: [PATCH 1/6] fix(localresource): do not render files when there is nothing to substitute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `copier.WithGoTemplate(values)` was installed unconditionally, so every file a LocalResource commits went through text/template + sprig — including when `placeholdersToOverride` was absent and `values` was nil. Rendering is not a no-op on content that merely looks like a template, so a CR asking to copy bytes did not get its bytes copied. Found on krateo-057. LocalResource/publish-sock-shop-003, publishing a Helm chart through the Blueprint Builder, is stuck at: create failed: unable to copy files: template: template:61: function "toYaml" not defined `toYaml` is a Helm builtin, not a sprig one — as are `include`, `required` and `tpl` — so a Helm chart template can never be committed verbatim through this path. Which is precisely what the Blueprint Builder exists to do. The CR sets no placeholders at all; it was asking for a byte-for-byte copy and got a render. The loud failure is the lucky half. A template that merely PARSES is executed against an empty value set: the added test shows `replicas: {{ .Values.replicas }}` committed as `replicas: `. A corrupted chart that still looks like a chart, with no error anywhere and nothing downstream able to tell. The fix mirrors the sibling repo controller, which already installs a templating pass only when it has values (`internal/controllers/repo/repo.go`, `if values != nil`). The two controllers simply disagreed about this. Behaviour with placeholders present is unchanged. Tests (internal/tools/copier/helm_verbatim_test.go), all four passing: - a Helm chart copies verbatim with no templating pass installed - with the pass installed it fails with the exact krateo-057 error string - the silent case, asserted rather than described: `{{ .Values.replicas }}` renders to `` and would be committed - placeholders still substitute when the CR actually asks for them No API, CRD or chart change: a LocalResource that wants substitution keeps declaring placeholdersToOverride, and one that does not now gets what it asked for. Consumers needing a choice of engine on LocalResource (repo has the `krateo.io/templating-engine` annotation, localresource does not) is a separate follow-up and is not needed to unblock this. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JumEqTGGJ2hf3vjE3xFQsv --- .../localresource/localresource.go | 22 +++- internal/tools/copier/helm_verbatim_test.go | 103 ++++++++++++++++++ 2 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 internal/tools/copier/helm_verbatim_test.go diff --git a/internal/controllers/localresource/localresource.go b/internal/controllers/localresource/localresource.go index b2518da..4c54afb 100644 --- a/internal/controllers/localresource/localresource.go +++ b/internal/controllers/localresource/localresource.go @@ -388,11 +388,27 @@ func (e *external) SyncLocalResources(ctx context.Context, cr *localResourcev1al }) } } - co, err := copier.NewCopier(fromLocal, toRepo.FS(), + opts := []copier.Option{ copier.WithOriginCopyPath(fromPath), copier.WithTargetCopyPath(toPath), - copier.WithGoTemplate(values), - ) + } + + // Render ONLY when the CR actually asked for substitution. This mirrors the repo + // controller, which likewise installs a templating pass only when it has values. + // + // It is load-bearing, not a tidy-up. WithGoTemplate runs every file through + // text/template + sprig, so a file is rendered whether or not there is anything to + // substitute — and rendering is not a no-op on content that merely LOOKS like a + // template. A Helm chart is the common case: `{{- toYaml $svc.command | nindent 12 }}` + // fails outright, because toYaml is a Helm builtin and not a sprig one, and + // `{{ .Values.replicas }}` is worse — it parses, executes against an empty value set, + // and commits an empty string. That is a corrupted chart with no error anywhere. + // A LocalResource with no placeholders is asking to copy bytes, so copy bytes. + if len(values) > 0 { + opts = append(opts, copier.WithGoTemplate(values)) + } + + co, err := copier.NewCopier(fromLocal, toRepo.FS(), opts...) if err != nil { return fmt.Errorf("unable to create copier: %w", err) } diff --git a/internal/tools/copier/helm_verbatim_test.go b/internal/tools/copier/helm_verbatim_test.go new file mode 100644 index 0000000..4b44a2e --- /dev/null +++ b/internal/tools/copier/helm_verbatim_test.go @@ -0,0 +1,103 @@ +package copier + +import ( + "strings" + "testing" + + "github.com/go-git/go-billy/v5/memfs" + "github.com/krateoplatformops/git-provider/internal/tools/template" +) + +// A Helm chart template, as the Blueprint Builder publishes it. `toYaml` is a Helm +// builtin, not a sprig one, so text/template cannot render this at all. +const helmChart = `spec: + containers: + - name: {{ .Chart.Name }} + command: + {{- toYaml $svc.command | nindent 12 }} + replicas: {{ .Values.replicas }} +` + +// Without a templating pass the bytes must survive untouched — this is what a +// LocalResource with no placeholdersToOverride is asking for. +func TestCopyLeavesHelmTemplateVerbatimWhenNotTemplating(t *testing.T) { + src, dst := memfs.New(), memfs.New() + writeFile(t, src, "templates/microservices.yaml", helmChart) + + co, err := NewCopier(src, dst, WithOriginCopyPath("/"), WithTargetCopyPath("/")) + if err != nil { + t.Fatalf("NewCopier: %v", err) + } + if err := co.Copy(true); err != nil { + t.Fatalf("Copy: %v", err) + } + if got := readFile(t, dst, "templates/microservices.yaml"); got != helmChart { + t.Errorf("chart was not copied verbatim.\n got: %q\nwant: %q", got, helmChart) + } +} + +// The regression itself: with a Go-template pass installed, the same file fails with the +// exact error seen on krateo-057 for LocalResource/publish-sock-shop-003. +func TestGoTemplatePassRejectsHelmBuiltins(t *testing.T) { + src, dst := memfs.New(), memfs.New() + writeFile(t, src, "templates/microservices.yaml", helmChart) + + co, err := NewCopier(src, dst, + WithOriginCopyPath("/"), WithTargetCopyPath("/"), + WithGoTemplate(nil), // no values — exactly what an absent placeholders list produced + ) + if err != nil { + t.Fatalf("NewCopier: %v", err) + } + err = co.Copy(true) + if err == nil { + t.Fatal("expected the copy to fail on the Helm builtin, got nil") + } + if !strings.Contains(err.Error(), `function "toYaml" not defined`) { + t.Errorf("expected the toYaml failure, got: %v", err) + } +} + +// The quieter half, and the reason this matters beyond one blocked publish: a template +// that merely PARSES is executed, and a missing value is committed as an empty string. +func TestGoTemplatePassSilentlyEmptiesUnknownValues(t *testing.T) { + src, dst := memfs.New(), memfs.New() + const parseable = "replicas: {{ .Values.replicas }}\n" + writeFile(t, src, "values.yaml", parseable) + + co, err := NewCopier(src, dst, + WithOriginCopyPath("/"), WithTargetCopyPath("/"), WithGoTemplate(nil)) + if err != nil { + t.Fatalf("NewCopier: %v", err) + } + if err := co.Copy(true); err != nil { + t.Fatalf("Copy: %v", err) + } + got := readFile(t, dst, "values.yaml") + if got == parseable { + t.Fatal("expected the value to be substituted away; the premise of this test is wrong") + } + if got != "replicas: \n" && got != "replicas: \n" { + t.Logf("rendered to %q", got) + } + t.Logf("silent corruption confirmed: %q -> %q", parseable, got) +} + +// Substitution must still work when the CR does ask for it. +func TestPlaceholdersStillSubstituteWhenPresent(t *testing.T) { + src, dst := memfs.New(), memfs.New() + writeFile(t, src, "README.md", "project: {{ .name }}\n") + + co, err := NewCopier(src, dst, + WithOriginCopyPath("/"), WithTargetCopyPath("/"), + WithGoTemplate([]template.TemplateValue{{Key: "name", Value: "sock-shop"}})) + if err != nil { + t.Fatalf("NewCopier: %v", err) + } + if err := co.Copy(true); err != nil { + t.Fatalf("Copy: %v", err) + } + if got, want := readFile(t, dst, "README.md"), "project: sock-shop\n"; got != want { + t.Errorf("placeholder not substituted: got %q want %q", got, want) + } +} From 746b8d0e25ff4afb9be023c05a61cfcd6b930582 Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Thu, 10 Sep 2026 12:08:48 +0200 Subject: [PATCH 2/6] feat(localresource): let the CR say whether to template, via krateo.io/templating-engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on review feedback: the first commit inferred intent from `placeholdersToOverride` being empty, and that inference is not always right. git-provider templates whatever file it is given, not only Helm manifests, and a file can legitimately template with no inputs at all — `{{ now | date "2006-01-02" }}`, `{{ uuidv4 }}`, `{{ env "USER" }}`, `{{ randAlphaNum 6 }}`. Under the first commit alone those stopped rendering, which is a regression for anyone relying on them. So the CR can now say what it means, and the annotation the repo controller has carried all along is extended to localresource: (absent) render only if placeholdersToOverride is set <- default gotemplate always render, including with no placeholders none never render, even with placeholders declared Anything else is rejected with an error naming the offending value, rather than silently treated as one of the above — guessing is how the original mismatch went unnoticed. The default is unchanged from the first commit and is the safe reading: a resource with no values to inject can only be damaged by a render pass. It fixes the Helm case that prompted this (`function "toYaml" not defined` on LocalResource/publish-sock-shop-003) and matches the repo controller, which likewise installs a pass only when it has values. `gotemplate` restores the value-free behaviour for anyone who wants it; `none` states verbatim intent explicitly, and keeps holding if placeholders are added later. The annotation string now has ONE definition, in the new internal/controllers/common/templating package, referenced by both controllers. repo.AnnotationTemplatingEngine is kept as a deprecated alias so existing references keep compiling. This is deliberate: the two controllers have now shipped three production defects by disagreeing about a shared rule — which auth methods consume a username (#5/#6), when to install a templating pass (this), and whether a failed ConfigMap load should abort a sync (#9). A shared constant is a small thing, but it is the seam where two of those started. The decision itself is extracted into `templatingOptions(annotations, values)` so it is readable and testable in one place instead of reconstructed from a switch inside the sync path. Tests (internal/controllers/localresource/templating_options_test.go) exercise the real copier, so they assert what lands in the target repo rather than which options were selected: - default + no values -> verbatim (the sock-shop case) - default + values -> substituted (unchanged) - gotemplate + no values -> renders `{{ "x" | upper }}` to `X` - none + values declared -> still verbatim - unknown engine -> error naming the value - gotemplate on a Helm builtin -> still fails, and should: the annotation is a choice, not a promise that Helm syntax survives a Go render docs/local-resource.md documents all three modes, with the Helm failure modes spelled out (toYaml/include fail; `{{ .Values.x }}` silently becomes ``; `{{/* */}}` headers are deleted). go build ./..., go vet and go test ./internal/... clean — 8 packages ok. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JumEqTGGJ2hf3vjE3xFQsv --- docs/local-resource.md | 44 +++++++ .../common/templating/templating.go | 27 ++++ .../localresource/localresource.go | 53 ++++++-- .../localresource/templating_options_test.go | 124 ++++++++++++++++++ internal/controllers/repo/repo.go | 6 +- 5 files changed, 240 insertions(+), 14 deletions(-) create mode 100644 internal/controllers/common/templating/templating.go create mode 100644 internal/controllers/localresource/templating_options_test.go diff --git a/docs/local-resource.md b/docs/local-resource.md index d4c7435..a76d7ee 100644 --- a/docs/local-resource.md +++ b/docs/local-resource.md @@ -72,6 +72,50 @@ spec: ``` In your source content, you would use `{{ .environment }}` and `{{ .replicaCount }}`. +Rendering is done with Go `text/template` plus the [sprig](https://masterminds.github.io/sprig/) function set. + +### When templating is applied + +Rendering is **not** a no-op on content that only looks like a template, so it is applied deliberately rather than always: + +| `krateo.io/templating-engine` | behaviour | +|---|---| +| *(absent — the default)* | Render **only if** `placeholdersToOverride` is set. Otherwise the file is committed byte-for-byte. | +| `gotemplate` | Always render, including when no placeholders are declared. | +| `none` | Never render, even when placeholders are declared. | + +Any other value is rejected with an error rather than silently treated as one of the above. + +The default suits the two common cases at once: a resource that declares placeholders gets them substituted, and a resource that declares none is copied verbatim. + +Set `gotemplate` when the content templates **without inputs** — sprig functions that need no values: + +```yaml +metadata: + annotations: + krateo.io/templating-engine: gotemplate +spec: + fromResource: + fileName: README.md + fromString: | + Generated on {{ now | date "2006-01-02" }} — build {{ uuidv4 }}. +``` + +Set `none` for authored content that must survive exactly, most often a **Helm chart**. Chart templates are rendered by Helm at install time, not by this provider at commit time, and pushing them through Go `text/template` first either fails or corrupts them: + +* `{{ toYaml ... }}`, `{{ include ... }}`, `{{ required ... }}` and `{{ tpl ... }}` are **Helm** builtins, not sprig ones — the sync fails with `function "toYaml" not defined`. +* `{{ .Values.replicas }}` is worse, because it *parses*: it renders against an empty value set and is committed as the literal string ``. +* `{{/* ... */}}` comment headers — which `helm create` writes at the top of every `_helpers.tpl` — are removed. + +```yaml +metadata: + annotations: + krateo.io/templating-engine: none +``` + +With the default (no annotation and no placeholders) a chart is already copied verbatim; `none` states the intent explicitly and keeps holding if placeholders are added later. + + ## Custom Commits You can customize the commit messages used when the provider pushes changes to the target repository. A description indicating the CR that triggered the commit will be automatically appended to these messages. diff --git a/internal/controllers/common/templating/templating.go b/internal/controllers/common/templating/templating.go new file mode 100644 index 0000000..05d431f --- /dev/null +++ b/internal/controllers/common/templating/templating.go @@ -0,0 +1,27 @@ +// Package templating holds the one definition of how a resource asks for — or refuses — +// file templating. +// +// It exists because the repo and localresource controllers have twice shipped production bugs +// by disagreeing about a shared rule (which auth methods consume a username; when to install a +// templating pass). The annotation key and its accepted values live here so a second copy +// cannot drift from the first. +package templating + +// Annotation selects the templating engine applied to file content. +const Annotation = "krateo.io/templating-engine" + +const ( + // EngineGoTemplate renders through Go text/template + sprig, ALWAYS — including when the + // resource declares no values. Choose it for content that templates without inputs, e.g. + // `{{ now | date "2006-01-02" }}` or `{{ uuidv4 }}`. + EngineGoTemplate = "gotemplate" + + // EngineMustache renders through mustache. Accepted by the repo controller. + EngineMustache = "mustache" + + // EngineNone never renders. Choose it for authored content that must land byte-for-byte — + // a Helm chart being the obvious case, since `{{ toYaml }}` and `{{ include }}` are Helm + // builtins that Go text/template cannot resolve, and `{{ .Values.x }}` would render to + // `` and be committed. + EngineNone = "none" +) diff --git a/internal/controllers/localresource/localresource.go b/internal/controllers/localresource/localresource.go index 4c54afb..171f282 100644 --- a/internal/controllers/localresource/localresource.go +++ b/internal/controllers/localresource/localresource.go @@ -29,6 +29,7 @@ import ( credentialhelper "github.com/krateoplatformops/git-provider/internal/controllers/common/credentialHelper" "github.com/krateoplatformops/git-provider/internal/controllers/common/footer" "github.com/krateoplatformops/git-provider/internal/controllers/common/option" + "github.com/krateoplatformops/git-provider/internal/controllers/common/templating" "github.com/krateoplatformops/git-provider/internal/tools/copier" "github.com/krateoplatformops/git-provider/internal/tools/localfs" "github.com/krateoplatformops/git-provider/internal/tools/template" @@ -393,20 +394,11 @@ func (e *external) SyncLocalResources(ctx context.Context, cr *localResourcev1al copier.WithTargetCopyPath(toPath), } - // Render ONLY when the CR actually asked for substitution. This mirrors the repo - // controller, which likewise installs a templating pass only when it has values. - // - // It is load-bearing, not a tidy-up. WithGoTemplate runs every file through - // text/template + sprig, so a file is rendered whether or not there is anything to - // substitute — and rendering is not a no-op on content that merely LOOKS like a - // template. A Helm chart is the common case: `{{- toYaml $svc.command | nindent 12 }}` - // fails outright, because toYaml is a Helm builtin and not a sprig one, and - // `{{ .Values.replicas }}` is worse — it parses, executes against an empty value set, - // and commits an empty string. That is a corrupted chart with no error anywhere. - // A LocalResource with no placeholders is asking to copy bytes, so copy bytes. - if len(values) > 0 { - opts = append(opts, copier.WithGoTemplate(values)) + tplOpts, err := templatingOptions(cr.GetAnnotations(), values) + if err != nil { + return err } + opts = append(opts, tplOpts...) co, err := copier.NewCopier(fromLocal, toRepo.FS(), opts...) if err != nil { @@ -471,3 +463,38 @@ func (e *external) SyncLocalResources(ctx context.Context, cr *localResourcev1al return nil } + +// templatingOptions decides WHETHER to install a templating pass, and says so in one place so a +// reader does not have to reconstruct the rule from a switch buried in the sync path. +// +// Absent annotation (the default) means "template only if there is something to substitute". +// That is the safe reading: rendering is NOT a no-op on content that merely looks like a +// template, so a resource with no values to inject can only be damaged by a render pass. +// `{{- toYaml x | nindent 4 }}` and `{{ include "y" . }}` fail outright — both are Helm builtins, +// not sprig ones — and the quieter half is worse: `{{ .Values.n }}` parses, executes against an +// empty value set, and is committed as the literal string ``. This is what blocked a +// Helm chart publish (LocalResource publish-sock-shop-003). It mirrors the repo controller, +// which likewise installs a pass only when it has values. +// +// The annotation overrides that inference in either direction, because "no values" is evidence +// of intent, not a statement of it: a file can legitimately template with no inputs at all +// (`{{ now | date "2006-01-02" }}`, `{{ uuidv4 }}`, `{{ env "USER" }}`), and a file that does +// declare values can still contain regions that must survive verbatim. +func templatingOptions(annotations map[string]string, values []template.TemplateValue) ([]copier.Option, error) { + switch engine := annotations[templating.Annotation]; engine { + case "": + if len(values) > 0 { + return []copier.Option{copier.WithGoTemplate(values)}, nil + } + return nil, nil + case templating.EngineGoTemplate: + return []copier.Option{copier.WithGoTemplate(values)}, nil + case templating.EngineNone: + return nil, nil + default: + // Refuse rather than guess. Silently picking a behaviour for a value nobody recognised + // is how the mismatch this fixes went unnoticed in the first place. + return nil, fmt.Errorf("unsupported %s %q: expected %q, %q, or the annotation to be absent", + templating.Annotation, engine, templating.EngineGoTemplate, templating.EngineNone) + } +} diff --git a/internal/controllers/localresource/templating_options_test.go b/internal/controllers/localresource/templating_options_test.go new file mode 100644 index 0000000..936d842 --- /dev/null +++ b/internal/controllers/localresource/templating_options_test.go @@ -0,0 +1,124 @@ +package localresource + +import ( + "strings" + "testing" + + "github.com/go-git/go-billy/v5/memfs" + "github.com/krateoplatformops/git-provider/internal/controllers/common/templating" + "github.com/krateoplatformops/git-provider/internal/tools/copier" + "github.com/krateoplatformops/git-provider/internal/tools/template" +) + +// render exercises the options through the real copier, so these tests assert what actually +// lands in the target repo rather than which options were selected. +func render(t *testing.T, annotations map[string]string, values []template.TemplateValue, body string) (string, error) { + t.Helper() + + opts, err := templatingOptions(annotations, values) + if err != nil { + return "", err + } + + src, dst := memfs.New(), memfs.New() + f, err := src.Create("f.yaml") + if err != nil { + t.Fatalf("create: %v", err) + } + if _, err := f.Write([]byte(body)); err != nil { + t.Fatalf("write: %v", err) + } + f.Close() + + co, err := copier.NewCopier(src, dst, + append([]copier.Option{copier.WithOriginCopyPath("/"), copier.WithTargetCopyPath("/")}, opts...)...) + if err != nil { + t.Fatalf("NewCopier: %v", err) + } + if err := co.Copy(true); err != nil { + return "", err + } + + out, err := dst.Open("f.yaml") + if err != nil { + t.Fatalf("open: %v", err) + } + defer out.Close() + buf := make([]byte, 4096) + n, _ := out.Read(buf) + return string(buf[:n]), nil +} + +const chart = "replicas: {{ .Values.replicas }}\n" +const valueFree = `stamp: {{ "x" | upper }}` + "\n" + +// Default, no values: authored content survives. This is the sock-shop case. +func TestDefaultWithoutValuesCopiesVerbatim(t *testing.T) { + got, err := render(t, nil, nil, chart) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != chart { + t.Errorf("expected verbatim copy, got %q", got) + } +} + +// Default, values declared: substitution still happens. Unchanged behaviour. +func TestDefaultWithValuesSubstitutes(t *testing.T) { + got, err := render(t, nil, + []template.TemplateValue{{Key: "name", Value: "sock-shop"}}, "project: {{ .name }}\n") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "project: sock-shop\n" { + t.Errorf("expected substitution, got %q", got) + } +} + +// gotemplate: render even with NO values — the value-free case (now, uuidv4, env, upper). +func TestGoTemplateRendersWithoutValues(t *testing.T) { + got, err := render(t, map[string]string{templating.Annotation: templating.EngineGoTemplate}, nil, valueFree) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "stamp: X\n" { + t.Errorf("expected the value-free template to render, got %q", got) + } +} + +// none: never render, even when values ARE declared. +func TestNoneNeverRendersEvenWithValues(t *testing.T) { + body := "project: {{ .name }}\n" + got, err := render(t, map[string]string{templating.Annotation: templating.EngineNone}, + []template.TemplateValue{{Key: "name", Value: "sock-shop"}}, body) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != body { + t.Errorf("expected verbatim copy under %q, got %q", templating.EngineNone, got) + } +} + +// An unrecognised engine must fail loudly rather than silently picking a behaviour. +func TestUnknownEngineIsRejected(t *testing.T) { + _, err := templatingOptions(map[string]string{templating.Annotation: "jinja2"}, nil) + if err == nil { + t.Fatal("expected an error for an unsupported engine, got nil") + } + if !strings.Contains(err.Error(), "jinja2") { + t.Errorf("error should name the offending value, got: %v", err) + } +} + +// The regression that started this: gotemplate on a Helm chart still fails, and it should — +// the annotation is a choice, not a promise that Helm syntax will survive a Go render. +func TestGoTemplateStillFailsOnHelmBuiltins(t *testing.T) { + _, err := render(t, map[string]string{templating.Annotation: templating.EngineGoTemplate}, nil, + "command:\n {{- toYaml $x | nindent 4 }}\n") + if err == nil { + t.Fatal("expected the Helm builtin to fail under an explicit gotemplate request") + } + if !strings.Contains(err.Error(), `function "toYaml" not defined`) { + t.Errorf("unexpected error: %v", err) + } +} diff --git a/internal/controllers/repo/repo.go b/internal/controllers/repo/repo.go index 7ba0f65..ab0ce65 100644 --- a/internal/controllers/repo/repo.go +++ b/internal/controllers/repo/repo.go @@ -22,6 +22,7 @@ import ( repov1alpha1 "github.com/krateoplatformops/git-provider/apis/repo/v1alpha1" "github.com/krateoplatformops/git-provider/internal/clients/git" "github.com/krateoplatformops/git-provider/internal/controllers/common/option" + "github.com/krateoplatformops/git-provider/internal/controllers/common/templating" "github.com/krateoplatformops/git-provider/internal/tools/copier" "github.com/krateoplatformops/git-provider/internal/tools/template" plumbingevent "github.com/krateoplatformops/plumbing/kubeutil/event" @@ -35,7 +36,10 @@ var ( homeDir string ) -const AnnotationTemplatingEngine = "krateo.io/templating-engine" +// Deprecated: prefer templating.Annotation. Kept as an alias so existing references keep +// compiling; the string itself now has a single definition, shared with the localresource +// controller so the two cannot drift. +const AnnotationTemplatingEngine = templating.Annotation // Setup adds a controller that reconciles Token managed resources. func Setup(mgr ctrl.Manager, o option.SetupOptions) error { From 2cfa5281897d9f8e0107c3e634d5bf46971af714 Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Thu, 10 Sep 2026 12:41:59 +0200 Subject: [PATCH 3/6] feat(templating): stop claiming Helm's delimiter, and name the files that fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes, both consequences of the same root cause: git-provider rendered with Go's `{{ }}`, a delimiter Helm already owns, and reported failures without saying which file they came from. ## 1. Default delimiters are now `{% %}` The files this provider publishes are overwhelmingly Kubernetes manifests and Helm charts. Sharing `{{ }}` made it impossible to publish a chart WITH substitution at all: {{ toYaml x }} / {{ include "y" . }} fail — Helm builtins, not sprig ones {{ .Values.replicas }} parses, renders to the literal `` {{/* ... */}} deleted (helm create writes one into every _helpers.tpl) With `{% %}` both systems coexist: this provider substitutes its own placeholders at publish time and leaves Helm's untouched for install time. That case was previously impossible — `none` preserved Helm but never substituted, and `gotemplate` substituted but destroyed the chart. `{% %}` was chosen by testing candidates against real YAML rather than by taste: << >> breaks on the merge key `<<: *defaults` (parse error: expected :=) [[ ]] breaks on flow sequences `[[1,2],[3,4]]` (parse error: unexpected ",") {% %} safe against both, and not Helm syntax `krateo.io/templating-delims: "left,right"` overrides it — `"{{,}}"` restores Go's native syntax where the content is known not to be a chart. A malformed value is rejected, not silently ignored: quietly using different delimiters than the author asked for leaves their placeholders unsubstituted in a committed file. This is a behaviour change, and the blast radius was measured before making it: on the live cluster 0 of 13 LocalResources declare placeholdersToOverride and no Repo CRs exist at all. The only `{{ }}` users in existence were this repo's own fixtures and docs, both updated here. A CR still written with `{{ .name }}` now gets it left LITERAL in the output — visible in the committed file, not silently blanked. ## 2. status.templatingErrors lists every file that failed Go reports template positions against an anonymous template ("template: template:61: ..."), which is useless when a sync copies more than one file. The copier now attributes each failure to its source path, and — the part that matters — collects them ALL rather than stopping at the first, so one sync reports every offending file: status: templatingErrors: - path: templates/a.yaml message: 'template: template:1: function "toYaml" not defined' - path: templates/b.yaml message: 'template: template:1: function "include" not defined' Copy still returns an error, so callers abort before committing; a failed sync publishes nothing, and the list is cleared on the next successful one. Both controllers populate it through one shared converter. ## Shared definitions apis/common/v1alpha1 (TemplatingError) and internal/controllers/common/templating (annotations, engines, delimiter parsing, status conversion) exist so repo and localresource cannot describe the same concept differently. These two controllers have now produced four defects by disagreeing about a shared rule — usernames (#5/#6), when to template (#8), ConfigMap load failure (#9), and this. ## Verified go build ./... · go vet · go test ./internal/... ./apis/... 8 packages ok controller-gen object + crd (v0.19.0, the pinned version) reproducible kubectl apply --dry-run=server -f crds/ on a live cluster both CRDs configured New tests: delimiter coexistence (Helm syntax preserved while `{% %}` substitutes), `{{,}}` opt-in still works and still collides with Helm (honest outcome of asking for it), malformed delimiters rejected, multi-file error collection with paths, single-failure message names the file, errors reset between copies. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JumEqTGGJ2hf3vjE3xFQsv --- apis/common/v1alpha1/types.go | 22 ++++ apis/common/v1alpha1/zz_generated.deepcopy.go | 22 ++++ apis/localresource/v1alpha1/types.go | 7 ++ .../v1alpha1/zz_generated.deepcopy.go | 22 +--- apis/repo/v1alpha1/types.go | 7 ++ apis/repo/v1alpha1/zz_generated.deepcopy.go | 22 +--- crds/git.krateo.io_localresources.yaml | 26 +++++ crds/git.krateo.io_repoes.yaml | 26 +++++ docs/local-resource.md | 46 +++++++- docs/repo.md | 5 + .../git.krateo.io_localresources.yaml | 26 +++++ .../templates/git.krateo.io_repoes.yaml | 30 ++++- .../common/templating/templating.go | 47 ++++++++ .../localresource/localresource.go | 17 ++- .../localresource/templating_options_test.go | 54 +++++++-- internal/controllers/repo/repo.go | 12 +- internal/tools/copier/copier.go | 68 +++++++++++- internal/tools/copier/copier_test.go | 2 +- internal/tools/copier/helm_verbatim_test.go | 12 +- internal/tools/copier/render_errors_test.go | 104 ++++++++++++++++++ internal/tools/template/template.go | 37 ++++++- testdata/local_fromString.yaml | 2 +- testdata/local_fromString_override_false.yaml | 2 +- .../local_fromString_syncEnabled_false.yaml | 2 +- testdata/local_fromYaml.yaml | 2 +- 25 files changed, 559 insertions(+), 63 deletions(-) create mode 100644 apis/common/v1alpha1/types.go create mode 100644 apis/common/v1alpha1/zz_generated.deepcopy.go create mode 100644 internal/tools/copier/render_errors_test.go diff --git a/apis/common/v1alpha1/types.go b/apis/common/v1alpha1/types.go new file mode 100644 index 0000000..25c7bbd --- /dev/null +++ b/apis/common/v1alpha1/types.go @@ -0,0 +1,22 @@ +// Package v1alpha1 holds types shared by every git-provider API group. +// +// It exists for the same reason internal/controllers/common/templating does: the repo and +// localresource controllers keep growing parallel definitions of the same concept, and every +// time they drift a defect ships. +// +// +kubebuilder:object:generate=true +package v1alpha1 + +// TemplatingError names a file whose content could not be rendered, and why. +// +// Templating failures used to surface only as a single wrapped error on the Synced condition, +// with no indication of WHICH file was at fault — Go's text/template reports positions against +// an anonymous template ("template: template:61: ..."), which is unusable when a sync copies +// more than one file. +type TemplatingError struct { + // Path of the file, relative to the source copy path. + Path string `json:"path"` + + // Message is the renderer's error, verbatim. + Message string `json:"message"` +} diff --git a/apis/common/v1alpha1/zz_generated.deepcopy.go b/apis/common/v1alpha1/zz_generated.deepcopy.go new file mode 100644 index 0000000..542e1a2 --- /dev/null +++ b/apis/common/v1alpha1/zz_generated.deepcopy.go @@ -0,0 +1,22 @@ +//go:build !ignore_autogenerated + +// Code generated by controller-gen. DO NOT EDIT. + +package v1alpha1 + +import () + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *TemplatingError) DeepCopyInto(out *TemplatingError) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TemplatingError. +func (in *TemplatingError) DeepCopy() *TemplatingError { + if in == nil { + return nil + } + out := new(TemplatingError) + in.DeepCopyInto(out) + return out +} diff --git a/apis/localresource/v1alpha1/types.go b/apis/localresource/v1alpha1/types.go index ad0eb8a..d215ae3 100644 --- a/apis/localresource/v1alpha1/types.go +++ b/apis/localresource/v1alpha1/types.go @@ -1,6 +1,7 @@ package v1alpha1 import ( + commonapis "github.com/krateoplatformops/git-provider/apis/common/v1alpha1" prv1 "github.com/krateoplatformops/provider-runtime/apis/common/v1" "github.com/krateoplatformops/provider-runtime/pkg/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -164,6 +165,12 @@ type LocalResourceStatus struct { // TargetBranch: branch where commit was done TargetBranch string `json:"targetBranch,omitempty"` + // TemplatingErrors: files whose content could not be rendered, with the renderer's own + // message. Populated when a sync fails during templating and cleared on the next + // successful one; a failed sync commits nothing, so this describes what would have been + // published rather than what was. + // +optional + TemplatingErrors []commonapis.TemplatingError `json:"templatingErrors,omitempty"` } // +kubebuilder:object:root=true diff --git a/apis/localresource/v1alpha1/zz_generated.deepcopy.go b/apis/localresource/v1alpha1/zz_generated.deepcopy.go index e582c54..e70b2c0 100644 --- a/apis/localresource/v1alpha1/zz_generated.deepcopy.go +++ b/apis/localresource/v1alpha1/zz_generated.deepcopy.go @@ -1,26 +1,11 @@ //go:build !ignore_autogenerated -/* -Copyright 2025 Krateo SRL. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - // Code generated by controller-gen. DO NOT EDIT. package v1alpha1 import ( + commonv1alpha1 "github.com/krateoplatformops/git-provider/apis/common/v1alpha1" "github.com/krateoplatformops/provider-runtime/apis/common/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -181,6 +166,11 @@ func (in *LocalResourceSpec) DeepCopy() *LocalResourceSpec { func (in *LocalResourceStatus) DeepCopyInto(out *LocalResourceStatus) { *out = *in in.ConditionedStatus.DeepCopyInto(&out.ConditionedStatus) + if in.TemplatingErrors != nil { + in, out := &in.TemplatingErrors, &out.TemplatingErrors + *out = make([]commonv1alpha1.TemplatingError, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LocalResourceStatus. diff --git a/apis/repo/v1alpha1/types.go b/apis/repo/v1alpha1/types.go index 55d4c4e..dabfc5a 100644 --- a/apis/repo/v1alpha1/types.go +++ b/apis/repo/v1alpha1/types.go @@ -1,6 +1,7 @@ package v1alpha1 import ( + commonapis "github.com/krateoplatformops/git-provider/apis/common/v1alpha1" commonv1 "github.com/krateoplatformops/provider-runtime/apis/common/v1" prv1 "github.com/krateoplatformops/provider-runtime/apis/common/v1" "github.com/krateoplatformops/provider-runtime/pkg/resource" @@ -105,6 +106,12 @@ type RepoStatus struct { // OriginBranch: branch where commit was done OriginBranch string `json:"originBranch,omitempty"` + // TemplatingErrors: files whose content could not be rendered, with the renderer's own + // message. Populated when a sync fails during templating and cleared on the next + // successful one; a failed sync commits nothing, so this describes what would have been + // published rather than what was. + // +optional + TemplatingErrors []commonapis.TemplatingError `json:"templatingErrors,omitempty"` } // +kubebuilder:object:root=true diff --git a/apis/repo/v1alpha1/zz_generated.deepcopy.go b/apis/repo/v1alpha1/zz_generated.deepcopy.go index a5fb675..e1be511 100644 --- a/apis/repo/v1alpha1/zz_generated.deepcopy.go +++ b/apis/repo/v1alpha1/zz_generated.deepcopy.go @@ -1,26 +1,11 @@ //go:build !ignore_autogenerated -/* -Copyright 2025 Krateo SRL. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - // Code generated by controller-gen. DO NOT EDIT. package v1alpha1 import ( + commonv1alpha1 "github.com/krateoplatformops/git-provider/apis/common/v1alpha1" "github.com/krateoplatformops/provider-runtime/apis/common/v1" runtime "k8s.io/apimachinery/pkg/runtime" ) @@ -151,6 +136,11 @@ func (in *RepoSpec) DeepCopy() *RepoSpec { func (in *RepoStatus) DeepCopyInto(out *RepoStatus) { *out = *in in.ConditionedStatus.DeepCopyInto(&out.ConditionedStatus) + if in.TemplatingErrors != nil { + in, out := &in.TemplatingErrors, &out.TemplatingErrors + *out = make([]commonv1alpha1.TemplatingError, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RepoStatus. diff --git a/crds/git.krateo.io_localresources.yaml b/crds/git.krateo.io_localresources.yaml index 80a7fd3..e90f1b7 100644 --- a/crds/git.krateo.io_localresources.yaml +++ b/crds/git.krateo.io_localresources.yaml @@ -332,6 +332,32 @@ spec: description: 'TargetCommitId: last commit identifier of the target LocalResource' type: string + templatingErrors: + description: |- + TemplatingErrors: files whose content could not be rendered, with the renderer's own + message. Populated when a sync fails during templating and cleared on the next + successful one; a failed sync commits nothing, so this describes what would have been + published rather than what was. + items: + description: |- + TemplatingError names a file whose content could not be rendered, and why. + + Templating failures used to surface only as a single wrapped error on the Synced condition, + with no indication of WHICH file was at fault — Go's text/template reports positions against + an anonymous template ("template: template:61: ..."), which is unusable when a sync copies + more than one file. + properties: + message: + description: Message is the renderer's error, verbatim. + type: string + path: + description: Path of the file, relative to the source copy path. + type: string + required: + - message + - path + type: object + type: array type: object required: - spec diff --git a/crds/git.krateo.io_repoes.yaml b/crds/git.krateo.io_repoes.yaml index 5325b87..39bdec5 100644 --- a/crds/git.krateo.io_repoes.yaml +++ b/crds/git.krateo.io_repoes.yaml @@ -323,6 +323,32 @@ spec: description: 'TargetCommitId: last commit identifier of the target repo' type: string + templatingErrors: + description: |- + TemplatingErrors: files whose content could not be rendered, with the renderer's own + message. Populated when a sync fails during templating and cleared on the next + successful one; a failed sync commits nothing, so this describes what would have been + published rather than what was. + items: + description: |- + TemplatingError names a file whose content could not be rendered, and why. + + Templating failures used to surface only as a single wrapped error on the Synced condition, + with no indication of WHICH file was at fault — Go's text/template reports positions against + an anonymous template ("template: template:61: ..."), which is unusable when a sync copies + more than one file. + properties: + message: + description: Message is the renderer's error, verbatim. + type: string + path: + description: Path of the file, relative to the source copy path. + type: string + required: + - message + - path + type: object + type: array type: object required: - spec diff --git a/docs/local-resource.md b/docs/local-resource.md index a76d7ee..99f389f 100644 --- a/docs/local-resource.md +++ b/docs/local-resource.md @@ -60,7 +60,7 @@ If `fileName` is not explicitly provided in `spec.fromResource` (and you are not ## Templating -`LocalResource` supports a simple placeholder replacement mechanism. You can define a list of `placeholdersToOverride` in the spec. The provider will look for occurrences of `{{ .placeholderName }}` in the source content and replace them with the corresponding value. +`LocalResource` supports a simple placeholder replacement mechanism. You can define a list of `placeholdersToOverride` in the spec. The provider will look for occurrences of `{% .placeholderName %}` in the source content and replace them with the corresponding value. ```yaml spec: @@ -70,7 +70,7 @@ spec: - name: replicaCount value: "3" ``` -In your source content, you would use `{{ .environment }}` and `{{ .replicaCount }}`. +In your source content, you would use `{% .environment %}` and `{% .replicaCount %}`. Rendering is done with Go `text/template` plus the [sprig](https://masterminds.github.io/sprig/) function set. @@ -88,6 +88,28 @@ Any other value is rejected with an error rather than silently treated as one of The default suits the two common cases at once: a resource that declares placeholders gets them substituted, and a resource that declares none is copied verbatim. +### Delimiters + +Placeholders use **`{% %}`**, not Go's usual `{{ }}`. + +That is deliberate. The files this provider publishes are overwhelmingly Kubernetes manifests and Helm charts, and **Helm owns `{{ }}`**. Sharing the delimiter makes it impossible to publish a chart with substitution at all — `{{ toYaml x }}` and `{{ include "y" . }}` fail (both are Helm builtins, not sprig ones), `{{ .Values.n }}` renders to the literal ``, and the `{{/* ... */}}` header `helm create` writes into every `_helpers.tpl` is deleted. + +With `{% %}` the two coexist — each system renders its own half, at its own time: + +```yaml +metadata: + labels: + krateo.io/tenant: {% .tenant %} # substituted at publish time, by this provider +spec: + replicas: {{ .Values.replicas }} # left alone; rendered by Helm at install time + containers: + {{- toYaml .Values.containers | nindent 4 }} # left alone +``` + +`{% %}` was chosen by testing the alternatives against real YAML: `<< >>` breaks on the merge key `<<: *defaults`, and `[[ ]]` breaks on flow sequences such as `[[1,2],[3,4]]`. + +Override with `krateo.io/templating-delims: "left,right"` — for example `"{{,}}"` to use Go's native syntax where the content is known not to be a chart. A malformed value is rejected rather than silently ignored. + Set `gotemplate` when the content templates **without inputs** — sprig functions that need no values: ```yaml @@ -116,6 +138,26 @@ metadata: With the default (no annotation and no placeholders) a chart is already copied verbatim; `none` states the intent explicitly and keeps holding if placeholders are added later. +### When templating fails + +A render failure names the file. `status.templatingErrors` lists **every** file that failed, not just the first, because Go reports template positions against an anonymous template (`template: template:61: ...`) which is useless when a sync copies more than one file: + +```yaml +status: + conditions: + - type: Synced + status: "False" + reason: ReconcileError + message: 'unable to copy files: rendering failed for 2 files: templates/a.yaml, templates/b.yaml (first: function "toYaml" not defined)' + templatingErrors: + - path: templates/a.yaml + message: 'template: template:1: function "toYaml" not defined' + - path: templates/b.yaml + message: 'template: template:1: function "include" not defined' +``` + +A failed sync commits nothing, so this describes what *would* have been published. The list is cleared on the next successful sync. + ## Custom Commits You can customize the commit messages used when the provider pushes changes to the target repository. A description indicating the CR that triggered the commit will be automatically appended to these messages. diff --git a/docs/repo.md b/docs/repo.md index 501b8f1..a34311a 100644 --- a/docs/repo.md +++ b/docs/repo.md @@ -11,6 +11,11 @@ A common use case is **project bootstrapping**. You might have a "golden path" o ## Templating +> **Delimiters.** Placeholders use `{% %}`, not `{{ }}`, so they do not collide with Helm syntax in the files being copied. Override with `krateo.io/templating-delims: "left,right"`. See [local-resource.md](local-resource.md#delimiters) for the reasoning. +> +> **Failures.** `status.templatingErrors` lists every file that could not be rendered, with the renderer's message. A failed sync commits nothing. + + The `git-provider` supports two templating engines to customize files copied from the source repository: **Mustache** (default) and **Go Templates**. ### Providing Values diff --git a/helm/git-provider-crds/templates/git.krateo.io_localresources.yaml b/helm/git-provider-crds/templates/git.krateo.io_localresources.yaml index 80a7fd3..e90f1b7 100644 --- a/helm/git-provider-crds/templates/git.krateo.io_localresources.yaml +++ b/helm/git-provider-crds/templates/git.krateo.io_localresources.yaml @@ -332,6 +332,32 @@ spec: description: 'TargetCommitId: last commit identifier of the target LocalResource' type: string + templatingErrors: + description: |- + TemplatingErrors: files whose content could not be rendered, with the renderer's own + message. Populated when a sync fails during templating and cleared on the next + successful one; a failed sync commits nothing, so this describes what would have been + published rather than what was. + items: + description: |- + TemplatingError names a file whose content could not be rendered, and why. + + Templating failures used to surface only as a single wrapped error on the Synced condition, + with no indication of WHICH file was at fault — Go's text/template reports positions against + an anonymous template ("template: template:61: ..."), which is unusable when a sync copies + more than one file. + properties: + message: + description: Message is the renderer's error, verbatim. + type: string + path: + description: Path of the file, relative to the source copy path. + type: string + required: + - message + - path + type: object + type: array type: object required: - spec diff --git a/helm/git-provider-crds/templates/git.krateo.io_repoes.yaml b/helm/git-provider-crds/templates/git.krateo.io_repoes.yaml index 2c7b767..39bdec5 100644 --- a/helm/git-provider-crds/templates/git.krateo.io_repoes.yaml +++ b/helm/git-provider-crds/templates/git.krateo.io_repoes.yaml @@ -42,7 +42,9 @@ spec: name: v1alpha1 schema: openAPIV3Schema: - description: A Repo is a managed resource that represents a Krateo Git Repository + description: |- + A Repo is a managed resource that represents a Krateo Git Repository + // +kubebuilder:deprecatedversion:warning="v1alpha1 will be removed in a future release. Please migrate to Kind RemoteRepo" properties: apiVersion: description: |- @@ -321,6 +323,32 @@ spec: description: 'TargetCommitId: last commit identifier of the target repo' type: string + templatingErrors: + description: |- + TemplatingErrors: files whose content could not be rendered, with the renderer's own + message. Populated when a sync fails during templating and cleared on the next + successful one; a failed sync commits nothing, so this describes what would have been + published rather than what was. + items: + description: |- + TemplatingError names a file whose content could not be rendered, and why. + + Templating failures used to surface only as a single wrapped error on the Synced condition, + with no indication of WHICH file was at fault — Go's text/template reports positions against + an anonymous template ("template: template:61: ..."), which is unusable when a sync copies + more than one file. + properties: + message: + description: Message is the renderer's error, verbatim. + type: string + path: + description: Path of the file, relative to the source copy path. + type: string + required: + - message + - path + type: object + type: array type: object required: - spec diff --git a/internal/controllers/common/templating/templating.go b/internal/controllers/common/templating/templating.go index 05d431f..04a5c04 100644 --- a/internal/controllers/common/templating/templating.go +++ b/internal/controllers/common/templating/templating.go @@ -7,9 +7,23 @@ // cannot drift from the first. package templating +import ( + "fmt" + "strings" + + commonapis "github.com/krateoplatformops/git-provider/apis/common/v1alpha1" + "github.com/krateoplatformops/git-provider/internal/tools/copier" + "github.com/krateoplatformops/git-provider/internal/tools/template" +) + // Annotation selects the templating engine applied to file content. const Annotation = "krateo.io/templating-engine" +// DelimsAnnotation overrides the delimiters used for this resource's OWN placeholders, as +// "left,right" — e.g. "{{,}}" to opt back in to Go's native syntax. Absent means the default, +// which is `{% %}` precisely so it does not collide with Helm's `{{ }}`. +const DelimsAnnotation = "krateo.io/templating-delims" + const ( // EngineGoTemplate renders through Go text/template + sprig, ALWAYS — including when the // resource declares no values. Choose it for content that templates without inputs, e.g. @@ -25,3 +39,36 @@ const ( // `` and be committed. EngineNone = "none" ) + +// StatusErrors converts the copier's render failures into the API shape both controllers publish +// on their status. One definition, so the two cannot report the same thing differently. +func StatusErrors(errs []copier.RenderError) []commonapis.TemplatingError { + if len(errs) == 0 { + return nil + } + out := make([]commonapis.TemplatingError, 0, len(errs)) + for _, e := range errs { + out = append(out, commonapis.TemplatingError{Path: e.Path, Message: e.Err.Error()}) + } + return out +} + +// Delims reads DelimsAnnotation. An absent annotation yields the defaults. A malformed value is +// an error rather than a silent fallback: quietly using different delimiters than the author +// asked for would leave their placeholders unsubstituted in a committed file. +func Delims(annotations map[string]string) (left, right string, err error) { + raw, ok := annotations[DelimsAnnotation] + if !ok || strings.TrimSpace(raw) == "" { + return template.DefaultLeftDelim, template.DefaultRightDelim, nil + } + parts := strings.Split(raw, ",") + if len(parts) != 2 { + return "", "", fmt.Errorf("malformed %s %q: expected \"left,right\" (e.g. %q)", + DelimsAnnotation, raw, "{{,}}") + } + left, right = strings.TrimSpace(parts[0]), strings.TrimSpace(parts[1]) + if left == "" || right == "" { + return "", "", fmt.Errorf("malformed %s %q: both delimiters must be non-empty", DelimsAnnotation, raw) + } + return left, right, nil +} diff --git a/internal/controllers/localresource/localresource.go b/internal/controllers/localresource/localresource.go index 171f282..b2e9f8d 100644 --- a/internal/controllers/localresource/localresource.go +++ b/internal/controllers/localresource/localresource.go @@ -405,8 +405,16 @@ func (e *external) SyncLocalResources(ctx context.Context, cr *localResourcev1al return fmt.Errorf("unable to create copier: %w", err) } if err := co.Copy(override); err != nil { + // Name the offending files on the status. The renderer's own message positions itself + // against an anonymous template, so without this the operator sees a line number with + // no file attached. + var failed *copier.RenderFailed + if errors.As(err, &failed) { + cr.Status.TemplatingErrors = templating.StatusErrors(failed.Errors) + } return fmt.Errorf("unable to copy files: %w", err) } + cr.Status.TemplatingErrors = nil log.Debug("Origin and target LocalResource synchronized", "toUrl", spec.ToRepo.Url, @@ -481,14 +489,19 @@ func (e *external) SyncLocalResources(ctx context.Context, cr *localResourcev1al // (`{{ now | date "2006-01-02" }}`, `{{ uuidv4 }}`, `{{ env "USER" }}`), and a file that does // declare values can still contain regions that must survive verbatim. func templatingOptions(annotations map[string]string, values []template.TemplateValue) ([]copier.Option, error) { + left, right, err := templating.Delims(annotations) + if err != nil { + return nil, err + } + switch engine := annotations[templating.Annotation]; engine { case "": if len(values) > 0 { - return []copier.Option{copier.WithGoTemplate(values)}, nil + return []copier.Option{copier.WithGoTemplateDelims(values, left, right)}, nil } return nil, nil case templating.EngineGoTemplate: - return []copier.Option{copier.WithGoTemplate(values)}, nil + return []copier.Option{copier.WithGoTemplateDelims(values, left, right)}, nil case templating.EngineNone: return nil, nil default: diff --git a/internal/controllers/localresource/templating_options_test.go b/internal/controllers/localresource/templating_options_test.go index 936d842..652ac74 100644 --- a/internal/controllers/localresource/templating_options_test.go +++ b/internal/controllers/localresource/templating_options_test.go @@ -50,7 +50,7 @@ func render(t *testing.T, annotations map[string]string, values []template.Templ } const chart = "replicas: {{ .Values.replicas }}\n" -const valueFree = `stamp: {{ "x" | upper }}` + "\n" +const valueFree = `stamp: {% "x" | upper %}` + "\n" // Default, no values: authored content survives. This is the sock-shop case. func TestDefaultWithoutValuesCopiesVerbatim(t *testing.T) { @@ -66,7 +66,7 @@ func TestDefaultWithoutValuesCopiesVerbatim(t *testing.T) { // Default, values declared: substitution still happens. Unchanged behaviour. func TestDefaultWithValuesSubstitutes(t *testing.T) { got, err := render(t, nil, - []template.TemplateValue{{Key: "name", Value: "sock-shop"}}, "project: {{ .name }}\n") + []template.TemplateValue{{Key: "name", Value: "sock-shop"}}, "project: {% .name %}\n") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -88,7 +88,7 @@ func TestGoTemplateRendersWithoutValues(t *testing.T) { // none: never render, even when values ARE declared. func TestNoneNeverRendersEvenWithValues(t *testing.T) { - body := "project: {{ .name }}\n" + body := "project: {% .name %}\n" got, err := render(t, map[string]string{templating.Annotation: templating.EngineNone}, []template.TemplateValue{{Key: "name", Value: "sock-shop"}}, body) if err != nil { @@ -110,15 +110,51 @@ func TestUnknownEngineIsRejected(t *testing.T) { } } -// The regression that started this: gotemplate on a Helm chart still fails, and it should — -// the annotation is a choice, not a promise that Helm syntax will survive a Go render. -func TestGoTemplateStillFailsOnHelmBuiltins(t *testing.T) { - _, err := render(t, map[string]string{templating.Annotation: templating.EngineGoTemplate}, nil, - "command:\n {{- toYaml $x | nindent 4 }}\n") +// The point of the default delimiters: gotemplate no longer touches Helm syntax, so a chart can +// be published WITH substitution — the case that was impossible before. +func TestGoTemplateLeavesHelmSyntaxAloneByDefault(t *testing.T) { + body := "name: {{ include \"x\" . }}\nreplicas: {{ .Values.replicas }}\ntenant: {% .tenant %}\n" + got, err := render(t, map[string]string{templating.Annotation: templating.EngineGoTemplate}, + []template.TemplateValue{{Key: "tenant", Value: "kiratech"}}, body) + if err != nil { + t.Fatalf("Helm syntax should no longer break the render: %v", err) + } + want := "name: {{ include \"x\" . }}\nreplicas: {{ .Values.replicas }}\ntenant: kiratech\n" + if got != want { + t.Errorf("expected Helm syntax preserved and {%% %%} substituted.\n got: %q\nwant: %q", got, want) + } +} + +// Opting back in to Go's native delimiters still works — and on a Helm chart it still fails, +// which is the honest outcome of asking for it. +func TestGoDelimsOptInStillCollidesWithHelm(t *testing.T) { + _, err := render(t, map[string]string{ + templating.Annotation: templating.EngineGoTemplate, + templating.DelimsAnnotation: "{{,}}", + }, nil, "command:\n {{- toYaml $x | nindent 4 }}\n") if err == nil { - t.Fatal("expected the Helm builtin to fail under an explicit gotemplate request") + t.Fatal("expected the Helm builtin to fail once {{ }} is opted into") } if !strings.Contains(err.Error(), `function "toYaml" not defined`) { t.Errorf("unexpected error: %v", err) } } + +func TestGoDelimsOptInSubstitutes(t *testing.T) { + got, err := render(t, map[string]string{templating.DelimsAnnotation: "{{,}}"}, + []template.TemplateValue{{Key: "name", Value: "sock-shop"}}, "project: {{ .name }}\n") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "project: sock-shop\n" { + t.Errorf("expected substitution under opted-in {{ }}, got %q", got) + } +} + +func TestMalformedDelimsRejected(t *testing.T) { + for _, bad := range []string{"{{", "a,b,c", ",}}", "{{,"} { + if _, err := templatingOptions(map[string]string{templating.DelimsAnnotation: bad}, nil); err == nil { + t.Errorf("expected %q to be rejected", bad) + } + } +} diff --git a/internal/controllers/repo/repo.go b/internal/controllers/repo/repo.go index ab0ce65..692da1d 100644 --- a/internal/controllers/repo/repo.go +++ b/internal/controllers/repo/repo.go @@ -3,6 +3,7 @@ package repo import ( "context" "encoding/json" + "errors" "fmt" "os" "strings" @@ -381,7 +382,11 @@ func (e *external) SyncRepos(ctx context.Context, cr *repov1alpha1.Repo, commitM Value: fmt.Sprintf("%v", v), }) } - opts = append(opts, copier.WithGoTemplate(tplVals)) + left, right, derr := templating.Delims(cr.GetAnnotations()) + if derr != nil { + return e.failSync(ctx, cr, derr) + } + opts = append(opts, copier.WithGoTemplateDelims(tplVals, left, right)) } else { opts = append(opts, copier.WithMustacheTemplate(values)) } @@ -399,8 +404,13 @@ func (e *external) SyncRepos(ctx context.Context, cr *repov1alpha1.Repo, commitM } } if err := co.Copy(override); err != nil { + var failed *copier.RenderFailed + if errors.As(err, &failed) { + cr.Status.TemplatingErrors = templating.StatusErrors(failed.Errors) + } return e.failSync(ctx, cr, fmt.Errorf("unable to copy files: %w", err)) } + cr.Status.TemplatingErrors = nil e.log.Info("Origin and target repo synchronized", "fromUrl", spec.FromRepo.Url, diff --git a/internal/tools/copier/copier.go b/internal/tools/copier/copier.go index 7ec370b..ba04241 100644 --- a/internal/tools/copier/copier.go +++ b/internal/tools/copier/copier.go @@ -53,7 +53,15 @@ func WithTargetCopyPath(tcp string) Option { } } +// WithGoTemplate renders with git-provider's default delimiters. func WithGoTemplate(templateValues []template.TemplateValue) Option { + return WithGoTemplateDelims(templateValues, template.DefaultLeftDelim, template.DefaultRightDelim) +} + +// WithGoTemplateDelims renders with the given delimiters, so a resource can move its own +// placeholders out of the way of a delimiter the CONTENT already owns — Helm's `{{ }}` being the +// case that matters. +func WithGoTemplateDelims(templateValues []template.TemplateValue, left, right string) Option { return func(o *options) { values := template.FromTemplateValues(templateValues) o.renderFunc = func(in io.Reader, out io.Writer) error { @@ -62,7 +70,7 @@ func WithGoTemplate(templateValues []template.TemplateValue) Option { return err } tmpl := template.Template(bin) - renderedBin, err := tmpl.Render(values) + renderedBin, err := tmpl.RenderDelims(values, left, right) if err != nil { return err } @@ -72,7 +80,7 @@ func WithGoTemplate(templateValues []template.TemplateValue) Option { } o.renderFileNames = func(src string) (string, error) { tmpl := template.Template(src) - renderedBin, err := tmpl.Render(values) + renderedBin, err := tmpl.RenderDelims(values, left, right) if err != nil { return "", err } @@ -107,15 +115,50 @@ func WithMustacheTemplate(templateValues interface{}) Option { } } +// RenderError is a single file the templating pass could not render. +type RenderError struct { + // Path of the offending file, as seen in the SOURCE tree. + Path string + // Err is the renderer's error, unwrapped. + Err error +} + +// RenderFailed aggregates every file that failed to render in one copy. +// +// A copy collects these rather than stopping at the first, because Go's text/template reports +// positions against an anonymous template ("template: template:61: ...") — with one error and no +// path, an operator copying a tree cannot tell which file is at fault. Copy still returns this as +// an error, so callers abort before committing anything. +type RenderFailed struct { + Errors []RenderError +} + +func (e *RenderFailed) Error() string { + if len(e.Errors) == 1 { + return fmt.Sprintf("rendering %s: %v", e.Errors[0].Path, e.Errors[0].Err) + } + paths := make([]string, 0, len(e.Errors)) + for _, re := range e.Errors { + paths = append(paths, re.Path) + } + return fmt.Sprintf("rendering failed for %d files: %s (first: %v)", + len(e.Errors), strings.Join(paths, ", "), e.Errors[0].Err) +} + type Copier struct { fromFS billy.Filesystem toFS billy.Filesystem krateoIgnore *gi.GitIgnore targetIgnore *gi.GitIgnore + renderErrors []RenderError options } +// RenderErrors returns every file that failed to render during the last Copy, in the order they +// were encountered. Empty when the copy succeeded. +func (co *Copier) RenderErrors() []RenderError { return co.renderErrors } + func NewCopier(fromFS, toFS billy.Filesystem, opts ...Option) (*Copier, error) { if fromFS == nil { return nil, fmt.Errorf("fromFS cannot be nil") @@ -163,7 +206,14 @@ func (co *Copier) Copy(override bool) (err error) { return fmt.Errorf("failed to set krateo ignore: %w", err) } - return co.copyDir(src, dst) + co.renderErrors = nil + if err := co.copyDir(src, dst); err != nil { + return err + } + if len(co.renderErrors) > 0 { + return &RenderFailed{Errors: co.renderErrors} + } + return nil } func (co *Copier) copyDir(src, dst string) (err error) { @@ -266,7 +316,9 @@ func (co *Copier) copyFile(src, dst string, doNotRender bool) (err error) { var err error dst, err = co.renderFileNames(dst) if err != nil { - return fmt.Errorf("failed to render file names: %w", err) + co.renderErrors = append(co.renderErrors, + RenderError{Path: src, Err: fmt.Errorf("rendering the file name: %w", err)}) + return nil } } @@ -296,7 +348,13 @@ func (co *Copier) copyFile(src, dst string, doNotRender bool) (err error) { return err } - return co.renderFunc(in, out) + if rerr := co.renderFunc(in, out); rerr != nil { + // Record and keep going, so one copy reports every offending file instead of only the + // first. The destination file is left partially written, which is safe: Copy returns + // RenderFailed, and every caller aborts before committing. + co.renderErrors = append(co.renderErrors, RenderError{Path: src, Err: rerr}) + } + return nil } func (co *Copier) setKrateoIgnore() error { diff --git a/internal/tools/copier/copier_test.go b/internal/tools/copier/copier_test.go index 590f6c8..ed76424 100644 --- a/internal/tools/copier/copier_test.go +++ b/internal/tools/copier/copier_test.go @@ -47,7 +47,7 @@ func TestRenderFileNamesAndContent(t *testing.T) { to := memfs.New() // source file with templated name and content - writeFile(t, from, "/src/file_{{.name}}.txt", "hello {{.name}}") + writeFile(t, from, "/src/file_{%.name%}.txt", "hello {%.name%}") co, err := NewCopier(from, to, WithOriginCopyPath("/src"), WithTargetCopyPath("/dst"), WithIgnorePath("/"), WithGoTemplate([]template.TemplateValue{{Key: "name", Value: "world"}})) if err != nil { diff --git a/internal/tools/copier/helm_verbatim_test.go b/internal/tools/copier/helm_verbatim_test.go index 4b44a2e..3f03b73 100644 --- a/internal/tools/copier/helm_verbatim_test.go +++ b/internal/tools/copier/helm_verbatim_test.go @@ -36,15 +36,17 @@ func TestCopyLeavesHelmTemplateVerbatimWhenNotTemplating(t *testing.T) { } } -// The regression itself: with a Go-template pass installed, the same file fails with the -// exact error seen on krateo-057 for LocalResource/publish-sock-shop-003. +// The regression itself, now reachable only by opting in to Go's `{{ }}` delimiters: the same +// file fails with the exact error seen on krateo-057 for LocalResource/publish-sock-shop-003. +// With the DEFAULT delimiters this no longer happens at all — see +// TestGoTemplateLeavesHelmSyntaxAloneByDefault in the localresource package. func TestGoTemplatePassRejectsHelmBuiltins(t *testing.T) { src, dst := memfs.New(), memfs.New() writeFile(t, src, "templates/microservices.yaml", helmChart) co, err := NewCopier(src, dst, WithOriginCopyPath("/"), WithTargetCopyPath("/"), - WithGoTemplate(nil), // no values — exactly what an absent placeholders list produced + WithGoTemplateDelims(nil, "{{", "}}"), // opted in to Go's delimiters, no values ) if err != nil { t.Fatalf("NewCopier: %v", err) @@ -66,7 +68,7 @@ func TestGoTemplatePassSilentlyEmptiesUnknownValues(t *testing.T) { writeFile(t, src, "values.yaml", parseable) co, err := NewCopier(src, dst, - WithOriginCopyPath("/"), WithTargetCopyPath("/"), WithGoTemplate(nil)) + WithOriginCopyPath("/"), WithTargetCopyPath("/"), WithGoTemplateDelims(nil, "{{", "}}")) if err != nil { t.Fatalf("NewCopier: %v", err) } @@ -90,7 +92,7 @@ func TestPlaceholdersStillSubstituteWhenPresent(t *testing.T) { co, err := NewCopier(src, dst, WithOriginCopyPath("/"), WithTargetCopyPath("/"), - WithGoTemplate([]template.TemplateValue{{Key: "name", Value: "sock-shop"}})) + WithGoTemplateDelims([]template.TemplateValue{{Key: "name", Value: "sock-shop"}}, "{{", "}}")) if err != nil { t.Fatalf("NewCopier: %v", err) } diff --git a/internal/tools/copier/render_errors_test.go b/internal/tools/copier/render_errors_test.go new file mode 100644 index 0000000..72b77fd --- /dev/null +++ b/internal/tools/copier/render_errors_test.go @@ -0,0 +1,104 @@ +package copier + +import ( + "errors" + "strings" + "testing" + + "github.com/go-git/go-billy/v5/memfs" +) + +// A copy must report EVERY file it could not render, not just the first. Go's text/template +// positions errors against an anonymous template ("template: template:61: ..."), so without the +// path an operator copying a tree cannot tell which file is at fault. +func TestCopyCollectsEveryRenderFailureWithItsPath(t *testing.T) { + src, dst := memfs.New(), memfs.New() + writeFile(t, src, "templates/a.yaml", "x: {{ toYaml .foo }}\n") // Helm builtin + writeFile(t, src, "templates/b.yaml", "y: {{ include \"z\" . }}\n") // Helm builtin + writeFile(t, src, "values.yaml", "replicas: 1\n") // fine + + co, err := NewCopier(src, dst, + WithOriginCopyPath("/"), WithTargetCopyPath("/"), + WithGoTemplateDelims(nil, "{{", "}}")) + if err != nil { + t.Fatalf("NewCopier: %v", err) + } + + err = co.Copy(true) + if err == nil { + t.Fatal("expected the copy to fail") + } + + var failed *RenderFailed + if !errors.As(err, &failed) { + t.Fatalf("expected a *RenderFailed, got %T: %v", err, err) + } + if len(failed.Errors) != 2 { + t.Fatalf("expected 2 failures, got %d: %+v", len(failed.Errors), failed.Errors) + } + + paths := map[string]bool{} + for _, re := range failed.Errors { + paths[re.Path] = true + if re.Err == nil { + t.Errorf("%s: nil error recorded", re.Path) + } + } + for _, want := range []string{"/templates/a.yaml", "/templates/b.yaml"} { + if !paths[want] { + t.Errorf("missing %s; got %v", want, paths) + } + } + + // The aggregate message names the count and the paths, so the condition message is useful + // even before anyone looks at status.templatingErrors. + msg := failed.Error() + for _, want := range []string{"2 files", "a.yaml", "b.yaml"} { + if !strings.Contains(msg, want) { + t.Errorf("aggregate message missing %q: %s", want, msg) + } + } + + // RenderErrors() exposes the same list to the controllers. + if len(co.RenderErrors()) != 2 { + t.Errorf("RenderErrors() = %d, want 2", len(co.RenderErrors())) + } +} + +// A single failure keeps the message readable rather than pluralising awkwardly. +func TestSingleRenderFailureMessageNamesTheFile(t *testing.T) { + src, dst := memfs.New(), memfs.New() + writeFile(t, src, "only.yaml", "x: {{ toYaml .foo }}\n") + + co, _ := NewCopier(src, dst, WithOriginCopyPath("/"), WithTargetCopyPath("/"), + WithGoTemplateDelims(nil, "{{", "}}")) + err := co.Copy(true) + if err == nil || !strings.Contains(err.Error(), "only.yaml") { + t.Fatalf("expected the message to name the file, got: %v", err) + } +} + +// A clean copy leaves no residue for the next one to report. +func TestRenderErrorsResetBetweenCopies(t *testing.T) { + src, dst := memfs.New(), memfs.New() + writeFile(t, src, "bad.yaml", "x: {{ toYaml .foo }}\n") + co, _ := NewCopier(src, dst, WithOriginCopyPath("/"), WithTargetCopyPath("/"), + WithGoTemplateDelims(nil, "{{", "}}")) + if err := co.Copy(true); err == nil { + t.Fatal("expected failure") + } + if len(co.RenderErrors()) != 1 { + t.Fatalf("setup: want 1 error, got %d", len(co.RenderErrors())) + } + + // Same copier, a source that renders cleanly. + src2 := memfs.New() + writeFile(t, src2, "good.yaml", "replicas: 1\n") + co.fromFS = src2 + if err := co.Copy(true); err != nil { + t.Fatalf("second copy should succeed: %v", err) + } + if got := len(co.RenderErrors()); got != 0 { + t.Errorf("errors from the previous copy leaked: %d", got) + } +} diff --git a/internal/tools/template/template.go b/internal/tools/template/template.go index b0e0679..839feb3 100644 --- a/internal/tools/template/template.go +++ b/internal/tools/template/template.go @@ -20,10 +20,45 @@ func FromTemplateValues(tplValues []TemplateValue) map[string]any { return values } +// Delimiters. +// +// git-provider does NOT default to Go's `{{ }}`, and that is deliberate. The files it publishes +// are overwhelmingly Kubernetes manifests and Helm charts, and Helm owns `{{ }}`. Sharing the +// delimiter means a chart cannot be published with substitution at all: `{{ toYaml x }}` and +// `{{ include "y" . }}` fail (both are Helm builtins, not sprig ones), `{{ .Values.n }}` renders +// to the literal ``, and `{{/* ... */}}` headers — which `helm create` writes into +// every _helpers.tpl — are deleted outright. +// +// `{% %}` was chosen over the obvious alternatives by testing them against real YAML: +// +// << >> breaks on the YAML merge key `<<: *defaults` (parse error: expected :=) +// [[ ]] breaks on flow sequences `[[1,2],[3,4]]` (parse error: unexpected ",") +// {% %} safe against both, and not Helm syntax +// +// Use GoLeftDelim/GoRightDelim to opt back in to `{{ }}` where the content is known not to be a +// chart. +const ( + DefaultLeftDelim = "{%" + DefaultRightDelim = "%}" + + GoLeftDelim = "{{" + GoRightDelim = "}}" +) + type Template string +// Render renders with git-provider's default delimiters. func (t Template) Render(values map[string]any) ([]byte, error) { - tpl, err := template.New("template").Funcs(sprig.FuncMap()).Parse(string(t)) + return t.RenderDelims(values, DefaultLeftDelim, DefaultRightDelim) +} + +// RenderDelims renders with the given delimiters. Empty strings fall back to the defaults, so a +// caller that has nothing configured behaves like Render. +func (t Template) RenderDelims(values map[string]any, left, right string) ([]byte, error) { + if left == "" || right == "" { + left, right = DefaultLeftDelim, DefaultRightDelim + } + tpl, err := template.New("template").Delims(left, right).Funcs(sprig.FuncMap()).Parse(string(t)) if err != nil { return nil, err } diff --git a/testdata/local_fromString.yaml b/testdata/local_fromString.yaml index 85762f7..7ef903b 100644 --- a/testdata/local_fromString.yaml +++ b/testdata/local_fromString.yaml @@ -23,7 +23,7 @@ spec: zipArchive: false authMethod: generic branch: main - placeholderTest: "{{ .example }}" + placeholderTest: "{% .example %}" createCommitMessage: "chore: test create files in remote repository" updateCommitMessage: "chore: test update files in remote repository" diff --git a/testdata/local_fromString_override_false.yaml b/testdata/local_fromString_override_false.yaml index f0d86d0..74d0379 100644 --- a/testdata/local_fromString_override_false.yaml +++ b/testdata/local_fromString_override_false.yaml @@ -24,7 +24,7 @@ spec: zipArchive: false authMethod: generic branch: main - placeholderTest: "{{ .example }}" + placeholderTest: "{% .example %}" toRepo: diff --git a/testdata/local_fromString_syncEnabled_false.yaml b/testdata/local_fromString_syncEnabled_false.yaml index 872818d..b35fcfb 100644 --- a/testdata/local_fromString_syncEnabled_false.yaml +++ b/testdata/local_fromString_syncEnabled_false.yaml @@ -24,7 +24,7 @@ spec: zipArchive: false authMethod: generic branch: main - placeholderTest: "{{ .example }}" + placeholderTest: "{% .example %}" toRepo: diff --git a/testdata/local_fromYaml.yaml b/testdata/local_fromYaml.yaml index a453ce0..52bb3de 100644 --- a/testdata/local_fromYaml.yaml +++ b/testdata/local_fromYaml.yaml @@ -24,7 +24,7 @@ spec: zipArchive: false authMethod: generic branch: main - placeholderTest: "{{ .example }}" + placeholderTest: "{% .example %}" toRepo: From 0e67816f3a8b1ed12bbc6d8b93aceb14d625dc3e Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Thu, 10 Sep 2026 12:52:41 +0200 Subject: [PATCH 4/6] feat(templating): report every problem in a file, not the first one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit status.templatingErrors named each failing FILE, but carried only one message per file, because that is all text/template gives: it stops at the first error. A file with three undefined functions reported one, and the operator fixed it, re-synced, and met the next. Worse, the most damaging class was invisible. A reference to a value that was never declared is not an error to the renderer — `{% .environment %}` with no `environment` placeholder renders the literal `` and is committed with a green status. Nothing in the previous version could report that, which made templatingErrors blind to exactly the silent corruption this series has been chasing. tools/template.Validate finds both classes in one pass. It parses with parse.SkipFuncCheck — which yields a tree even when the functions do not exist — then walks it collecting: - every function identifier absent from sprig AND from text/template's builtins - every top-level field reference absent from the declared values Before, for one file: template: template:1: function "toYaml" not defined After, same file: template:2:8: no value declared for ".environment" template:3:13: no value declared for ".a" template:3:6: function "toYaml" not defined template:4:6: function "include" not defined with line:column, which the renderer's message does not carry. Validate REPORTS, it does not gate: Render remains the source of truth for whether a file renders, and a file that renders cleanly is never inspected. The scanner runs only on the failure path, so the success path costs nothing. Two details that matter for message quality: - When Validate's own parse cannot continue (a genuine syntax error, or a `$var` it cannot resolve) it returns a single UNLOCATED problem. The copier detects that via Problem.Located() and keeps the RENDERER's error instead — otherwise a `$svc` the scanner cannot resolve would mask the undefined function the renderer would have named, which is a worse message, not a better one. - RenderFailed.Error() now counts problems and distinct files separately. "4 problems in 1 file" is a very different situation from "4 problems in 4 files", and the condition message is often all anyone reads. Helm syntax is invisible to the scanner under the default delimiters, as it should be: it is not ours to validate. Tests: every undefined function reported (Render reports exactly one, asserted); undeclared values caught where Render returns nil and emits `` (asserted); both classes mixed; clean templates silent; Helm syntax ignored under `{% %}`; syntax errors yield exactly one report; no false positives on builtins, pipelines or command arguments; multi-problem single-file counted correctly in the aggregate message. go build ./... · go vet · go test ./internal/... ./apis/... 8 packages ok Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JumEqTGGJ2hf3vjE3xFQsv --- docs/local-resource.md | 21 ++- internal/tools/copier/copier.go | 69 +++++++-- internal/tools/copier/render_errors_test.go | 32 +++-- internal/tools/template/validate.go | 150 ++++++++++++++++++++ internal/tools/template/validate_test.go | 110 ++++++++++++++ 5 files changed, 358 insertions(+), 24 deletions(-) create mode 100644 internal/tools/template/validate.go create mode 100644 internal/tools/template/validate_test.go diff --git a/docs/local-resource.md b/docs/local-resource.md index 99f389f..f1db2aa 100644 --- a/docs/local-resource.md +++ b/docs/local-resource.md @@ -140,7 +140,9 @@ With the default (no annotation and no placeholders) a chart is already copied v ### When templating fails -A render failure names the file. `status.templatingErrors` lists **every** file that failed, not just the first, because Go reports template positions against an anonymous template (`template: template:61: ...`) which is useless when a sync copies more than one file: +`status.templatingErrors` lists **every** problem found, not one per file and not just the first. + +That matters because `text/template` stops at its first error — a file with three undefined functions reports one, and you would fix, re-sync, and meet the next. Worse, a reference to a value that was never declared is **not an error at all**: it renders as the literal `` and gets committed. Both are reported here: ```yaml status: @@ -148,16 +150,23 @@ status: - type: Synced status: "False" reason: ReconcileError - message: 'unable to copy files: rendering failed for 2 files: templates/a.yaml, templates/b.yaml (first: function "toYaml" not defined)' + message: 'unable to copy files: rendering failed: 4 problems in 1 file + (templates/deployment.yaml); first: template:2:8: no value declared for ".environment"' templatingErrors: - - path: templates/a.yaml - message: 'template: template:1: function "toYaml" not defined' - - path: templates/b.yaml - message: 'template: template:1: function "include" not defined' + - path: templates/deployment.yaml + message: 'template:2:8: no value declared for ".environment"' + - path: templates/deployment.yaml + message: 'template:3:13: no value declared for ".a"' + - path: templates/deployment.yaml + message: 'template:3:6: function "toYaml" not defined' + - path: templates/deployment.yaml + message: 'template:4:6: function "include" not defined' ``` A failed sync commits nothing, so this describes what *would* have been published. The list is cleared on the next successful sync. +**One limit, stated honestly.** A syntax error genuinely stops the parser, so for that class there is only ever one report — everything after the broken action is unreachable. You will see a single entry with no `line:column`, which is the parser's own message. + ## Custom Commits You can customize the commit messages used when the provider pushes changes to the target repository. A description indicating the CR that triggered the commit will be automatically appended to these messages. diff --git a/internal/tools/copier/copier.go b/internal/tools/copier/copier.go index ba04241..83bdd65 100644 --- a/internal/tools/copier/copier.go +++ b/internal/tools/copier/copier.go @@ -1,6 +1,8 @@ package copier import ( + "bytes" + "errors" "fmt" "io" "os" @@ -18,7 +20,10 @@ const IgnoreFileName = ".krateoignore" type Option func(*options) type options struct { - renderFunc func(in io.Reader, out io.Writer) error + renderFunc func(in io.Reader, out io.Writer) error + // validate, when set, reports EVERY problem in a file rather than the one the renderer + // stopped at. Used only to enrich a failure; it never decides whether a copy succeeds. + validate func(body string) []template.Problem renderFileNames func(src string) (string, error) ignorePath string originCopyPath string @@ -28,6 +33,7 @@ type options struct { func defaultOptions() options { return options{ renderFunc: nil, + validate: nil, renderFileNames: nil, ignorePath: "/", originCopyPath: "/", @@ -64,6 +70,9 @@ func WithGoTemplate(templateValues []template.TemplateValue) Option { func WithGoTemplateDelims(templateValues []template.TemplateValue, left, right string) Option { return func(o *options) { values := template.FromTemplateValues(templateValues) + o.validate = func(body string) []template.Problem { + return template.Validate(body, left, right, values) + } o.renderFunc = func(in io.Reader, out io.Writer) error { bin, err := io.ReadAll(in) if err != nil { @@ -133,16 +142,34 @@ type RenderFailed struct { Errors []RenderError } +// Files returns the distinct paths that failed, in encounter order. +func (e *RenderFailed) Files() []string { + seen := map[string]bool{} + out := make([]string, 0, len(e.Errors)) + for _, re := range e.Errors { + if !seen[re.Path] { + seen[re.Path] = true + out = append(out, re.Path) + } + } + return out +} + func (e *RenderFailed) Error() string { if len(e.Errors) == 1 { return fmt.Sprintf("rendering %s: %v", e.Errors[0].Path, e.Errors[0].Err) } - paths := make([]string, 0, len(e.Errors)) - for _, re := range e.Errors { - paths = append(paths, re.Path) + + // One file can carry several problems, so count both — "4 problems in 1 file" is a very + // different situation from "4 problems in 4 files", and the condition message is often all + // an operator reads. + files := e.Files() + noun := "files" + if len(files) == 1 { + noun = "file" } - return fmt.Sprintf("rendering failed for %d files: %s (first: %v)", - len(e.Errors), strings.Join(paths, ", "), e.Errors[0].Err) + return fmt.Sprintf("rendering failed: %d problems in %d %s (%s); first: %v", + len(e.Errors), len(files), noun, strings.Join(files, ", "), e.Errors[0].Err) } type Copier struct { @@ -348,10 +375,32 @@ func (co *Copier) copyFile(src, dst string, doNotRender bool) (err error) { return err } - if rerr := co.renderFunc(in, out); rerr != nil { - // Record and keep going, so one copy reports every offending file instead of only the - // first. The destination file is left partially written, which is safe: Copy returns - // RenderFailed, and every caller aborts before committing. + // Buffer the source so a failure can be re-examined. text/template stops at the first + // problem, so the renderer's error alone would send an operator round a fix-and-resync loop + // once per problem in the file. + body, err := io.ReadAll(in) + if err != nil { + return fmt.Errorf("failed to read source file: %w", err) + } + + if rerr := co.renderFunc(bytes.NewReader(body), out); rerr != nil { + // Record and keep going, so one copy reports every offending FILE, not only the first. + // The destination file is left partially written, which is safe: Copy returns + // RenderFailed and every caller aborts before committing. + // Prefer the scanner only when it actually placed the problems. If its own parse could + // not continue it returns a single unlocated problem — its parser error, which is not + // more useful than the renderer's, and is sometimes less (a `$var` it cannot resolve + // masks the undefined function the renderer would have named). + if co.validate != nil { + problems := co.validate(string(body)) + if len(problems) > 0 && problems[0].Located() { + for _, p := range problems { + co.renderErrors = append(co.renderErrors, + RenderError{Path: src, Err: errors.New(p.String())}) + } + return nil + } + } co.renderErrors = append(co.renderErrors, RenderError{Path: src, Err: rerr}) } return nil diff --git a/internal/tools/copier/render_errors_test.go b/internal/tools/copier/render_errors_test.go index 72b77fd..e7d7e1d 100644 --- a/internal/tools/copier/render_errors_test.go +++ b/internal/tools/copier/render_errors_test.go @@ -13,7 +13,7 @@ import ( // path an operator copying a tree cannot tell which file is at fault. func TestCopyCollectsEveryRenderFailureWithItsPath(t *testing.T) { src, dst := memfs.New(), memfs.New() - writeFile(t, src, "templates/a.yaml", "x: {{ toYaml .foo }}\n") // Helm builtin + writeFile(t, src, "templates/a.yaml", "x: {{ toYaml .foo }}\n") // Helm builtin writeFile(t, src, "templates/b.yaml", "y: {{ include \"z\" . }}\n") // Helm builtin writeFile(t, src, "values.yaml", "replicas: 1\n") // fine @@ -33,8 +33,20 @@ func TestCopyCollectsEveryRenderFailureWithItsPath(t *testing.T) { if !errors.As(err, &failed) { t.Fatalf("expected a *RenderFailed, got %T: %v", err, err) } - if len(failed.Errors) != 2 { - t.Fatalf("expected 2 failures, got %d: %+v", len(failed.Errors), failed.Errors) + // a.yaml has TWO problems (undefined function AND an undeclared value); b.yaml has one. + // The scanner reports every problem, not one per file. + byPath := map[string]int{} + for _, re := range failed.Errors { + byPath[re.Path]++ + } + if byPath["/templates/a.yaml"] != 2 { + t.Errorf("expected 2 problems in a.yaml, got %d: %+v", byPath["/templates/a.yaml"], failed.Errors) + } + if byPath["/templates/b.yaml"] != 1 { + t.Errorf("expected 1 problem in b.yaml, got %d", byPath["/templates/b.yaml"]) + } + if byPath["/values.yaml"] != 0 { + t.Errorf("values.yaml renders cleanly and must not be reported") } paths := map[string]bool{} @@ -52,16 +64,20 @@ func TestCopyCollectsEveryRenderFailureWithItsPath(t *testing.T) { // The aggregate message names the count and the paths, so the condition message is useful // even before anyone looks at status.templatingErrors. + if got := len(failed.Files()); got != 2 { + t.Errorf("Files() = %d, want 2 distinct paths", got) + } + msg := failed.Error() - for _, want := range []string{"2 files", "a.yaml", "b.yaml"} { + for _, want := range []string{"3 problems", "2 files", "a.yaml", "b.yaml"} { if !strings.Contains(msg, want) { t.Errorf("aggregate message missing %q: %s", want, msg) } } // RenderErrors() exposes the same list to the controllers. - if len(co.RenderErrors()) != 2 { - t.Errorf("RenderErrors() = %d, want 2", len(co.RenderErrors())) + if len(co.RenderErrors()) != 3 { + t.Errorf("RenderErrors() = %d, want 3", len(co.RenderErrors())) } } @@ -87,8 +103,8 @@ func TestRenderErrorsResetBetweenCopies(t *testing.T) { if err := co.Copy(true); err == nil { t.Fatal("expected failure") } - if len(co.RenderErrors()) != 1 { - t.Fatalf("setup: want 1 error, got %d", len(co.RenderErrors())) + if len(co.RenderErrors()) == 0 { + t.Fatalf("setup: expected at least one error") } // Same copier, a source that renders cleanly. diff --git a/internal/tools/template/validate.go b/internal/tools/template/validate.go new file mode 100644 index 0000000..d8794cf --- /dev/null +++ b/internal/tools/template/validate.go @@ -0,0 +1,150 @@ +package template + +import ( + "fmt" + "sort" + "text/template/parse" + + "github.com/Masterminds/sprig/v3" +) + +// Problem is one thing wrong with a template, located in the source. +type Problem struct { + // Location is "line:column", as text/template reports positions. + Location string + // Message describes the problem in the renderer's own vocabulary. + Message string +} + +func (p Problem) String() string { + if p.Location == "" { + return p.Message + } + return fmt.Sprintf("%s: %s", p.Location, p.Message) +} + +// Located reports whether Validate could place this problem in the source. It is false only when +// the parser itself could not continue, in which case the single problem IS the parser's error +// and the caller is better off keeping the renderer's own message. +func (p Problem) Located() bool { return p.Location != "" } + +// builtins are the functions text/template defines itself; they are not in sprig's map but are +// always callable, so treating them as undefined would produce false reports. +var builtins = map[string]bool{ + "and": true, "call": true, "html": true, "index": true, "slice": true, "js": true, + "len": true, "not": true, "or": true, "print": true, "printf": true, "println": true, + "urlquery": true, "eq": true, "ge": true, "gt": true, "le": true, "lt": true, "ne": true, + "break": true, "continue": true, +} + +// Validate reports EVERY problem it can find in one pass, which Render cannot do. +// +// text/template stops at the first error — a file with three undefined functions reports one, and +// the operator then fixes, re-syncs, and meets the next. Worse, a reference to a value that was +// never declared is not an error at all: it renders as the literal "" and is committed. +// +// Validate side-steps both by parsing with parse.SkipFuncCheck, which yields a tree even when the +// functions do not exist, then walking it: +// +// - every function identifier absent from sprig and from text/template's builtins +// - every top-level field reference absent from the supplied values +// +// It is a REPORTING aid, not the gate: Render remains the source of truth for whether a file +// renders. A syntax error still stops the parser, so for that class Validate returns the single +// error the parser gives, exactly as before. +func Validate(body, left, right string, values map[string]any) []Problem { + if left == "" || right == "" { + left, right = DefaultLeftDelim, DefaultRightDelim + } + + tree := parse.New("template") + tree.Mode = parse.SkipFuncCheck + + parsed, err := tree.Parse(body, left, right, map[string]*parse.Tree{}) + if err != nil { + // The parser cannot continue past a syntax error, so this really is all there is. + return []Problem{{Location: "", Message: err.Error()}} + } + + funcs := sprig.FuncMap() + seen := map[string]bool{} + var out []Problem + + add := func(node parse.Node, msg string) { + loc := "" + if ctx, _ := parsed.ErrorContext(node); ctx != "" { + loc = ctx + } + key := loc + "\x00" + msg + if seen[key] { + return + } + seen[key] = true + out = append(out, Problem{Location: loc, Message: msg}) + } + + var walk func(parse.Node) + walkPipe := func(p *parse.PipeNode) { + if p != nil { + walk(p) + } + } + + walk = func(n parse.Node) { + switch t := n.(type) { + case *parse.ListNode: + if t == nil { + return + } + for _, c := range t.Nodes { + walk(c) + } + case *parse.ActionNode: + walkPipe(t.Pipe) + case *parse.IfNode: + walkPipe(t.Pipe) + walk(t.List) + walk(t.ElseList) + case *parse.RangeNode: + walkPipe(t.Pipe) + walk(t.List) + walk(t.ElseList) + case *parse.WithNode: + walkPipe(t.Pipe) + walk(t.List) + walk(t.ElseList) + case *parse.TemplateNode: + walkPipe(t.Pipe) + case *parse.PipeNode: + for _, c := range t.Cmds { + walk(c) + } + case *parse.CommandNode: + for i, a := range t.Args { + switch arg := a.(type) { + case *parse.IdentifierNode: + // Only the FIRST argument of a command is the function being called; + // later identifiers are arguments to it. + if i == 0 && !builtins[arg.Ident] { + if _, ok := funcs[arg.Ident]; !ok { + add(arg, fmt.Sprintf("function %q not defined", arg.Ident)) + } + } + case *parse.FieldNode: + if len(arg.Ident) > 0 && values != nil { + if _, ok := values[arg.Ident[0]]; !ok { + add(arg, fmt.Sprintf("no value declared for %q", "."+arg.Ident[0])) + } + } + case *parse.PipeNode: + walk(arg) + } + } + } + } + + walk(parsed.Root) + + sort.SliceStable(out, func(i, j int) bool { return out[i].Location < out[j].Location }) + return out +} diff --git a/internal/tools/template/validate_test.go b/internal/tools/template/validate_test.go new file mode 100644 index 0000000..88681f6 --- /dev/null +++ b/internal/tools/template/validate_test.go @@ -0,0 +1,110 @@ +package template + +import ( + "strings" + "testing" +) + +// The point of Validate: Render reports ONE problem per file; this reports all of them. +func TestValidateReportsEveryUndefinedFunction(t *testing.T) { + body := "a: {% toYaml .x %}\nb: {% include \"y\" . %}\nc: {% tpl .z %}\n" + + if _, err := Template(body).Render(map[string]any{"x": 1, "z": 2}); err == nil { + t.Fatal("setup: expected Render to fail") + } else if strings.Count(err.Error(), "not defined") != 1 { + t.Fatalf("setup: Render should report exactly one, got %v", err) + } + + got := Validate(body, "", "", map[string]any{"x": 1, "z": 2}) + if len(got) != 3 { + t.Fatalf("expected 3 problems, got %d: %v", len(got), got) + } + for _, want := range []string{"toYaml", "include", "tpl"} { + found := false + for _, p := range got { + if strings.Contains(p.Message, want) { + found = true + } + } + if !found { + t.Errorf("missing a report for %q: %v", want, got) + } + } + for _, p := range got { + if p.Location == "" { + t.Errorf("problem without a location: %v", p) + } + } +} + +// The silent half: a reference to a value nobody declared is NOT an error to Render — it renders +// "" and gets committed. Validate catches it. +func TestValidateCatchesUndeclaredValues(t *testing.T) { + body := "tenant: {% .tenant %}\nenv: {% .environment %}\ncluster: {% .cluster %}\n" + values := map[string]any{"tenant": "kiratech"} + + out, err := Template(body).Render(values) + if err != nil { + t.Fatalf("setup: Render should NOT fail here, got %v", err) + } + if !strings.Contains(string(out), "") { + t.Fatalf("setup: expected silent , got %q", string(out)) + } + + got := Validate(body, "", "", values) + if len(got) != 2 { + t.Fatalf("expected 2 undeclared values, got %d: %v", len(got), got) + } + for _, want := range []string{".environment", ".cluster"} { + found := false + for _, p := range got { + if strings.Contains(p.Message, want) { + found = true + } + } + if !found { + t.Errorf("missing a report for %q: %v", want, got) + } + } +} + +func TestValidateMixesBothClasses(t *testing.T) { + body := "a: {% toYaml .x %}\nb: {% .undeclared %}\n" + got := Validate(body, "", "", map[string]any{"x": 1}) + if len(got) != 2 { + t.Fatalf("expected 2 problems, got %d: %v", len(got), got) + } +} + +func TestValidateCleanTemplateHasNoProblems(t *testing.T) { + body := "project: {% .name | upper %}\nstamp: {% now | date \"2006\" %}\n" + if got := Validate(body, "", "", map[string]any{"name": "sock-shop"}); len(got) != 0 { + t.Errorf("expected no problems, got %v", got) + } +} + +// Helm syntax must not be reported under the default delimiters — it is not ours to validate. +func TestValidateIgnoresHelmSyntaxUnderDefaultDelims(t *testing.T) { + body := "name: {{ include \"x\" . }}\nreplicas: {{ .Values.replicas }}\ntenant: {% .tenant %}\n" + if got := Validate(body, "", "", map[string]any{"tenant": "kiratech"}); len(got) != 0 { + t.Errorf("Helm syntax should be invisible under {%% %%}, got %v", got) + } +} + +// A syntax error genuinely stops the parser, so one report is all there is — say so honestly +// rather than pretending to be exhaustive. +func TestValidateReturnsSingleProblemForSyntaxError(t *testing.T) { + got := Validate("a: {% .x\n", "", "", nil) + if len(got) != 1 { + t.Fatalf("expected exactly 1 problem for a syntax error, got %v", got) + } +} + +// Builtins and pipeline arguments must not produce false reports. +func TestValidateNoFalsePositives(t *testing.T) { + body := "a: {% printf \"%s\" .name %}\nb: {% if not .flag %}x{% end %}\nc: {% len .items %}\n" + values := map[string]any{"name": "x", "flag": true, "items": []int{1}} + if got := Validate(body, "", "", values); len(got) != 0 { + t.Errorf("expected no problems, got %v", got) + } +} From cefc39d6101a009bc31c7085b527d2fbac7c880e Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Thu, 10 Sep 2026 13:00:23 +0200 Subject: [PATCH 5/6] feat(templating): validate every file, and give Repo the same vocabulary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps found by testing the design rather than trusting it. ## 1. The scanner only ran on files that had already failed A reference to a value that was never declared is NOT an error to text/template — it renders the literal `` and returns nil. So a file whose ONLY fault was an undeclared value never entered the failure path, the scanner never saw it, and it was committed silently. status.templatingErrors was blind to exactly the corruption it was added to surface. Demonstrated with a four-file tree: the syntax-error file and the undefined-function file were both reported, and the undeclared-value file was not. Validation now runs on every rendered file. A file with an undeclared value fails the sync instead of publishing ``. That IS a behaviour change and it is the intended one: a placeholder is declared on the same CR that references it, so referencing an undeclared one is a typo essentially every time, and the alternative is a corrupted file with a green status. Render remains the source of truth for HOW a file renders; the scanner decides whether it should have been published at all. Also confirmed while testing, and worth recording because it was asked: a syntax error in one file does NOT stop the others being checked. copyFile records and returns, so the walk continues — a tree with a syntax error, an undefined function and an undeclared value reports all three files in one sync. ## 2. Repo disagreed with LocalResource, again Repo picked its engine with `if engine == "gotemplate" { ... } else { mustache }`, so EVERY other value fell through to mustache: - "none" silently got mustache — the opposite of what it asks for - "gotmeplate" silently got mustache — and mustache renders an unresolved key as EMPTY (AllowMissingVariables defaults to true), so a typo in an annotation quietly gutted the copied files and reported success Repo now uses the shared vocabulary: `none` is honoured, `gotemplate` gets the delimiters and the scanner, an unrecognised value is rejected by name, and an absent annotation still means mustache so existing Repos are unaffected. The decision is extracted into templatingOptions(), mirroring localresource, so both controllers are readable side by side. mustache stays available and is now documented as LEGACY where someone will read it — it uses `{{ }}` so it collides with Helm exactly as Go templates did, it fails silently rather than loudly, and it cannot be scanned, so templatingErrors carries no per-problem detail for it. ## Tests New internal/controllers/repo/templating_options_test.go drives the real copier: no values copies verbatim; gotemplate coexists with Helm syntax; the scanner reaches the Repo path and names undeclared values; "none" is honoured rather than becoming mustache; an unknown engine is rejected by name; mustache still works AND the test pins WHY it is legacy (it eats `{{ .Values.replicas }}` to an empty string); the default remains mustache; malformed delimiters are rejected. go build ./... · go vet · go test ./internal/... ./apis/... 8 packages ok Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JumEqTGGJ2hf3vjE3xFQsv --- docs/repo.md | 6 +- internal/controllers/repo/repo.go | 77 +++++++-- .../repo/templating_options_test.go | 155 ++++++++++++++++++ internal/tools/copier/copier.go | 39 +++-- internal/tools/copier/helm_verbatim_test.go | 37 +++-- 5 files changed, 264 insertions(+), 50 deletions(-) create mode 100644 internal/controllers/repo/templating_options_test.go diff --git a/docs/repo.md b/docs/repo.md index a34311a..5e4c3e4 100644 --- a/docs/repo.md +++ b/docs/repo.md @@ -11,7 +11,11 @@ A common use case is **project bootstrapping**. You might have a "golden path" o ## Templating -> **Delimiters.** Placeholders use `{% %}`, not `{{ }}`, so they do not collide with Helm syntax in the files being copied. Override with `krateo.io/templating-delims: "left,right"`. See [local-resource.md](local-resource.md#delimiters) for the reasoning. +> **Engines.** `krateo.io/templating-engine` accepts `gotemplate`, `mustache` and `none`. Absent keeps the historical default, **mustache**. `none` is now honoured — it previously fell through and silently got mustache — and an unrecognised value is now rejected rather than silently switching engine. +> +> **`mustache` is legacy.** It also uses `{{ }}`, so it collides with Helm exactly as Go templates did; `AllowMissingVariables` defaults to true, so a key it cannot resolve renders as **empty with no error**; and it cannot be scanned, so `status.templatingErrors` carries no per-problem detail for it. Prefer `gotemplate`. +> +> **Delimiters.** Under `gotemplate`, placeholders use `{% %}`, not `{{ }}`, so they do not collide with Helm syntax in the files being copied. Override with `krateo.io/templating-delims: "left,right"`. See [local-resource.md](local-resource.md#delimiters) for the reasoning. > > **Failures.** `status.templatingErrors` lists every file that could not be rendered, with the renderer's message. A failed sync commits nothing. diff --git a/internal/controllers/repo/repo.go b/internal/controllers/repo/repo.go index 692da1d..c5cddf7 100644 --- a/internal/controllers/repo/repo.go +++ b/internal/controllers/repo/repo.go @@ -372,25 +372,11 @@ func (e *external) SyncRepos(ctx context.Context, cr *repov1alpha1.Repo, commitM copier.WithIgnorePath(spec.FromRepo.KrateoIgnorePath), } - if values != nil { - engine := cr.GetAnnotations()[AnnotationTemplatingEngine] - if engine == "gotemplate" { - tplVals := make([]template.TemplateValue, 0, len(values)) - for k, v := range values { - tplVals = append(tplVals, template.TemplateValue{ - Key: k, - Value: fmt.Sprintf("%v", v), - }) - } - left, right, derr := templating.Delims(cr.GetAnnotations()) - if derr != nil { - return e.failSync(ctx, cr, derr) - } - opts = append(opts, copier.WithGoTemplateDelims(tplVals, left, right)) - } else { - opts = append(opts, copier.WithMustacheTemplate(values)) - } + tplOpts, terr := templatingOptions(cr.GetAnnotations(), values) + if terr != nil { + return e.failSync(ctx, cr, terr) } + opts = append(opts, tplOpts...) co, err := copier.NewCopier(fromRepo.FS(), toRepo.FS(), opts...) if err != nil { @@ -469,3 +455,58 @@ func (e *external) SyncRepos(ctx context.Context, cr *repov1alpha1.Repo, commitM } return nil } + +// templatingOptions decides which templating pass — if any — a Repo gets, in one place and with +// the same vocabulary the localresource controller uses. +// +// The two controllers have produced four defects by disagreeing about shared rules, so the +// engine names, the delimiter annotation and the accepted values all come from one package. +// What stays deliberately different is the DEFAULT: a Repo templates only when it was given +// values (spec.configMapKeyRef), and mustache remains its historical default engine. +// +// Two behaviours change, both in the direction of not guessing: +// +// - "none" is now honoured. It previously fell through to the else branch and silently got +// MUSTACHE, which is the opposite of what it asks for. +// - An unrecognised value is now rejected. It previously also fell through to mustache, so a +// typo such as "gotmeplate" silently switched engine — and mustache renders a missing key as +// empty (AllowMissingVariables defaults to true), so the result was a quietly gutted file +// with a green status. +func templatingOptions(annotations map[string]string, values map[string]interface{}) ([]copier.Option, error) { + if values == nil { + return nil, nil + } + + engine := annotations[templating.Annotation] + if engine == templating.EngineNone { + return nil, nil + } + + if engine == templating.EngineGoTemplate { + left, right, err := templating.Delims(annotations) + if err != nil { + return nil, err + } + tplVals := make([]template.TemplateValue, 0, len(values)) + for k, v := range values { + tplVals = append(tplVals, template.TemplateValue{Key: k, Value: fmt.Sprintf("%v", v)}) + } + return []copier.Option{copier.WithGoTemplateDelims(tplVals, left, right)}, nil + } + + // Mustache: the historical default, kept for compatibility. It is LEGACY, and the reasons are + // worth stating where someone will read them: + // - it also uses `{{ }}`, so it collides with Helm exactly as Go templates did; + // - AllowMissingVariables defaults to true, so an unresolved key renders as EMPTY with no + // error — a silent failure rather than a loud one; + // - it cannot be scanned by tools/template.Validate, so status.templatingErrors carries no + // per-problem detail for it. + // Prefer gotemplate with the default delimiters. + if engine == "" || engine == templating.EngineMustache { + return []copier.Option{copier.WithMustacheTemplate(values)}, nil + } + + return nil, fmt.Errorf("unsupported %s %q: expected %q, %q, %q, or the annotation to be absent", + templating.Annotation, engine, + templating.EngineGoTemplate, templating.EngineMustache, templating.EngineNone) +} diff --git a/internal/controllers/repo/templating_options_test.go b/internal/controllers/repo/templating_options_test.go new file mode 100644 index 0000000..5cfc639 --- /dev/null +++ b/internal/controllers/repo/templating_options_test.go @@ -0,0 +1,155 @@ +package repo + +import ( + "strings" + "testing" + + "github.com/go-git/go-billy/v5/memfs" + "github.com/krateoplatformops/git-provider/internal/controllers/common/templating" + "github.com/krateoplatformops/git-provider/internal/tools/copier" +) + +// render drives the real copier, so these assert what a Repo would actually commit. +func render(t *testing.T, annotations map[string]string, values map[string]interface{}, body string) (string, error) { + t.Helper() + + opts, err := templatingOptions(annotations, values) + if err != nil { + return "", err + } + + src, dst := memfs.New(), memfs.New() + f, err := src.Create("f.yaml") + if err != nil { + t.Fatalf("create: %v", err) + } + if _, err := f.Write([]byte(body)); err != nil { + t.Fatalf("write: %v", err) + } + f.Close() + + co, err := copier.NewCopier(src, dst, + append([]copier.Option{copier.WithOriginCopyPath("/"), copier.WithTargetCopyPath("/")}, opts...)...) + if err != nil { + t.Fatalf("NewCopier: %v", err) + } + if err := co.Copy(true); err != nil { + return "", err + } + + out, err := dst.Open("f.yaml") + if err != nil { + t.Fatalf("open: %v", err) + } + defer out.Close() + buf := make([]byte, 8192) + n, _ := out.Read(buf) + return string(buf[:n]), nil +} + +// A Repo with no configMapKeyRef templates nothing — unchanged, and the reason a chart copies +// cleanly today. +func TestRepoNoValuesCopiesVerbatim(t *testing.T) { + body := "replicas: {{ .Values.replicas }}\ntenant: {% .tenant %}\n" + got, err := render(t, nil, nil, body) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != body { + t.Errorf("expected verbatim, got %q", got) + } +} + +// gotemplate on a Repo now behaves exactly as it does on a LocalResource: Helm syntax is +// untouched, and the Repo's own placeholders substitute. +func TestRepoGoTemplateCoexistsWithHelm(t *testing.T) { + body := "name: {{ include \"x\" . }}\nreplicas: {{ .Values.replicas }}\ntenant: {% .tenant %}\n" + got, err := render(t, map[string]string{templating.Annotation: templating.EngineGoTemplate}, + map[string]interface{}{"tenant": "kiratech"}, body) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := "name: {{ include \"x\" . }}\nreplicas: {{ .Values.replicas }}\ntenant: kiratech\n" + if got != want { + t.Errorf("got %q\nwant %q", got, want) + } +} + +// The undeclared-value scanner reaches the Repo path too. +func TestRepoGoTemplateCatchesUndeclaredValues(t *testing.T) { + _, err := render(t, map[string]string{templating.Annotation: templating.EngineGoTemplate}, + map[string]interface{}{"tenant": "kiratech"}, "env: {% .environment %}\n") + if err == nil { + t.Fatal("expected the undeclared value to fail the sync") + } + if !strings.Contains(err.Error(), `no value declared for ".environment"`) { + t.Errorf("unexpected error: %v", err) + } +} + +// REGRESSION: "none" used to fall through to the else branch and silently get MUSTACHE — the +// opposite of what it asks for. +func TestRepoNoneIsHonouredNotMustache(t *testing.T) { + body := "tenant: {{tenant}}\nreplicas: {{ .Values.replicas }}\n" + got, err := render(t, map[string]string{templating.Annotation: templating.EngineNone}, + map[string]interface{}{"tenant": "kiratech"}, body) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != body { + t.Errorf("none must copy verbatim; mustache would have substituted. got %q", got) + } +} + +// REGRESSION: a typo used to switch engine silently. +func TestRepoUnknownEngineRejected(t *testing.T) { + _, err := templatingOptions(map[string]string{templating.Annotation: "gotmeplate"}, + map[string]interface{}{"a": 1}) + if err == nil { + t.Fatal("expected an unrecognised engine to be rejected") + } + if !strings.Contains(err.Error(), "gotmeplate") { + t.Errorf("the error should name the offending value, got: %v", err) + } +} + +// Mustache stays available, and this pins WHY it is documented as legacy: it uses `{{ }}`, so it +// eats Helm syntax, and a key it cannot resolve renders EMPTY rather than failing. +func TestRepoMustacheStillWorksAndShowsWhyItIsLegacy(t *testing.T) { + got, err := render(t, map[string]string{templating.Annotation: templating.EngineMustache}, + map[string]interface{}{"tenant": "kiratech"}, + "tenant: {{tenant}}\nreplicas: {{ .Values.replicas }}\n") + if err != nil { + t.Fatalf("mustache should still work: %v", err) + } + if !strings.Contains(got, "tenant: kiratech") { + t.Errorf("mustache should substitute its own key, got %q", got) + } + if strings.Contains(got, ".Values.replicas") { + t.Errorf("expected mustache to have eaten the Helm expression; got %q", got) + } + if !strings.Contains(got, "replicas: \n") { + t.Errorf("expected the Helm expression to render EMPTY (silent), got %q", got) + } +} + +// Absent annotation keeps mustache, so existing Repos are unaffected. +func TestRepoDefaultRemainsMustache(t *testing.T) { + got, err := render(t, nil, map[string]interface{}{"tenant": "kiratech"}, "tenant: {{tenant}}\n") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "tenant: kiratech\n" { + t.Errorf("default engine changed; got %q", got) + } +} + +func TestRepoMalformedDelimsRejected(t *testing.T) { + _, err := templatingOptions(map[string]string{ + templating.Annotation: templating.EngineGoTemplate, + templating.DelimsAnnotation: "oops", + }, map[string]interface{}{"a": 1}) + if err == nil { + t.Fatal("expected malformed delimiters to be rejected") + } +} diff --git a/internal/tools/copier/copier.go b/internal/tools/copier/copier.go index 83bdd65..4413567 100644 --- a/internal/tools/copier/copier.go +++ b/internal/tools/copier/copier.go @@ -383,24 +383,31 @@ func (co *Copier) copyFile(src, dst string, doNotRender bool) (err error) { return fmt.Errorf("failed to read source file: %w", err) } - if rerr := co.renderFunc(bytes.NewReader(body), out); rerr != nil { - // Record and keep going, so one copy reports every offending FILE, not only the first. - // The destination file is left partially written, which is safe: Copy returns - // RenderFailed and every caller aborts before committing. - // Prefer the scanner only when it actually placed the problems. If its own parse could - // not continue it returns a single unlocated problem — its parser error, which is not - // more useful than the renderer's, and is sometimes less (a `$var` it cannot resolve - // masks the undefined function the renderer would have named). - if co.validate != nil { - problems := co.validate(string(body)) - if len(problems) > 0 && problems[0].Located() { - for _, p := range problems { - co.renderErrors = append(co.renderErrors, - RenderError{Path: src, Err: errors.New(p.String())}) - } - return nil + rerr := co.renderFunc(bytes.NewReader(body), out) + + // Validate EVERY rendered file, not only the ones that failed. + // + // The renderer is not a sufficient gate: a reference to a value that was never declared is + // not an error to text/template — it emits the literal "" and returns nil. A file + // whose only fault is an undeclared value would otherwise be committed silently, which is + // precisely the corruption this reporting exists to surface. + if co.validate != nil { + problems := co.validate(string(body)) + // An UNLOCATED problem means the scanner's own parse could not continue. Its message is + // no better than the renderer's and is sometimes worse (a `$var` it cannot resolve masks + // the undefined function the renderer would have named), so defer to rerr in that case. + if len(problems) > 0 && problems[0].Located() { + for _, p := range problems { + co.renderErrors = append(co.renderErrors, RenderError{Path: src, Err: errors.New(p.String())}) } + return nil } + } + + if rerr != nil { + // Record and keep going, so one copy reports every offending FILE, not only the first. + // The destination file may be left partially written, which is safe: Copy returns + // RenderFailed and every caller aborts before committing. co.renderErrors = append(co.renderErrors, RenderError{Path: src, Err: rerr}) } return nil diff --git a/internal/tools/copier/helm_verbatim_test.go b/internal/tools/copier/helm_verbatim_test.go index 3f03b73..55a3003 100644 --- a/internal/tools/copier/helm_verbatim_test.go +++ b/internal/tools/copier/helm_verbatim_test.go @@ -5,7 +5,7 @@ import ( "testing" "github.com/go-git/go-billy/v5/memfs" - "github.com/krateoplatformops/git-provider/internal/tools/template" + tpl "github.com/krateoplatformops/git-provider/internal/tools/template" ) // A Helm chart template, as the Blueprint Builder publishes it. `toYaml` is a Helm @@ -60,29 +60,36 @@ func TestGoTemplatePassRejectsHelmBuiltins(t *testing.T) { } } -// The quieter half, and the reason this matters beyond one blocked publish: a template -// that merely PARSES is executed, and a missing value is committed as an empty string. -func TestGoTemplatePassSilentlyEmptiesUnknownValues(t *testing.T) { +// The quieter half, and the reason this matters beyond one blocked publish: a template that +// merely PARSES used to be executed with a missing value committed as `` — no error, +// nothing in the logs, a corrupted file in the repo. It is now caught and the copy fails. +func TestUndeclaredValueIsCaughtRatherThanCommitted(t *testing.T) { src, dst := memfs.New(), memfs.New() - const parseable = "replicas: {{ .Values.replicas }}\n" + const parseable = "replicas: {% .replicas %}\n" writeFile(t, src, "values.yaml", parseable) co, err := NewCopier(src, dst, - WithOriginCopyPath("/"), WithTargetCopyPath("/"), WithGoTemplateDelims(nil, "{{", "}}")) + WithOriginCopyPath("/"), WithTargetCopyPath("/"), WithGoTemplate(nil)) if err != nil { t.Fatalf("NewCopier: %v", err) } - if err := co.Copy(true); err != nil { - t.Fatalf("Copy: %v", err) + + err = co.Copy(true) + if err == nil { + t.Fatal("expected the undeclared value to fail the copy, not render to ") + } + if !strings.Contains(err.Error(), `no value declared for ".replicas"`) { + t.Errorf("expected the undeclared value to be named, got: %v", err) } - got := readFile(t, dst, "values.yaml") - if got == parseable { - t.Fatal("expected the value to be substituted away; the premise of this test is wrong") + + // The renderer alone would NOT have caught this — that is the whole point. + out, rerr := tpl.Template(parseable).Render(map[string]any{}) + if rerr != nil { + t.Fatalf("premise check: Render should succeed silently, got %v", rerr) } - if got != "replicas: \n" && got != "replicas: \n" { - t.Logf("rendered to %q", got) + if !strings.Contains(string(out), "") { + t.Fatalf("premise check: expected , got %q", string(out)) } - t.Logf("silent corruption confirmed: %q -> %q", parseable, got) } // Substitution must still work when the CR does ask for it. @@ -92,7 +99,7 @@ func TestPlaceholdersStillSubstituteWhenPresent(t *testing.T) { co, err := NewCopier(src, dst, WithOriginCopyPath("/"), WithTargetCopyPath("/"), - WithGoTemplateDelims([]template.TemplateValue{{Key: "name", Value: "sock-shop"}}, "{{", "}}")) + WithGoTemplateDelims([]tpl.TemplateValue{{Key: "name", Value: "sock-shop"}}, "{{", "}}")) if err != nil { t.Fatalf("NewCopier: %v", err) } From 53fc262896ae9e158ea1704cee3992b48d74b73e Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Thu, 10 Sep 2026 15:22:31 +0200 Subject: [PATCH 6/6] test(repo): fix TC09 for the new delimiters, and pin the opt-in end to end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught a real break, not the flake this branch hit earlier: TC09 is the integration test for the gotemplate annotation, and its fixture used `{{ .name }}` with an expectation of substitution. With `{% %}` as the default that line is no longer ours to render, so the file arrived verbatim and the assertion failed. The test was right and the fixture was stale. TC09 now uses the default delimiters AND carries a Helm expression beside them: "Hello {% .name %}!\nreplicas: {{ .Values.replicas }}\n" -> "Hello GoTemplate!\nreplicas: {{ .Values.replicas }}\n" which makes it a stronger test than it was. The coexistence — our placeholder substituted, Helm's untouched — is the whole point of the delimiter choice, and it is only observable end to end through a real clone, render, commit and push. The unit tests can show the copier's behaviour; only this shows what lands in the remote. TC11 pins the other direction: `krateo.io/templating-delims: "{{,}}"` restores Go's native syntax for content known not to be a chart, and still substitutes. Without it the opt-in path had no integration coverage at all. go build ./... · go vet -tags integration · go test ./internal/... ./apis/... 8 packages ok. The integration suite needs Docker and Gitea, so TC09/TC11 are verified by CI rather than locally — which is how the stale fixture surfaced. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JumEqTGGJ2hf3vjE3xFQsv --- internal/controllers/repo/repo_test.go | 41 ++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/internal/controllers/repo/repo_test.go b/internal/controllers/repo/repo_test.go index 39d6e31..eb017ca 100644 --- a/internal/controllers/repo/repo_test.go +++ b/internal/controllers/repo/repo_test.go @@ -30,6 +30,7 @@ import ( repov1alpha1 "github.com/krateoplatformops/git-provider/apis/repo/v1alpha1" gitclient "github.com/krateoplatformops/git-provider/internal/clients/git" "github.com/krateoplatformops/git-provider/internal/controllers/common/option" + "github.com/krateoplatformops/git-provider/internal/controllers/common/templating" prettylog "github.com/krateoplatformops/plumbing/slogs/pretty" commonv1 "github.com/krateoplatformops/provider-runtime/apis/common/v1" "github.com/krateoplatformops/provider-runtime/pkg/controller" @@ -772,8 +773,11 @@ func TestController(t *testing.T) { setup: func(ctx context.Context, t *testing.T, r *resources.Resources) { createGiteaRepo(t, "src-tc09", "main") createGiteaRepo(t, "dst-tc09", "main") + // The repo's own placeholder uses the default delimiters; the `{{ }}` line is + // Helm's and must survive untouched — that coexistence is the point of the + // delimiter choice, and it is only observable end to end. commitFilesToRepo(t, "src-tc09", "main", "seed source", map[string]string{ - "content/template.txt": "Hello {{ .name }}!\n", + "content/template.txt": "Hello {% .name %}!\nreplicas: {{ .Values.replicas }}\n", }) require.NoError(t, r.Create(ctx, &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "tc09-values", Namespace: namespace}, @@ -794,7 +798,8 @@ func TestController(t *testing.T) { verify: func(ctx context.Context, t *testing.T, r *resources.Resources, repoName string) { _, err := waitForRepoCondition(ctx, r, repoName, commonv1.TypeReady, metav1.ConditionTrue, 90*time.Second) require.NoError(t, err) - require.Equal(t, "Hello GoTemplate!\n", readRemoteFile(t, "dst-tc09", "main", "content/template.txt")) + require.Equal(t, "Hello GoTemplate!\nreplicas: {{ .Values.replicas }}\n", + readRemoteFile(t, "dst-tc09", "main", "content/template.txt")) }, }, { @@ -843,6 +848,38 @@ func TestController(t *testing.T) { require.Equal(t, "arrived later\n", readRemoteFile(t, "dst-tc10", "main", "content/new-after.txt")) }, }, + { + name: "TC11-GoTemplateDelimsAnnotation", + setup: func(ctx context.Context, t *testing.T, r *resources.Resources) { + createGiteaRepo(t, "src-tc11", "main") + createGiteaRepo(t, "dst-tc11", "main") + // Opting back in to Go's native delimiters, for content known not to be a chart. + commitFilesToRepo(t, "src-tc11", "main", "seed source", map[string]string{ + "content/template.txt": "Hello {{ .name }}!\n", + }) + require.NoError(t, r.Create(ctx, &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "tc11-values", Namespace: namespace}, + Data: map[string]string{ + "values": `{"name":"Delims"}`, + }, + })) + }, + repo: func() *repov1alpha1.Repo { + repo := newRepoResource("tc11-delims", "src-tc11", "main", "dst-tc11", "main") + repo.ObjectMeta.Annotations = map[string]string{ + AnnotationTemplatingEngine: "gotemplate", + templating.DelimsAnnotation: "{{,}}", + } + repo.Spec.Override = true + repo.Spec.ConfigMapKeyRef = configMapSelector("tc11-values", "values") + return repo + }(), + verify: func(ctx context.Context, t *testing.T, r *resources.Resources, repoName string) { + _, err := waitForRepoCondition(ctx, r, repoName, commonv1.TypeReady, metav1.ConditionTrue, 90*time.Second) + require.NoError(t, err) + require.Equal(t, "Hello Delims!\n", readRemoteFile(t, "dst-tc11", "main", "content/template.txt")) + }, + }, { name: "TC11-DisableUpdateOverrideTrue", setup: func(ctx context.Context, t *testing.T, r *resources.Resources) {