Chore: Set up helmcov and helmdocs for charts.

[jordan-simonovski]

I might consider moving to helmver next instead of changesets and alleviating one more dependency in the repo in future. Open to suggestions.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 5c0c7e6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[CLAassistant]

All committers have signed the CLA.

[github-actions]

Deep Review

✅ No critical issues found. This PR changes no runtime Helm templates (only values.yaml comment reformatting, new tests, scripts, Makefile, and CI). No template fails to render, no secret is introduced, and no default guarantees a broken deployment. The findings below are supply-chain/governance hardening and dev-workflow robustness for the new helmcov/helm-docs tooling, plus test-quality and standards nits.

🟡 P2 — recommended

• .github/workflows/helmcov.yaml:39 — the new PR-triggered workflow runs third-party action jordan-simonovski/helmcov@v1 (mutable major tag, personal namespace) with pull-requests: write, so a retag or account compromise executes attacker code in the trusted same-repo context.
  • Fix: pin the action to a full commit SHA and mirror it into the ClickHouse org; fork-PR tokens are already read-only under the pull_request trigger, so the residual risk is the trusted-context path.
  • _{security, adversarial}
• scripts/tool-versions.env:8 — the entire helmcov coverage gate (CI action and local make coverage image) depends on a personal jordan-simonovski namespace; deletion or visibility change breaks both, as the file's own TODO acknowledges.
  • Fix: mirror the digest-pinned image and the action into ghcr.io/clickhouse before this gate becomes load-bearing.
  • _{maintainability, adversarial, security}
• .github/workflows/helmcov.yaml:39 — the helmcov binary version is pinned independently in two places, the image digest in scripts/tool-versions.env versus the action version: v0.4.0 at @v1, so local and CI runs can silently use different binaries.
  • Fix: drive both from a single source of truth or document that the action's version input matches the pinned digest.
  • _{maintainability}
• .githooks/pre-commit:31 — a contributor without Helm/Docker staging even a trivial charts/ change is hard-rejected, because make test → chart-deps runs command -v helm which exits non-zero under set -e, and the hook also shells into a network helm-docs download.
  • Fix: warn and exit 0 when helm/docker are absent instead of failing the commit.
  • _adversarial
• charts/clickstack/tests/values/deployment-tpl-defaults.yaml:1 — the ~100-line defaultConnections/defaultSources JSON blob is copied verbatim from values.yaml with no sync enforcement, while the test asserts only two regex lines, creating a silent drift trap.
  • Fix: trim the fixture to the minimal connection and log source the two assertions actually exercise.
  • _{maintainability}

🔵 P3 nitpicks (8)

• .githooks/pre-commit:22 — the inner case branches scripts/helmdocs.sh|scripts/install-helm-docs.sh sit inside the outer charts/*) arm and are unreachable, so editing only those scripts never triggers make docs.
  • Fix: evaluate those two patterns in an outer-level case arm independent of charts/*.
• .githooks/pre-commit:30 — editing charts/clickstack-operators/ sets chart_changes=true and runs make test, but make test hardcodes CHART=charts/clickstack, so the operators chart is reported as tested while nothing renders or asserts it.
  • Fix: loop test/validate over all charts, or document that operators is intentionally untested.
• scripts/install-helm-docs.sh:41 — the helm-docs release tarball is fetched over curl -fsSL with no checksum or signature verification and then executed, now reachable in CI via make setup.
  • Fix: verify the download against a pinned SHA256 from the release checksums before install/exec.
• scripts/helmcov.sh:13 — the coverage threshold 30 is duplicated across Makefile, scripts/helmcov.sh, and .github/workflows/helmcov.yaml.
  • Fix: define the threshold once (e.g. in scripts/tool-versions.env) and reference it everywhere.
• AGENTS.md:210 — the new scripts use #!/usr/bin/env bash + set -euo pipefail, diverging from the "Shell Script" conventions (#!/bin/bash, set -e, set -o pipefail) that this PR edits but leaves unchanged.
  • Fix: update the conventions section to permit the new style, or align the new scripts with the documented standard.
• charts/clickstack/tests/additional-manifests-multi_test.yaml:11 — the second rendered manifest (the ConfigMap) is only counted, never asserted, so its tpl rendering is unverified.
  • Fix: add isKind/equal assertions at documentIndex: 1 for the ConfigMap name and data.
• charts/clickstack/tests/deployment-features_test.yaml:17 — matchRegex with the unanchored substring custom-init is weaker than an exact match when the document index already pins the container.
  • Fix: replace with equal value custom-init.
• charts/clickstack/tests/tasks-with-args_test.yaml:9 — the fixture declares two additionalArgs (concurrency, sourceTimeoutMs) but only --concurrency/4 is asserted.
  • Fix: assert --sourceTimeoutMs/90000 too, or drop the unused fixture arg.

---

Reviewers (6): correctness, testing, maintainability, project-standards, adversarial, security.

Testing gaps:

• The README drift gate (helm-test.yaml runs make setup test docs then git diff --exit-code on both chart READMEs) could not be executed here; confirm the committed READMEs were generated by exactly helm-docs v1.14.2 with --badge-style flat --ignore-non-descriptions.
• The new dev/CI tooling (Makefile, scripts/*.sh, .githooks/pre-commit, load-tool-versions.sh) has no automated tests; the unreachable pre-commit branch and operators-not-tested gap would be caught by a small shell test.
• Two low-confidence items were suppressed by the confidence gate: scripts/helmcov.sh forcing --platform linux/amd64 on arm64 dev machines, and the # -- helm-docs convention not applied to the top-level hyperdx: key.

[github-actions]

Helm template coverage

Metric	Value
Line coverage	33.69%
Branch coverage	75.38%
Threshold	30% (met)

Uncovered details

charts/clickstack/templates/NOTES.txt

Uncovered lines: 1, 3, 4, 5, 6, 8, 10, 11, 12, 14, 15, 18, 21, 22, 23, 25, 26, 27, 29, 33, 34, 37

Uncovered branches: none

charts/clickstack/templates/_helpers.tpl

Uncovered lines: 6, 13, 17, 18, 19, 20, 21, 22, 31, 34, 35, 42, 54, 62, 69, 76, 83, 90, 97, 104

Uncovered branches: 12:if:true, 16:if:false, 16:if:true, 30:if:true, 50:if:false

charts/clickstack/templates/additional-manifests.yaml

Uncovered lines: 2

Uncovered branches: none

charts/clickstack/templates/clickhouse/cluster.yaml

Uncovered lines: 2, 3, 4, 6, 8

Uncovered branches: none

charts/clickstack/templates/clickhouse/keeper.yaml

Uncovered lines: 2, 3, 4, 6, 8

Uncovered branches: none

charts/clickstack/templates/hyperdx/configmap.yaml

Uncovered lines: 1, 2, 3, 4, 5, 7, 9

Uncovered branches: 8:range:empty, 8:range:non-empty

charts/clickstack/templates/hyperdx/cronjob-check-alerts.yaml

Uncovered lines: 2, 3, 4, 5, 7, 9, 11, 12, 13, 14, 15, 16, 18, 19, 21, 24, 25, 26, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61

Uncovered branches: 30:if:false, 30:if:true

charts/clickstack/templates/hyperdx/deployment.yaml

Uncovered lines: 1, 2, 3, 5, 11, 13, 17, 18, 21, 22, 23, 26, 30, 32, 34, 36, 38, 40, 42, 44, 47, 50, 52, 54, 56, 58, 62, 65, 66, 68, 71, 72, 75, 76, 78, 80, 83, 84, 85, 87, 88, 89, 95, 97, 98, 99, 105, 107, 110, 111, 112, 114, 115, 117, 119, 120, 121, 124, 125, 126, 127, 130, 131, 133, 135, 137, 139, 140

Uncovered branches: 113:if:false, 132:if:false, 136:if:false, 14:if:false, 28:with:empty, 82:if:true, 9:with:empty

charts/clickstack/templates/hyperdx/hpa.yaml

Uncovered lines: 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18

Uncovered branches: 1:if:true

charts/clickstack/templates/hyperdx/ingress.yaml

Uncovered lines: 5, 6, 7, 9, 12, 13, 14, 16, 18, 20, 21, 22, 23, 25, 28, 29, 30, 31, 32, 34, 36, 37, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 53, 56, 57, 64, 65, 67, 69, 70, 71

Uncovered branches: 11:with:non-empty, 27:if:true, 35:if:true, 38:if:true, 54:range:empty, 58:range:empty, 59:if:false, 66:if:true

charts/clickstack/templates/hyperdx/networkpolicy.yaml

Uncovered lines: 5, 6, 7, 9, 12, 14

Uncovered branches: none

charts/clickstack/templates/hyperdx/pdb.yaml

Uncovered lines: 2, 3, 4, 6, 12, 14, 16, 17

Uncovered branches: 10:with:empty

charts/clickstack/templates/hyperdx/secret.yaml

Uncovered lines: 2, 3, 5, 6, 7, 8, 9, 11, 12, 14, 15

Uncovered branches: 13:range:empty, 13:range:non-empty, 1:if:true, 4:if:false

charts/clickstack/templates/hyperdx/service.yaml

Uncovered lines: 1, 2, 3, 5, 8, 11, 13, 15, 18, 22, 26, 27

Uncovered branches: 9:with:empty

charts/clickstack/templates/hyperdx/serviceaccount.yaml

Uncovered lines: 2, 3, 4, 6, 9, 11

Uncovered branches: none

charts/clickstack/templates/mongodb/community.yaml

Uncovered lines: 2, 3, 4, 6, 8

Uncovered branches: none

charts/clickstack/templates/mongodb/password-secret.yaml

Uncovered lines: 7, 8, 9, 11, 13, 14

Uncovered branches: 6:if:false

[github-actions]

Deep Review

Scope: PR #232 — dev/CI tooling setup (Makefile, helper shell scripts, two CI workflows, a pre-commit hook), helm-docs comment reformatting in values.yaml (comment-only), generated chart README.md files, AGENTS.md updates, and ~40 new helm-unittest test/fixture files. Base a24a014.

Intent: Add helmcov (template coverage) and helm-docs tooling for local dev and CI without changing rendered chart output.

✅ No critical issues found. Rendered chart output is unchanged — the values.yaml diff is comment-only (replicas: 1 and name: "" values are untouched), and the new tests are additive. Nothing in this diff leaks a secret, fails to render, or produces a manifest a cluster would reject, so there are no P0/P1 ship-blockers. The findings below are quality and robustness recommendations on the new tooling.

🟡 P2 — recommended

• .githooks/pre-commit:22 — The scripts/helmdocs.sh|scripts/install-helm-docs.sh patterns are nested inside the inner case under the charts/*) arm, so a scripts/… path never reaches them; editing the doc-generation tooling never sets docs_changes and never regenerates READMEs (caught only later by CI's git diff --exit-code).
  • Fix: Move the scripts/* doc-tooling patterns to a top-level case arm on $path, sibling to charts/*).
  • _{ce-correctness-reviewer, ce-testing-reviewer, ce-adversarial-reviewer}
• .github/workflows/helmcov.yaml:39 — The third-party jordan-simonovski/helmcov@v1 action is pinned to a mutable tag (not a commit SHA) while the job holds pull-requests: write, inconsistent with the digest-pinned HELMCOV_IMAGE; a repointed v1 would run with the write token on same-repo/push events.
  • Fix: Pin the action to a full commit SHA, or run scripts/helmcov.sh directly to drop the third-party action.
  • _{ce-security-reviewer, ce-maintainability-reviewer}
• scripts/install-helm-docs.sh:41 — The helm-docs tarball is fetched over HTTPS and installed as an executable with no checksum or signature verification, then run by make docs/make setup in CI; transport-only TLS does not bind the artifact to a known hash.
  • Fix: Pin an expected SHA256 (the release publishes a checksums file), verify with sha256sum -c, and fail closed on mismatch.
  • _{ce-security-reviewer, ce-adversarial-reviewer}
• scripts/install-hooks.sh:24 — cp overwrites an existing .git/hooks/pre-commit unconditionally; a contributor using husky or the pre-commit framework loses their hook with no backup or warning.
  • Fix: Back up or refuse to overwrite a differing existing hook, or set git config core.hooksPath .githooks instead of copying.
  • _{ce-adversarial-reviewer}
• scripts/load-tool-versions.sh:11 — On a CRLF checkout, each tool-versions.env value retains a trailing \r, so HELM_DOCS_VERSION becomes v1.14.2\r; the version comparison always mismatches (perpetual reinstall) and the download URL embeds %0D and 404s.
  • Fix: Strip the carriage return after read (line="${line%$'\r'}") or add tool-versions.env text eol=lf to .gitattributes.
  • _{ce-adversarial-reviewer}
• .githooks/pre-commit:31 — Any chart-touching commit runs make test, whose chart-deps dependency does helm repo add + helm dependency build (network); an offline or flaky-network commit is rejected for reasons unrelated to the change.
  • Fix: Gate the heavy hook behind an opt-in, skip chart-deps when subcharts are already vendored, or document --no-verify.
  • _{ce-adversarial-reviewer}
• Makefile:7 — Tool versions are split across sources: HELM_UNITTEST_VERSION in the Makefile, HELM_DOCS_VERSION/HELMCOV_IMAGE in tool-versions.env, and the CI action's version: v0.4.0 + threshold: "30" hardcoded separately, so the local digest-pinned image and CI action can drift apart.
  • Fix: Centralize all tool versions in tool-versions.env and drive CI coverage from the same pinned image (make coverage).
  • _{ce-maintainability-reviewer, ce-adversarial-reviewer}
• charts/clickstack/tests/deployment-features_test.yaml:7 — The fixture configures tolerations, topologySpreadConstraints, priorityClassName, annotations, volumes, and volumeMounts, but the test asserts none of them, so those rendering branches are exercised without verification.
  • Fix: Add equal/contains assertions for each configured field.
  • _{ce-testing-reviewer}
• .github/workflows/helm-test.yaml:15 — A paths: filter was added to the pull_request trigger (previously unfiltered); if this job is a required status check, PRs touching no matching path leave the check permanently pending and cannot merge.
  • Fix: Confirm branch-protection required checks; if required, drop the pull_request path filter or add a companion always-passing job.
  • _{ce-maintainability-reviewer}

🔵 P3 nitpicks (8)

• AGENTS.md:99 — The documented HELMCOV_IMAGE=… make coverage env override is silently ignored: include scripts/tool-versions.env assigns the variable in the Makefile, which takes precedence over the environment in GNU Make.
  • Fix: Document the working make coverage HELMCOV_IMAGE=… command-line form, or declare it with ?= so the environment takes effect.
• scripts/helmdocs.sh:1 — Five new scripts and the hook use #!/usr/bin/env bash + set -euo pipefail while AGENTS.md mandates #!/bin/bash + set -e + set -o pipefail, and helmcov.sh follows the older form — inconsistent within the same PR.
  • Fix: Standardize on one form across all scripts and update the AGENTS.md Shell Script Conventions section to match if adopting the stricter form.
• charts/clickstack/tests/deployment-features_test.yaml:18 — initContainers[1].name relies on mongodb defaulting to enabled so wait-for-mongodb sits at index 0; an unrelated default change shifts the index.
  • Fix: Assert via a name filter (initContainers[?(@.name=='custom-init')]) instead of a positional index.
• charts/clickstack/tests/networkpolicy-enabled_test.yaml:11 — The test only asserts the passthrough policyTypes and never covers the template's required-spec fail branch.
  • Fix: Add a negative test (enabled: true, no spec) asserting failedTemplate.
• scripts/helmcov.sh:14 — MAX_SCENARIOS=20 and SEED=42 are overridable but undocumented, and the CI action path does not pass them, so local and CI scenario counts may differ.
  • Fix: Comment their purpose and list them alongside COVERAGE_THRESHOLD in AGENTS.md.
• scripts/install-hooks.sh:22 — The loop copies and chmod +x every entry under .githooks/, so a future helper or README would be installed as an executable hook.
  • Fix: Whitelist known git-hook names or use core.hooksPath.
• .github/workflows/helm-test.yaml:36 — CI runs make setup, which installs git hooks into .git/hooks — pointless work in CI that assumes a writable .git/hooks.
  • Fix: Call the specific targets CI needs (install-helm-unittest install-helm-docs chart-deps) instead of make setup.
• Makefile:65 — The git diff --exit-code README check and its hardcoded two-README path list are duplicated across helm-test.yaml:38, the Makefile ci target, and the pre-commit hook (line 36).
  • Fix: Define the README path list once and have CI call make ci.

---

Reviewers (6): ce-correctness-reviewer, ce-security-reviewer, ce-testing-reviewer, ce-maintainability-reviewer, ce-project-standards-reviewer, ce-adversarial-reviewer.

Testing gaps:

• The dev-tooling shell scripts, Makefile targets, and pre-commit hook have no automated tests; the unreachable case arm and the CRLF parsing defect would be caught by a table-driven test of the hook's path classification and a CRLF-input test for load-tool-versions.sh.
• The 30% coverage threshold (hardcoded in helmcov.sh, Makefile, and helmcov.yaml) is a low gate for ~40 new suites — run helmcov to find the real number and ratchet the floor up rather than leaving most branches able to slip under it.
• No negative/failedTemplate tests exist for the chart's fail guards in hyperdx/ingress.yaml, hyperdx/secret.yaml, and hyperdx/networkpolicy.yaml; the happy paths are covered but the validation branches are not.

[wrn14897]

This is cool, but how good is the test coverage for this library?

[wrn14897]

Following up on this. wdyt about forking or moving this to the ClickHouse repo? I think it's mainly for testing, but I'd imagine we'd still need to go through a security audit of all the dependencies. Having it in the ClickHouse repo would likely make that process easier.

[jordan-simonovski]

Yeah I could do that for both this and the versioning tool I've got if you think it makes sense

[wrn14897]

Thanks for adding the Makefile. Can we make sure it also includes other test commands?