Repository navigation
ADR-010: Measure Helm template coverage with chart-coverage - #31
Conversation
PR Summary by QodoADR: Measure Helm template branches with chart-coverage
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
/cc |
| **Who is impacted**: | ||
| - Chart authors, who need to see uncovered template branches on PRs | ||
| - QE / Install, who own Helm quality signals | ||
| - Anyone comparing chart coverage to operator Codecov coverage |
There was a problem hiding this comment.
| - Anyone comparing chart coverage to operator Codecov coverage |
This is not a goal IMO. It doesn't make sense to compare coverage between those different install methods. The point is to ensure coverage is maintained at a reasonable level and does not degrade as we change code in the chart repo itself.
|
|
||
| **Constraints**: | ||
| - Do not rewrite charts or replace chart-testing | ||
| - OSS, pinable, Apache-2.0-compatible |
There was a problem hiding this comment.
| - OSS, pinable, Apache-2.0-compatible | |
| - OSS, pinable |
No constraint on the specific license, as long as it OSS and can be used.
| - Do not rewrite charts or replace chart-testing | ||
| - OSS, pinable, Apache-2.0-compatible | ||
| - Codecov-compatible report (Cobertura XML). Go `cover.out` is not required | ||
| - First numbers will be low (local baseline: rhdh ~28%, must-gather ~61%, orchestrator-infra ~84%). Coverage must not become a merge gate on day one |
There was a problem hiding this comment.
| - First numbers will be low (local baseline: rhdh ~28%, must-gather ~61%, orchestrator-infra ~84%). Coverage must not become a merge gate on day one | |
| - Coverage must not become a merge gate on day one |
Not necessary to mention this baseline IMO.
| - Codecov-compatible report (Cobertura XML). Go `cover.out` is not required | ||
| - First numbers will be low (local baseline: rhdh ~28%, must-gather ~61%, orchestrator-infra ~84%). Coverage must not become a merge gate on day one | ||
|
|
||
| Spike notes: [Google doc](https://docs.google.com/document/d/1iMGzEt_tpxikwQ01hFQphLoEBCWeHrXTlTR1g00NGhg/edit). Implementation: [RHIDP-18349](https://redhat.atlassian.net/browse/RHIDP-18349), [rhdh-chart#698](https://github.com/redhat-developer/rhdh-chart/pull/698). |
There was a problem hiding this comment.
Could move under a References section at the bottom of the ADR.
| Use **[chart-coverage](https://github.com/jCOTINEAU/chart-coverage)** (`helm-coverage.sh`) to measure Helm **template branch** coverage from existing `ci/` values, and upload Cobertura XML to **Codecov** as an informational check. | ||
|
|
||
| **Implementation approach**: | ||
| - Vendor `helm-coverage.sh` in `rhdh-chart` (Apache-2.0), pinned to a SHA. Do not curl `main` in CI |
There was a problem hiding this comment.
As I commented out in redhat-developer/rhdh-chart#698 (comment), I would suggest not vendoring it. We can curl it directly as needed using a specific commit SHA.
The "Do not curl main" concern is valid (pin to a SHA, not a branch), but that doesn't require vendoring; curl with a pinned SHA achieves the same.
There was a problem hiding this comment.
Ah ok, so instead of actively vendoring a copy of the coverage tool, fetching it from the existing repo that is included in CI which is this third party repo the helm chart uses for coverage - repo, instead of vendoring a direct copy of it everytime
|
|
||
| **Implementation approach**: | ||
| - Vendor `helm-coverage.sh` in `rhdh-chart` (Apache-2.0), pinned to a SHA. Do not curl `main` in CI | ||
| - New cluster-free GitHub Actions workflow on PRs to `main` / `release-*` and on push to `main` |
There was a problem hiding this comment.
My comment in redhat-developer/rhdh-chart#698 (comment) suggests hooking coverage into the existing test.yaml (PRs, changed charts only) and nightly.yaml (all charts), not a standalone workflow.
| - ✅ Uncovered template branches become visible on PRs once the `main` baseline exists | ||
|
|
||
| ### Negative | ||
| - ❌ First rhdh figure is ~28%. That is expected (only `ci/` branches), but it looks “low” next to Go coverage |
There was a problem hiding this comment.
| - ❌ First rhdh figure is ~28%. That is expected (only `ci/` branches), but it looks “low” next to Go coverage |
Not necessarily negative. The goal is just to start tracking coverage as a starting point, regardless of what the numbers are.
|
|
||
| ### Negative | ||
| - ❌ First rhdh figure is ~28%. That is expected (only `ci/` branches), but it looks “low” next to Go coverage | ||
| - ❌ Vendoring ~1.5k lines of upstream bash; we exclude it from Differential ShellCheck and Sonar rather than forking the tool |
There was a problem hiding this comment.
| - ❌ Vendoring ~1.5k lines of upstream bash; we exclude it from Differential ShellCheck and Sonar rather than forking the tool |
Vendoring not needed.
| **Who is impacted**: | ||
| - Chart authors, who need to see uncovered template branches on PRs | ||
| - QE / Install, who own Helm quality signals | ||
| - Anyone comparing chart coverage to operator Codecov coverage |
There was a problem hiding this comment.
Dropping this as it is not relevant, comparison of chart coverage to operator is not applicable to tracking chart coverage over a period of time - out of scope
|
|
||
| **Constraints**: | ||
| - Do not rewrite charts or replace chart-testing | ||
| - OSS, pinable, Apache-2.0-compatible |
There was a problem hiding this comment.
No license constraint, instead it is Open-Source
| - OSS, pinable, Apache-2.0-compatible | ||
| - OSS, pinable | ||
| - Codecov-compatible report (Cobertura XML). Go `cover.out` is not required | ||
| - First numbers will be low (local baseline: rhdh ~28%, must-gather ~61%, orchestrator-infra ~84%). Coverage must not become a merge gate on day one |
There was a problem hiding this comment.
Removing the base-line numbers 28/61/84, from constraints
| Use **[chart-coverage](https://github.com/jCOTINEAU/chart-coverage)** (`helm-coverage.sh`) to measure Helm **template branch** coverage from existing `ci/` values, and upload Cobertura XML to **Codecov** as an informational check. | ||
|
|
||
| **Implementation approach**: | ||
| - Vendor `helm-coverage.sh` in `rhdh-chart` (Apache-2.0), pinned to a SHA. Do not curl `main` in CI |
There was a problem hiding this comment.
No more vendoring a copy of helm-coverage.sh in rhdh-chart, with each CI run, curl pinned SHA instead of vendor.
Updated: fetch helm-coverage.sh from ./chart-coverage from a pinned SHA.
| - Hook coverage into existing workflows; do not add a standalone coverage workflow | ||
| - **PRs (`test.yaml`)**: coverage only for charts already in that workflow's matrix (changed charts; all non-excluded charts if CI files changed) | ||
| - **Nightly (`nightly.yaml`)**: coverage for every supported chart the nightly matrix already lists (`ct.yaml` exclusions unchanged) |
There was a problem hiding this comment.
Do not vendor, use curl + SHA.
Replaced "new standalone workflow" with hook into test.yaml and nightly.yaml.
test.yaml - only charts what the PR already tests.
nightly.yaml - every supported chart the nightly job (which is tested once a day) already lists (ct.yaml)
| - ❌ First rhdh figure is ~28%. That is expected (only `ci/` branches), but it looks “low” next to Go coverage | ||
| - ❌ Vendoring ~1.5k lines of upstream bash; we exclude it from Differential ShellCheck and Sonar rather than forking the tool |
There was a problem hiding this comment.
Deleting this as it is not relevant, we are measuring against the baseline coverage which is 28%
| - ❌ First rhdh figure is ~28%. That is expected (only `ci/` branches), but it looks “low” next to Go coverage | ||
| - ❌ Vendoring ~1.5k lines of upstream bash; we exclude it from Differential ShellCheck and Sonar rather than forking the tool |
There was a problem hiding this comment.
No need to vendor a copy, replacing the negatives in regard to vendoring, with SHA-bump trade off
Co-authored-by: Armel Soro <asoro@redhat.com>
Co-authored-by: Armel Soro <asoro@redhat.com>
Co-authored-by: Armel Soro <asoro@redhat.com>
Co-authored-by: Armel Soro <asoro@redhat.com>
Co-authored-by: Armel Soro <asoro@redhat.com>
Co-authored-by: Armel Soro <asoro@redhat.com>
Co-authored-by: Armel Soro <asoro@redhat.com>
Co-authored-by: Armel Soro <asoro@redhat.com>
Co-authored-by: Armel Soro <asoro@redhat.com>
|
@NiallTwomey2 From https://github.com/redhat-developer/rhdh-adr/actions/runs/41816281618?pr=31, please rename the decision file accordingly:
/retitle ADR-010: Measure Helm template coverage with chart-coverage |
|
Ack, I understand, renamed the ADR file to 010-helm-chart-template-coverage to reflect the sequences of live ADR files on |
RHIDP-18485
RHIDP-13290
RHIDP-11864