From f99a080e4dbdaacc9b4a43f181ee4c63402ce084 Mon Sep 17 00:00:00 2001 From: Vladyslav Plishchenko <58120045+geekhubuser@users.noreply.github.com> Date: Wed, 29 Jul 2026 12:34:18 +0300 Subject: [PATCH] EPMDEDP-17242: fix: generate the scaffold chart README instead of hand-writing it The README of the scaffolded application chart was a hand-transcribed copy of what helm-docs generates from values.tmpl. Adding the HTTPRoute exposure updated the values but transcribed the table wrongly, so on Envoy clusters every scaffolded project shipped a README helm-docs disagreed with, failing the helm-docs task of its review pipeline regardless of language. - Generate README.tmpl from the rendered chart with helm-docs, so the operator ships exactly what the review pipeline verifies and the table can no longer drift from the values next to it - Guard it in CI: make validate-scaffold-docs regenerates the templates and fails when they are stale - Document the parent Gateway per field and expose sectionName, which the collapsed map hid - Close the chart files the scaffold writes; the leaked handles broke test cleanup on Windows --- .github/workflows/pr.yaml | 19 ++ Makefile | 13 +- .../helm-chart/kubernetes/README.tmpl | 18 +- .../helm-chart/kubernetes/values.tmpl | 8 +- hack/scaffolddocs/main.go | 228 ++++++++++++++++++ hack/scaffolddocs/main_test.go | 161 +++++++++++++ hack/scaffolddocs/merge.go | 187 ++++++++++++++ hack/scaffolddocs/merge_test.go | 190 +++++++++++++++ pkg/util/template.go | 6 + pkg/util/template_test.go | 126 ++++++++++ 10 files changed, 944 insertions(+), 12 deletions(-) create mode 100644 hack/scaffolddocs/main.go create mode 100644 hack/scaffolddocs/main_test.go create mode 100644 hack/scaffolddocs/merge.go create mode 100644 hack/scaffolddocs/merge_test.go diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index 4c404889..6a733e1f 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -55,6 +55,25 @@ jobs: - name: "run tests" run: make build && make test + scaffold-docs: + name: Validate scaffold docs + runs-on: ubuntu-latest + + steps: + - name: Check out code + uses: actions/checkout@v6 + + - name: Set up Go + uses: actions/setup-go@v6 + with: + go-version: "1.25" + + # Regenerates the README.tmpl of the application chart scaffold and fails when it differs + # from the committed one. A stale template fails the helm-docs task in the review pipeline + # of every codebase scaffolded from it. + - name: Validate scaffold docs + run: make validate-scaffold-docs + helm-lint: name: Validate Helm chart runs-on: ubuntu-latest diff --git a/Makefile b/Makefile index 571ec5cd..75ef37cd 100644 --- a/Makefile +++ b/Makefile @@ -80,10 +80,18 @@ generate: controller-gen api-docs ## Generate code containing DeepCopy, DeepCopy $(CONTROLLER_GEN) object:headerFile="hack/boilerplate.go.txt" paths="./..." .PHONY: validate-docs -validate-docs: api-docs helm-docs ## Validate helm and api docs +validate-docs: api-docs helm-docs validate-scaffold-docs ## Validate helm and api docs @git diff -s --exit-code deploy-templates/README.md || (echo "Run 'make helm-docs' to address the issue." && git diff && exit 1) @git diff -s --exit-code docs/api.md || (echo " Run 'make api-docs' to address the issue." && git diff && exit 1) +.PHONY: scaffold-docs +scaffold-docs: helmdocs ## Generate the README.tmpl of the application Helm chart scaffold + go run ./hack/scaffolddocs --helm-docs $(HELMDOCS) + +.PHONY: validate-scaffold-docs +validate-scaffold-docs: scaffold-docs ## Validate the scaffold docs are up to date with its values + @git diff -s --exit-code build/templates || (echo "Run 'make scaffold-docs' to address the issue." && git diff build/templates && exit 1) + # Run tests test: fmt vet setup-envtest KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" \ @@ -148,6 +156,9 @@ ENVTEST_VERSION ?= $(shell go list -m -f "{{ .Version }}" sigs.k8s.io/controller ENVTEST_K8S_VERSION ?= $(shell go list -m -f "{{ .Version }}" k8s.io/api | awk -F'[v.]' '{printf "1.%d", $$3}') GOLANGCI_LINT_VERSION ?= v2.8.0 MOCKERY_VERSION ?= v3.6.2 +# Also generates the application chart scaffold docs (scaffold-docs). Those are re-validated by +# the helm-docs task of edp-tekton's review pipelines, so this has to keep producing the same +# output as the helm_docs_image pinned there - bump the two together. HELMDOCS_VERSION ?= v1.14.2 GITCHGLOG_VERSION ?= v0.15.4 CRDOC_VERSION ?= v0.6.4 diff --git a/build/templates/applications/helm-chart/kubernetes/README.tmpl b/build/templates/applications/helm-chart/kubernetes/README.tmpl index 9e55ed36..c1b9600c 100644 --- a/build/templates/applications/helm-chart/kubernetes/README.tmpl +++ b/build/templates/applications/helm-chart/kubernetes/README.tmpl @@ -26,21 +26,23 @@ A Helm chart for Kubernetes | autoscaling.minReplicas | int | `1` | | | autoscaling.targetCPUUtilizationPercentage | int | `80` | | | fullnameOverride | string | `""` | | -| image.digest | string | `""` | Image digest for immutable reference (e.g., sha256:abc123...). If set, deployed as :tag@digest. | -| image.pullPolicy | string | `"IfNotPresent"` | | -| image.repository | string | `"{{.Name}}"` | | -| image.tag | string | `""` | Overrides the image tag whose default is the chart appVersion. | -| imagePullSecrets[0].name | string | `"regcred"` | | {{- if eq .IngressController "envoy" }} | httproute.annotations | object | `{}` | | | httproute.dnsWildcard | string | `"{{.DnsWildcard}}"` | | | httproute.enabled | bool | `false` | | -| httproute.gateway.name | string | `"{{.GatewayName}}"` | | -| httproute.gateway.namespace | string | `"{{.GatewayNamespace}}"` | | +| httproute.gateway.name | string | `"{{.GatewayName}}"` | Name of the parent Gateway. | +| httproute.gateway.namespace | string | `"{{.GatewayNamespace}}"` | Namespace of the parent Gateway. | +| httproute.gateway.sectionName | string | `""` | Gateway listener to bind to, e.g. an HTTPS listener that terminates TLS. Empty attaches to every listener that accepts the route. | | httproute.hosts[0].host | string | `"edpDefault"` | | | httproute.hosts[0].paths[0].path | string | `"/"` | | | httproute.hosts[0].paths[0].pathType | string | `"PathPrefix"` | | -{{- else }} +{{- end }} +| image.digest | string | `""` | Image digest for immutable reference (e.g., sha256:abc123...). If set, deployed as :tag@digest. | +| image.pullPolicy | string | `"IfNotPresent"` | | +| image.repository | string | `"{{.Name}}"` | | +| image.tag | string | `""` | Overrides the image tag whose default is the chart appVersion. | +| imagePullSecrets[0].name | string | `"regcred"` | | +{{- if ne .IngressController "envoy" }} | ingress.annotations | object | `{}` | | | ingress.className | string | `""` | | | ingress.dnsWildcard | string | `"{{.DnsWildcard}}"` | | diff --git a/build/templates/applications/helm-chart/kubernetes/values.tmpl b/build/templates/applications/helm-chart/kubernetes/values.tmpl index 11fded9c..5ca89618 100644 --- a/build/templates/applications/helm-chart/kubernetes/values.tmpl +++ b/build/templates/applications/helm-chart/kubernetes/values.tmpl @@ -51,12 +51,14 @@ service: httproute: enabled: false dnsWildcard: {{.DnsWildcard}} - # -- Parent Gateway the HTTPRoute attaches to. Defaults to the platform gateway. + # Parent Gateway the HTTPRoute attaches to. Defaults to the platform gateway. gateway: + # -- Name of the parent Gateway. name: {{.GatewayName}} + # -- Namespace of the parent Gateway. namespace: {{.GatewayNamespace}} - # -- Optional Gateway listener to bind to (e.g. an HTTPS listener that terminates TLS). - # sectionName: "" + # -- Gateway listener to bind to, e.g. an HTTPS listener that terminates TLS. Empty attaches to every listener that accepts the route. + sectionName: "" # Each entry below is rendered as its own HTTPRoute, so paths stay scoped to their host. # 'edpDefault' expands to -.; set a literal host to override. hosts: diff --git a/hack/scaffolddocs/main.go b/hack/scaffolddocs/main.go new file mode 100644 index 00000000..916c3bfa --- /dev/null +++ b/hack/scaffolddocs/main.go @@ -0,0 +1,228 @@ +// Command scaffolddocs regenerates the README.tmpl files shipped with the application Helm +// chart scaffold. +// +// The templates are not hand-written. Each one is helm-docs output for the chart the operator +// actually scaffolds, with the codebase-specific values substituted back as Go template +// actions. Hand-transcribing that table is what let the documentation drift away from +// values.tmpl, which in turn failed the helm-docs task of every review pipeline. +// +// Run it through `make scaffold-docs`; `make validate-scaffold-docs` fails when the committed +// templates are stale. +package main + +import ( + "bytes" + "context" + "flag" + "fmt" + "io/fs" + "os" + "os/exec" + "path/filepath" + "strings" + + "github.com/go-logr/logr" + ctrl "sigs.k8s.io/controller-runtime" + + "github.com/epam/edp-codebase-operator/v2/pkg/model" + "github.com/epam/edp-codebase-operator/v2/pkg/platform" + "github.com/epam/edp-codebase-operator/v2/pkg/util" +) + +// Values the scaffold is rendered with so helm-docs sees a syntactically valid chart. They are +// turned back into template actions afterwards, so none of them must be a prefix of another. +const ( + nameValue = "scaffold-codebase-name" + dnsWildcardValue = "scaffold-dns-wildcard" + gitURLValue = "scaffold-git-url" + frameworkValue = "scaffold-framework" + gatewayNameValue = "scaffold-gateway-name" + gatewayNamespaceValue = "scaffold-gateway-ns" +) + +const chartDirName = "deploy-templates" + +// placeholder pairs a value the operator substitutes at scaffold time with the template action +// that produces it. +type placeholder struct { + value string + action string +} + +func placeholders() []placeholder { + return []placeholder{ + {value: nameValue, action: "{{.Name}}"}, + {value: dnsWildcardValue, action: "{{.DnsWildcard}}"}, + {value: gitURLValue, action: "{{.GitURL}}"}, + {value: frameworkValue, action: "{{.Framework}}"}, + {value: gatewayNameValue, action: "{{.GatewayName}}"}, + {value: gatewayNamespaceValue, action: "{{.GatewayNamespace}}"}, + } +} + +// chartVariant is one rendering of a scaffold. Variants differ only in which values they expose, +// so their tables are merged back into a single template in which each variant-specific run of +// rows is wrapped in the guard that selects it. +type chartVariant struct { + ingressController string + guard string +} + +// chartPlatforms are the scaffold directories under build/templates/applications/helm-chart. +func chartPlatforms() []string { + return []string{platform.K8S, platform.Openshift} +} + +// chartVariants are rendered for every platform, because the ingress controller is selected +// independently of the platform. A platform whose scaffold ignores the selection renders both +// variants identically, every row is then documented by both, and the merged template comes out +// unguarded - so covering a platform that does not branch costs one helm-docs run and nothing +// else, while a platform that starts branching is documented correctly without touching this. +func chartVariants() []chartVariant { + return []chartVariant{ + { + ingressController: platform.IngressControllerEnvoy, + guard: fmt.Sprintf(`{{- if eq .IngressController %q }}`, platform.IngressControllerEnvoy), + }, + { + ingressController: platform.IngressControllerNginx, + guard: fmt.Sprintf(`{{- if ne .IngressController %q }}`, platform.IngressControllerEnvoy), + }, + } +} + +func main() { + helmDocs := flag.String("helm-docs", filepath.Join("bin", "helm-docs"), "path to the helm-docs binary") + assets := flag.String("assets", "build", "directory holding the scaffold templates") + + flag.Parse() + + if err := run(*helmDocs, *assets); err != nil { + fmt.Fprintf(os.Stderr, "scaffolddocs: %v\n", err) + os.Exit(1) + } +} + +func run(helmDocs, assets string) error { + variants := chartVariants() + + for _, platformType := range chartPlatforms() { + documented := make([]readme, 0, len(variants)) + + for _, v := range variants { + doc, err := documentVariant(helmDocs, assets, platformType, v) + if err != nil { + return fmt.Errorf("failed to document %s/%s: %w", platformType, v.ingressController, err) + } + + documented = append(documented, doc) + } + + merged, err := mergeReadmes(documented) + if err != nil { + return fmt.Errorf("failed to merge %s documentation: %w", platformType, err) + } + + dest := filepath.Join( + assets, "templates", "applications", util.HelmChartDeploymentScriptType, platformType, util.ReadmeTemplate, + ) + + if err = os.WriteFile(dest, []byte(merged), 0o644); err != nil { + return fmt.Errorf("failed to write %q: %w", dest, err) + } + + fmt.Println("generated", dest) + } + + return nil +} + +// documentVariant renders one variant of the scaffold and runs helm-docs over the result. +func documentVariant(helmDocs, assets, platformType string, v chartVariant) (readme, error) { + workDir, err := os.MkdirTemp("", "scaffolddocs-") + if err != nil { + return readme{}, fmt.Errorf("failed to create work directory: %w", err) + } + + defer func() { _ = os.RemoveAll(workDir) }() + + // Render through the operator's own code path, so the documentation cannot describe a + // chart the operator would not produce. + ctx := ctrl.LoggerInto(context.Background(), logr.Discard()) + + err = util.CopyTemplate(ctx, util.HelmChartDeploymentScriptType, workDir, assets, config(platformType, v)) + if err != nil { + return readme{}, fmt.Errorf("failed to render scaffold: %w", err) + } + + if err = normalizeLineEndings(filepath.Join(workDir, chartDirName)); err != nil { + return readme{}, err + } + + // Invoked exactly like the review pipeline invokes it: from the repository root, against a + // relative chart directory. helm-docs overwrites the README.md that CopyTemplate rendered + // from the template being regenerated here, so the previous content cannot leak into it. + cmd := exec.Command(helmDocs, "--chart-search-root", chartDirName) + cmd.Dir = workDir + + if out, cmdErr := cmd.CombinedOutput(); cmdErr != nil { + return readme{}, fmt.Errorf("failed to run helm-docs: %w: %s", cmdErr, out) + } + + generated, err := os.ReadFile(filepath.Join(workDir, chartDirName, "README.md")) + if err != nil { + return readme{}, fmt.Errorf("failed to read generated documentation: %w", err) + } + + return parseReadme(restorePlaceholders(strings.ReplaceAll(string(generated), "\r\n", "\n")), v.guard) +} + +func config(platformType string, v chartVariant) *model.ConfigGoTemplating { + return &model.ConfigGoTemplating{ + Name: nameValue, + PlatformType: platformType, + DnsWildcard: dnsWildcardValue, + GitURL: gitURLValue, + Framework: frameworkValue, + IngressController: v.ingressController, + GatewayName: gatewayNameValue, + GatewayNamespace: gatewayNamespaceValue, + } +} + +// restorePlaceholders turns the rendered values back into the template actions that produced +// them, so the generated file is a template again. +func restorePlaceholders(doc string) string { + for _, p := range placeholders() { + doc = strings.ReplaceAll(doc, p.value, p.action) + } + + return doc +} + +// normalizeLineEndings rewrites the rendered chart with LF endings. A Git checkout on Windows +// carries CRLF, which the markdown post-processing of helm-docs does not recognise, leaving +// blank runs and trailing spaces that make the generated template differ per platform. +func normalizeLineEndings(root string) error { + return filepath.WalkDir(root, func(path string, entry fs.DirEntry, err error) error { + if err != nil || entry.IsDir() { + return err + } + + content, err := os.ReadFile(path) + if err != nil { + return fmt.Errorf("failed to read %q: %w", path, err) + } + + normalized := bytes.ReplaceAll(content, []byte("\r\n"), []byte("\n")) + if bytes.Equal(normalized, content) { + return nil + } + + if err = os.WriteFile(path, normalized, 0o644); err != nil { + return fmt.Errorf("failed to write %q: %w", path, err) + } + + return nil + }) +} diff --git a/hack/scaffolddocs/main_test.go b/hack/scaffolddocs/main_test.go new file mode 100644 index 00000000..f418d1a7 --- /dev/null +++ b/hack/scaffolddocs/main_test.go @@ -0,0 +1,161 @@ +package main + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/epam/edp-codebase-operator/v2/pkg/platform" + "github.com/epam/edp-codebase-operator/v2/pkg/util" +) + +// fieldAction matches a Go template action that substitutes a single config field. +var fieldAction = regexp.MustCompile(`\{\{\s*\.([A-Za-z]+)\s*\}\}`) + +// TestPlaceholders_CoverEveryTemplateAction fails when a scaffold template gains a field the +// generator does not supply. Such a field renders empty, which silently drops the row +// documenting it from the generated documentation instead of failing the generator. +func TestPlaceholders_CoverEveryTemplateAction(t *testing.T) { + t.Parallel() + + // Scoped to the scaffold the generator renders: the other deployment scripts are templated + // with a different config and are none of its business. + templates, err := filepath.Glob(filepath.Join( + "..", "..", "build", "templates", "applications", util.HelmChartDeploymentScriptType, "*", "*.tmpl", + )) + require.NoError(t, err) + require.NotEmpty(t, templates) + + supplied := make(map[string]bool, len(placeholders())) + + for _, p := range placeholders() { + field := fieldAction.FindStringSubmatch(p.action) + require.Len(t, field, 2, "placeholder action %q does not substitute a field", p.action) + + supplied[field[1]] = true + } + + for _, path := range templates { + // The README template is the generator's own output, described by the others. + if filepath.Base(path) == util.ReadmeTemplate { + continue + } + + content, err := os.ReadFile(path) + require.NoError(t, err) + + for _, match := range fieldAction.FindAllStringSubmatch(string(content), -1) { + assert.True(t, supplied[match[1]], + "%s substitutes {{.%s}}, which no placeholder supplies", path, match[1]) + } + } +} + +func TestPlaceholders_AreSubstitutableIndependently(t *testing.T) { + t.Parallel() + + // Placeholders are substituted one after another over the whole document, so a placeholder + // contained in another one would corrupt the longer one. + for i, outer := range placeholders() { + for j, inner := range placeholders() { + if i == j { + continue + } + + assert.NotContains(t, outer.value, inner.value, + "placeholder %q contains %q", outer.value, inner.value) + } + } +} + +func TestRestorePlaceholders(t *testing.T) { + t.Parallel() + + restored := restorePlaceholders(strings.Join([]string{ + "# " + nameValue, + "**Homepage:** <" + gitURLValue + ">", + "| httproute.dnsWildcard | string | `\"" + dnsWildcardValue + "\"` | |", + "| httproute.gateway.name | string | `\"" + gatewayNameValue + "\"` | |", + "| httproute.gateway.namespace | string | `\"" + gatewayNamespaceValue + "\"` | |", + "| framework | string | `\"" + frameworkValue + "\"` | |", + }, "\n")) + + assert.NotContains(t, restored, "scaffold-", "every placeholder must be turned back into an action") + assert.Contains(t, restored, "# {{.Name}}") + assert.Contains(t, restored, "<{{.GitURL}}>") + assert.Contains(t, restored, "`\"{{.DnsWildcard}}\"`") + assert.Contains(t, restored, "`\"{{.GatewayName}}\"`") + assert.Contains(t, restored, "`\"{{.GatewayNamespace}}\"`") + assert.Contains(t, restored, "`\"{{.Framework}}\"`") +} + +func TestConfig_FillsEveryPlaceholder(t *testing.T) { + t.Parallel() + + // Every placeholder has to be supplied to the scaffold, otherwise its values render empty + // and the rows documenting them silently disappear from the generated template. + cfg := config(platform.K8S, chartVariants()[0]) + + supplied := []string{ + cfg.Name, cfg.DnsWildcard, cfg.GitURL, cfg.Framework, cfg.GatewayName, cfg.GatewayNamespace, + } + + for _, p := range placeholders() { + assert.Contains(t, supplied, p.value, "placeholder %q is not supplied by config()", p.value) + } +} + +// TestChartVariants_CoverBothControllersWithDistinctGuards keeps the generator's variants a +// partition of what the scaffold can branch on. A controller left out would be documented by +// whichever variant did run, silently, because the generated template would still be exactly +// what the generator produces and the CI check compares against that. +func TestChartVariants_CoverBothControllersWithDistinctGuards(t *testing.T) { + t.Parallel() + + variants := chartVariants() + require.Len(t, variants, 2, "both ingress controllers have to be rendered") + + controllers := make([]string, 0, len(variants)) + guards := make(map[string]bool, len(variants)) + + for _, v := range variants { + require.NotEmpty(t, v.guard, "variant %q is unguarded", v.ingressController) + require.False(t, guards[v.guard], "guard %q is reused", v.guard) + + controllers = append(controllers, v.ingressController) + guards[v.guard] = true + } + + assert.Contains(t, controllers, platform.IngressControllerEnvoy) + assert.Contains(t, controllers, platform.IngressControllerNginx) +} + +// TestChartPlatforms_CoverEveryScaffoldDirectory fails when a scaffold directory is added without +// registering it, which would otherwise leave its README.tmpl hand-maintained and unvalidated - +// exactly the state this generator exists to remove. +func TestChartPlatforms_CoverEveryScaffoldDirectory(t *testing.T) { + t.Parallel() + + scaffolds, err := filepath.Glob(filepath.Join( + "..", "..", "build", "templates", "applications", util.HelmChartDeploymentScriptType, "*", + )) + require.NoError(t, err) + require.NotEmpty(t, scaffolds) + + for _, path := range scaffolds { + info, err := os.Stat(path) + require.NoError(t, err) + + if !info.IsDir() { + continue + } + + assert.Contains(t, chartPlatforms(), filepath.Base(path), + "scaffold %q is not registered, so its documentation is never generated", path) + } +} diff --git a/hack/scaffolddocs/merge.go b/hack/scaffolddocs/merge.go new file mode 100644 index 00000000..03c03f36 --- /dev/null +++ b/hack/scaffolddocs/merge.go @@ -0,0 +1,187 @@ +package main + +import ( + "errors" + "fmt" + "slices" + "strings" +) + +const ( + valuesHeading = "## Values" + tableSeparator = "|--" + tableRowPrefix = "|" + guardEnd = "{{- end }}" + rowKeyFieldCount = 3 +) + +// readme is a generated chart README split at its values table, so the tables of sibling +// variants can be merged while the prose around them is asserted to be identical. +type readme struct { + guard string + header []string + rows []string + footer []string +} + +// parseReadme splits generated documentation into the values table and its surroundings. +func parseReadme(doc, guard string) (readme, error) { + lines := strings.Split(doc, "\n") + + heading := slices.IndexFunc(lines, func(line string) bool { + return strings.TrimSpace(line) == valuesHeading + }) + if heading < 0 { + return readme{}, fmt.Errorf("section %q not found in generated documentation", valuesHeading) + } + + offset := slices.IndexFunc(lines[heading:], func(line string) bool { + return strings.HasPrefix(line, tableSeparator) + }) + if offset < 0 { + return readme{}, errors.New("values table separator not found in generated documentation") + } + + start := heading + offset + 1 + + end := start + for end < len(lines) && strings.HasPrefix(lines[end], tableRowPrefix) { + end++ + } + + if start == end { + return readme{}, errors.New("values table has no rows") + } + + return readme{ + guard: guard, + header: lines[:start], + rows: lines[start:end], + footer: lines[end:], + }, nil +} + +// mergeReadmes folds the variants of one platform into a single template. +func mergeReadmes(variants []readme) (string, error) { + if len(variants) == 0 { + return "", errors.New("no variants to merge") + } + + base := variants[0] + + for _, v := range variants[1:] { + if !slices.Equal(v.header, base.header) || !slices.Equal(v.footer, base.footer) { + return "", errors.New("variants differ outside the values table, which the generator cannot guard") + } + } + + rows, err := mergeRows(variants) + if err != nil { + return "", err + } + + merged := make([]string, 0, len(base.header)+len(rows)+len(base.footer)) + merged = append(merged, base.header...) + merged = append(merged, rows...) + merged = append(merged, base.footer...) + + return strings.Join(merged, "\n"), nil +} + +// mergedRow is a table row together with the variants that document it. +type mergedRow struct { + key string + line string + present []bool +} + +// mergeRows builds the union of the variants' tables, keeping the alphanumeric order helm-docs +// sorts by so that every variant's own table stays exactly what helm-docs would generate. +func mergeRows(variants []readme) ([]string, error) { + unique := make([]*mergedRow, 0, len(variants[0].rows)) + byLine := make(map[string]*mergedRow, len(variants[0].rows)) + + for i, v := range variants { + for _, line := range v.rows { + row, ok := byLine[line] + if !ok { + row = &mergedRow{key: rowKey(line), line: line, present: make([]bool, len(variants))} + byLine[line] = row + unique = append(unique, row) + } + + row.present[i] = true + } + } + + // Stable, so rows that share a key keep the order of the variants that declare them. + slices.SortStableFunc(unique, func(a, b *mergedRow) int { return strings.Compare(a.key, b.key) }) + + return guardRows(unique, variants) +} + +// guardRows wraps each run of variant-specific rows in the guard that selects it. +func guardRows(rows []*mergedRow, variants []readme) ([]string, error) { + guarded := make([]string, 0, len(rows)) + open := "" + + for _, row := range rows { + guard, err := rowGuard(row, variants) + if err != nil { + return nil, err + } + + if guard != open { + if open != "" { + guarded = append(guarded, guardEnd) + } + + if guard != "" { + guarded = append(guarded, guard) + } + + open = guard + } + + guarded = append(guarded, row.line) + } + + if open != "" { + guarded = append(guarded, guardEnd) + } + + return guarded, nil +} + +func rowGuard(row *mergedRow, variants []readme) (string, error) { + owners := make([]int, 0, len(variants)) + + for i, present := range row.present { + if present { + owners = append(owners, i) + } + } + + if len(owners) == len(variants) { + return "", nil + } + + if len(owners) == 1 { + return variants[owners[0]].guard, nil + } + + return "", fmt.Errorf( + "row %q is documented by %d of %d variants, which needs a combined guard", + row.key, len(owners), len(variants), + ) +} + +// rowKey returns the value path a table row documents, which is what helm-docs sorts by. +func rowKey(line string) string { + fields := strings.SplitN(line, "|", rowKeyFieldCount) + if len(fields) < rowKeyFieldCount { + return line + } + + return strings.TrimSpace(fields[1]) +} diff --git a/hack/scaffolddocs/merge_test.go b/hack/scaffolddocs/merge_test.go new file mode 100644 index 00000000..cef58fc2 --- /dev/null +++ b/hack/scaffolddocs/merge_test.go @@ -0,0 +1,190 @@ +package main + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + envoyGuard = `{{- if eq .IngressController "envoy" }}` + nginxGuard = `{{- if ne .IngressController "envoy" }}` +) + +// doc builds generated documentation with the given value rows. +func doc(rows ...string) string { + return strings.Join(append([]string{ + "# chart", + "", + "## Values", + "", + "| Key | Type | Default | Description |", + "|-----|------|---------|-------------|", + }, append(rows, "")...), "\n") +} + +func TestParseReadme(t *testing.T) { + t.Parallel() + + parsed, err := parseReadme(doc("| a | string | `\"1\"` | |", "| b | int | `2` | |"), envoyGuard) + require.NoError(t, err) + + assert.Equal(t, envoyGuard, parsed.guard) + assert.Equal(t, []string{"| a | string | `\"1\"` | |", "| b | int | `2` | |"}, parsed.rows) + assert.Equal(t, "# chart", parsed.header[0]) + assert.Equal(t, []string{""}, parsed.footer) +} + +func TestParseReadme_Invalid(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + doc string + wantErr string + }{ + { + name: "no values section", + doc: "# chart\n\nno values here\n", + wantErr: "not found in generated documentation", + }, + { + name: "no rows", + doc: doc(), + wantErr: "values table has no rows", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + _, err := parseReadme(tt.doc, envoyGuard) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} + +func TestMergeReadmes_GuardsVariantSpecificRows(t *testing.T) { + t.Parallel() + + // Rows exclusive to one variant must end up guarded, and the merged table must stay in the + // alphanumeric order helm-docs sorts by - that ordering is what the drift was about. + envoy, err := parseReadme(doc( + "| fullnameOverride | string | `\"\"` | |", + "| httproute.enabled | bool | `false` | |", + "| image.tag | string | `\"\"` | |", + ), envoyGuard) + require.NoError(t, err) + + nginx, err := parseReadme(doc( + "| fullnameOverride | string | `\"\"` | |", + "| image.tag | string | `\"\"` | |", + "| ingress.enabled | bool | `false` | |", + ), nginxGuard) + require.NoError(t, err) + + merged, err := mergeReadmes([]readme{envoy, nginx}) + require.NoError(t, err) + + assert.Contains(t, merged, strings.Join([]string{ + "| fullnameOverride | string | `\"\"` | |", + envoyGuard, + "| httproute.enabled | bool | `false` | |", + guardEnd, + "| image.tag | string | `\"\"` | |", + nginxGuard, + "| ingress.enabled | bool | `false` | |", + guardEnd, + }, "\n")) +} + +func TestMergeReadmes_SingleVariantIsUnguarded(t *testing.T) { + t.Parallel() + + only, err := parseReadme(doc("| replicaCount | int | `1` | |"), "") + require.NoError(t, err) + + merged, err := mergeReadmes([]readme{only}) + require.NoError(t, err) + + assert.NotContains(t, merged, guardEnd) + assert.Contains(t, merged, "| replicaCount | int | `1` | |") +} + +func TestMergeReadmes_SameKeyDifferentValue(t *testing.T) { + t.Parallel() + + // A key both variants document but with different content has to keep one row per variant, + // each behind its own guard, so exactly one of them renders. + envoy, err := parseReadme(doc("| service.port | int | `8080` | |"), envoyGuard) + require.NoError(t, err) + + nginx, err := parseReadme(doc("| service.port | int | `80` | |"), nginxGuard) + require.NoError(t, err) + + merged, err := mergeReadmes([]readme{envoy, nginx}) + require.NoError(t, err) + + assert.Contains(t, merged, strings.Join([]string{ + envoyGuard, + "| service.port | int | `8080` | |", + guardEnd, + nginxGuard, + "| service.port | int | `80` | |", + guardEnd, + }, "\n")) +} + +func TestMergeReadmes_RejectsDifferencesOutsideTheTable(t *testing.T) { + t.Parallel() + + envoy, err := parseReadme(doc("| a | int | `1` | |"), envoyGuard) + require.NoError(t, err) + + nginx, err := parseReadme(strings.Replace(doc("| a | int | `1` | |"), "# chart", "# other", 1), nginxGuard) + require.NoError(t, err) + + _, err = mergeReadmes([]readme{envoy, nginx}) + require.Error(t, err) + assert.Contains(t, err.Error(), "differ outside the values table") +} + +func TestMergeReadmes_RejectsPartiallySharedRow(t *testing.T) { + t.Parallel() + + // With three variants a row shared by two of them would need an OR of guards, which the + // generator does not emit - it must fail loudly instead of dropping the row. + shared := "| a | int | `1` | |" + + first, err := parseReadme(doc(shared), envoyGuard) + require.NoError(t, err) + + second, err := parseReadme(doc(shared), nginxGuard) + require.NoError(t, err) + + third, err := parseReadme(doc("| b | int | `2` | |"), "{{- if false }}") + require.NoError(t, err) + + _, err = mergeReadmes([]readme{first, second, third}) + require.Error(t, err) + assert.Contains(t, err.Error(), "needs a combined guard") +} + +func TestMergeReadmes_NoVariants(t *testing.T) { + t.Parallel() + + _, err := mergeReadmes(nil) + require.Error(t, err) +} + +func TestRowKey(t *testing.T) { + t.Parallel() + + assert.Equal(t, "image.tag", rowKey("| image.tag | string | `\"\"` | Overrides the tag. |")) + assert.Equal(t, "imagePullSecrets[0].name", rowKey("| imagePullSecrets[0].name | string | `\"regcred\"` | |")) + assert.Equal(t, "not a row", rowKey("not a row")) +} diff --git a/pkg/util/template.go b/pkg/util/template.go index 011d380f..c690074b 100644 --- a/pkg/util/template.go +++ b/pkg/util/template.go @@ -40,6 +40,8 @@ func CopyHelmChartTemplates(deploymentScript, templatesDest, assetsDir string, c return fmt.Errorf("failed to create Values file %q: %w", valuesFileName, err) } + defer CloseWithLogOnErr(log, valuesFile, "failed to close Values file") + log.Info("file is created", fileLogKey, valuesFileName) chartFileName := path.Join(templatesDest, "Chart.yaml") @@ -49,6 +51,8 @@ func CopyHelmChartTemplates(deploymentScript, templatesDest, assetsDir string, c return fmt.Errorf("failed to create chart file %q: %w", chartFileName, err) } + defer CloseWithLogOnErr(log, chartFile, "failed to close chart file") + log.Info("file is created", fileLogKey, chartFileName) readmeFileName := path.Join(templatesDest, "README.md") @@ -58,6 +62,8 @@ func CopyHelmChartTemplates(deploymentScript, templatesDest, assetsDir string, c return fmt.Errorf("failed to create chart file %q: %w", readmeFileName, err) } + defer CloseWithLogOnErr(log, readmeFile, "failed to close README file") + log.Info("file is created", fileLogKey, readmeFileName) templateFolder := path.Join(templatesDest, TemplateFolder) diff --git a/pkg/util/template_test.go b/pkg/util/template_test.go index b10a98d8..cafe3484 100644 --- a/pkg/util/template_test.go +++ b/pkg/util/template_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "strings" "testing" "github.com/go-logr/logr" @@ -133,6 +134,131 @@ func TestCopyTemplate_HelmTemplates_ExposureByController(t *testing.T) { } } +// valueRowKeys returns the value paths documented by a chart README, in the order they appear. +func valueRowKeys(t *testing.T, readme string) []string { + t.Helper() + + lines := strings.Split(readme, "\n") + + // The values table is the only one helm-docs writes without spaces around its separator, + // which keeps this from matching the maintainers table above it. + separator := -1 + + for i, line := range lines { + if strings.HasPrefix(line, "|--") { + separator = i + break + } + } + + require.GreaterOrEqual(t, separator, 0, "values table not found in README") + + keys := make([]string, 0, len(lines)-separator) + + for _, line := range lines[separator+1:] { + if !strings.HasPrefix(line, "|") { + break + } + + keys = append(keys, strings.TrimSpace(strings.SplitN(line, "|", 3)[1])) + } + + require.NotEmpty(t, keys, "values table has no rows") + + return keys +} + +// TestCopyTemplate_HelmTemplates_ReadmeDocumentsValues guards the contract the review pipeline +// of every scaffolded project checks: the README has to be what helm-docs generates from the +// values next to it. helm-docs sorts the table alphanumerically and documents the exposure the +// platform selected, so a README ordered differently, or describing the other exposure, fails +// that pipeline for every codebase created from this scaffold. +func TestCopyTemplate_HelmTemplates_ReadmeDocumentsValues(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + platformType string + ingressController string + wantKeys []string + absentKeys []string + }{ + { + name: "kubernetes with envoy documents the httproute", + platformType: "kubernetes", + ingressController: "envoy", + wantKeys: []string{ + "httproute.enabled", + "httproute.gateway.name", + "httproute.gateway.namespace", + "httproute.gateway.sectionName", + }, + absentKeys: []string{"ingress.enabled", "ingress.className"}, + }, + { + name: "kubernetes with nginx documents the ingress", + platformType: "kubernetes", + ingressController: "nginx", + wantKeys: []string{"ingress.enabled", "ingress.className", "ingress.tls"}, + absentKeys: []string{"httproute.enabled", "httproute.gateway.name"}, + }, + { + // OpenShift exposes applications through a Route and has no exposure variants, so + // the ingress-controller selection must not reach its documentation. + name: "openshift documents its route and never the httproute", + platformType: "openshift", + ingressController: "envoy", + wantKeys: []string{"ingress.enabled", "ingress.path", "ingress.pathType"}, + absentKeys: []string{"httproute.enabled", "httproute.gateway.name"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + testDir := t.TempDir() + + err := CopyTemplate( + ctrl.LoggerInto(context.Background(), logr.Discard()), + HelmChartDeploymentScriptType, + testDir, + "../../build", + &model.ConfigGoTemplating{ + Name: "c-name", + PlatformType: tt.platformType, + Lang: "go", + Framework: "beego", + DnsWildcard: "mydomain.example.com", + GitURL: "https://example.com", + IngressController: tt.ingressController, + GatewayName: "main-gateway", + GatewayNamespace: "envoy-gateway-system", + }, + ) + require.NoError(t, err) + + readme, err := os.ReadFile(fmt.Sprintf("%v/deploy-templates/README.md", testDir)) + require.NoError(t, err) + + keys := valueRowKeys(t, string(readme)) + + for i := 1; i < len(keys); i++ { + require.Less(t, keys[i-1], keys[i], + "README rows must keep the alphanumeric order helm-docs sorts by") + } + + for _, want := range tt.wantKeys { + assert.Contains(t, keys, want) + } + + for _, absent := range tt.absentKeys { + assert.NotContains(t, keys, absent) + } + }) + } +} + func TestCopyTemplate_HelmTemplates_DirectoryExists(t *testing.T) { t.Parallel()