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: 9b96ad7

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.16%
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 — set up helmcov (template coverage) and helm-docs (README generation): new Makefile, pinned scripts/tool-versions.env, install scripts, a .githooks/pre-commit hook, a CI helmcov job, values.yaml comment conversion to helm-docs # -- format, and ~40 new helm-unittest tests + shared fixtures.

This is a well-executed tooling PR. The new helm-unittest suites are comprehensive and correctly assert against real template output (verified: init-container ordering, service port ordering, version-conditional cronjob command paths, tpl interpolation, disabled-component hasDocuments: 0, ingress document counts). The values.yaml change is comment-only — no config keys were dropped or altered. No P0/P1 issues.

✅ No critical issues found.

🟡 P2 — recommended

• .github/workflows/helm-test.yaml:39 — The helmcov job declares pull-requests: write, but the workflow triggers on pull_request (not pull_request_target), so fork PRs receive a read-only token and the action's PR-comment step can 403; if the action treats that as fatal, external-contributor PRs fail this check.
  • Fix: Guard the comment behavior on github.event.pull_request.head.repo.fork == false, or use an action mode that no-ops when write scope is unavailable, and confirm the check is not marked required for fork PRs.
• scripts/tool-versions.env:9 — helmcov is pinned in three independent places that must be synced by hand: the digest-pinned HELMCOV_IMAGE (local make coverage), the mutable HELMCOV_VERSION=v0.4.0 tag, and the SHA-pinned action in CI — so local and CI coverage can silently compute against different builds.
  • Fix: Drive both paths from one pinned reference (resolve the image digest from HELMCOV_VERSION, or assert in CI that the action's resolved version matches the digest-pinned image).
  • _{security, maintainability, adversarial}
• scripts/install-helm-docs.sh:41 — The release tarball is downloaded and installed to .tools/helm-docs with no checksum or signature verification, so a tampered/replaced GitHub release asset would install an arbitrary binary that then runs locally and in CI.
  • Fix: Download the release checksums file for the pinned HELM_DOCS_VERSION and run sha256sum -c (or verify the release signature) before install.
  • _security
• .githooks/pre-commit:45 — The new stash/restore logic runs on every contributor commit and can strand or conflict-merge uncommitted work, yet has zero automated test coverage.
  • Fix: Add a shell test harness (bats/shunit2) covering staged+unstaged with make passing, make failing (exit-code propagation), and a forced git stash pop conflict.
  • _{testing, adversarial}

🔵 P3 nitpicks (12)

• scripts/install-helm-docs.sh:41 — curl -fsSL has no connect/max timeout or retry, so a stalled GitHub connection can hang the pre-commit hook or CI indefinitely.
  • Fix: Add --connect-timeout 10 --max-time 120 --retry 3 --retry-delay 2.
• .github/workflows/helm-test.yaml:53 — grep ... tool-versions.env >> $GITHUB_ENV reads the PR-head file, so a fork PR can set HELMCOV_VERSION/COVERAGE_THRESHOLD to arbitrary values fed to the action (bounded: read-only token, no secrets — worst case is self-lowering the coverage gate on the attacker's own PR).
  • Fix: Hard-code the version/threshold in the trusted workflow, or pin the action's CLI to a digest.
• .github/workflows/helm-test.yaml:30 — Helm CLI version v3.12.0 is hardcoded in two jobs rather than centralized in tool-versions.env, undercutting the PR's centralized-pinning goal.
  • Fix: Add HELM_VERSION to tool-versions.env and reference it via a workflow env var in both jobs.
• AGENTS.md:23 — States the pre-commit hook runs make docs on chart-doc changes, but the hook actually runs make docs-check (verify-only, reverts regenerated READMEs).
  • Fix: Change the reference to make docs-check to match .githooks/pre-commit.
• scripts/smoke-test.sh:1 — Pre-existing scripts (smoke-test.sh, integration-tests/*.sh) still use #!/bin/bash + set -e + set -o pipefail, now inconsistent with the shell convention this diff rewrote in AGENTS.md, which also cites these files as convention examples.
  • Fix: Update the pre-existing scripts to #!/usr/bin/env bash + set -euo pipefail, or scope the AGENTS.md convention to new scripts only.
• AGENTS.md:175 — The generic bullet "Document all values with inline YAML comments" now duplicates and slightly contradicts the new # -- requirement (helm-docs --ignore-non-descriptions drops plain # comments).
  • Fix: Merge the generic bullet into the # -- guidance.
• .githooks/pre-commit:54 — On a partial commit (overlapping staged+unstaged edits to one region), the restore git stash pop can conflict, blocking a valid commit and leaving conflict markers with a misleading "re-run git stash pop" message.
  • Fix: Detect the conflicted-pop state and emit accurate recovery instructions, or restore via a stash ref that preserves the staged/unstaged split.
• .githooks/pre-commit:53 — git checkout -- . || true in restore() silently swallows a working-tree reset failure before the stash pop.
  • Fix: Log a warning on failure instead of the bare || true.
• .github/workflows/helm-test.yaml:3 — No concurrency group, so rapid pushes spawn overlapping runs whose helmcov jobs race to post/update the same PR coverage comment.
  • Fix: Add concurrency: { group: helm-test-${{ github.ref }}, cancel-in-progress: true }.
• Makefile:68 — docs-check hardcodes exactly the two current chart README paths, so a future third chart's README drift would be neither detected nor reverted in CI.
  • Fix: Derive the README list from the chart search root instead of hardcoding paths.
• Makefile:38 — The command -v helm guard is duplicated verbatim in chart-deps and install-helm-unittest.
  • Fix: Factor it into a shared require-helm prerequisite.
• scripts/helmcov.sh:1 — Pure-logic pieces (load_tool_versions parsing, helmcov.sh threshold/precondition guards, arch branch) are testable without Docker/network but have no tests.
  • Fix: Add bats/shunit2 unit tests for the parser and guard branches.

---

Reviewers (6): security, testing, maintainability, project-standards, reliability, adversarial. (The correctness reviewer did not return before synthesis; its surface — shell/Makefile/hook/template rendering correctness — was covered by orchestrator-side verification: the Makefile include of the digest-pinned env parses as valid make assignment, load_tool_versions correctly preserves the @sha256: digest value, and all new test assertions render as claimed.)

Testing gaps:

• Pre-commit stash/restore failure paths (partial commit, pop conflict, make failure exit-code) are unverified end-to-end.
• Shell tooling scripts (load_tool_versions parsing, helmcov.sh guards) have no unit tests.
• No fixture exercises the docs-check stale-README failure path; only the happy path is implicitly covered.

[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

[jordan-simonovski]

Coverage on the library is about 73% at the moment. It's pretty well tested in terms of how it handles coverage assertions. And yeah I'm open to forking this. SHA pinning there should at least stop anything potentially malicious from happening, though if it's easier to use as an internal thing I'm ok with it.

[wrn14897]

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

[wrn14897]

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

We’re using changesets in the main OSS repo, and it’s been quite useful for developers to manage the changelog. We’re also using its github action to automatically create release PRs (for example: #229) and streamline the release process. Imo I don't think we need to deprecate it atm.