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 d4c7435..f1db2aa 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,102 @@ 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. + +### 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. + +### 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 +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. + + +### When templating fails + +`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: + conditions: + - type: Synced + status: "False" + reason: ReconcileError + 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/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 diff --git a/docs/repo.md b/docs/repo.md index 501b8f1..5e4c3e4 100644 --- a/docs/repo.md +++ b/docs/repo.md @@ -11,6 +11,15 @@ A common use case is **project bootstrapping**. You might have a "golden path" o ## Templating +> **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. + + 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 new file mode 100644 index 0000000..04a5c04 --- /dev/null +++ b/internal/controllers/common/templating/templating.go @@ -0,0 +1,74 @@ +// 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 + +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. + // `{{ 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" +) + +// 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 b2518da..b2e9f8d 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" @@ -388,17 +389,32 @@ 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), - ) + } + + 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 { 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, @@ -455,3 +471,43 @@ 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) { + 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.WithGoTemplateDelims(values, left, right)}, nil + } + return nil, nil + case templating.EngineGoTemplate: + return []copier.Option{copier.WithGoTemplateDelims(values, left, right)}, 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..652ac74 --- /dev/null +++ b/internal/controllers/localresource/templating_options_test.go @@ -0,0 +1,160 @@ +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 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 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 7ba0f65..c5cddf7 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" @@ -22,6 +23,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 +37,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 { @@ -367,21 +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), - }) - } - opts = append(opts, copier.WithGoTemplate(tplVals)) - } 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 { @@ -395,8 +390,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, @@ -455,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/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) { 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 7ec370b..4413567 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: "/", @@ -53,16 +59,27 @@ 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.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 { 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 +89,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 +124,68 @@ 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 +} + +// 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) + } + + // 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: %d problems in %d %s (%s); first: %v", + len(e.Errors), len(files), noun, strings.Join(files, ", "), 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 +233,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 +343,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 +375,42 @@ func (co *Copier) copyFile(src, dst string, doNotRender bool) (err error) { return err } - return co.renderFunc(in, out) + // 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) + } + + 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 } 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 new file mode 100644 index 0000000..55a3003 --- /dev/null +++ b/internal/tools/copier/helm_verbatim_test.go @@ -0,0 +1,112 @@ +package copier + +import ( + "strings" + "testing" + + "github.com/go-git/go-billy/v5/memfs" + tpl "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, 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("/"), + WithGoTemplateDelims(nil, "{{", "}}"), // opted in to Go's delimiters, no values + ) + 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 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: {% .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) + } + + 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) + } + + // 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 !strings.Contains(string(out), "") { + t.Fatalf("premise check: expected , got %q", string(out)) + } +} + +// 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("/"), + WithGoTemplateDelims([]tpl.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) + } +} diff --git a/internal/tools/copier/render_errors_test.go b/internal/tools/copier/render_errors_test.go new file mode 100644 index 0000000..e7d7e1d --- /dev/null +++ b/internal/tools/copier/render_errors_test.go @@ -0,0 +1,120 @@ +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) + } + // 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{} + 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. + if got := len(failed.Files()); got != 2 { + t.Errorf("Files() = %d, want 2 distinct paths", got) + } + + msg := failed.Error() + 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()) != 3 { + t.Errorf("RenderErrors() = %d, want 3", 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()) == 0 { + t.Fatalf("setup: expected at least one error") + } + + // 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/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) + } +} 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: