Skip to content

Chore: Set up helmcov and helmdocs for charts.#232

Open
jordan-simonovski wants to merge 6 commits into
mainfrom
jordansimonovski/setup-helmcov
Open

Chore: Set up helmcov and helmdocs for charts.#232
jordan-simonovski wants to merge 6 commits into
mainfrom
jordansimonovski/setup-helmcov

Conversation

@jordan-simonovski

Copy link
Copy Markdown

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

@jordan-simonovski jordan-simonovski requested a review from a team as a code owner June 27, 2026 01:51
@changeset-bot

changeset-bot Bot commented Jun 27, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: e1cfcd6

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

CLAassistant commented Jun 27, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

github-actions Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

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 testchart-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:17matchRegex 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.

jordan-simonovski and others added 3 commits June 27, 2026 14:13
Replace Docker-based make coverage in CI with jordan-simonovski/helmcov@v1
so pull requests get upserted coverage summaries with threshold enforcement.

Co-authored-by: Cursor <cursoragent@cursor.com>
v0.3.3 lacks --markdown-file support required by the v1 action entrypoint.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

github-actions Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants