feat(ci): validate every stack, not just the estate's (#263) - #278
Merged
Conversation
`STACK ?= observability` parameterised the lifecycle and secrets targets and stopped. Every validator carried its own `stacks/observability` — validate.sh, four Python checkers, check_loki_rules.sh, seed-validation-env.sh, ci.yml and pin-digests.sh — so stacks/lab landed as a stack CI had never seen: compose not `config`-checked, rules not promtool-tested, dashboards not checked against its own datasources, images pinned and digest-verified by nothing. scripts/stacks.sh is now the one definition of what a stack is, and everything that iterates stacks reads it — including the Python checkers, because a second implementation in Python would be a second definition, and it is the definition that decides what gets checked at all. A stack is a directory under stacks/ holding a compose.yaml; a directory without one is an ERROR rather than a skip, which is the acceptance criterion this issue asked for. Silently skipping a malformed directory is indistinguishable from the defect being fixed. Three checks got stronger rather than merely wider. Rules without promtool unit tests are now a failure. `check rules` parses PromQL and never asks whether an expression can be true — ContainerHighMemory was unfireable for months while passing it (#63) — so a new stack arriving with rules and no tests is that waiting to happen, and the moment to say so is when the rules land. The reload-config.sh cross-check split in two. SERVICES is the union across stacks and reload-config.sh now skips what a stack does not declare, so "not defined in this compose file" stopped being a defect on its own. Per file, an entry the stack DOES declare must still carry the healthcheck the probe stands on — now applied to every stack rather than only the estate's. Across the complete set, every entry must be declared somewhere, which is what still catches an array naming a service nothing has. ABSENT_BINARIES got the same treatment. The completeness half is only claimed when the script discovered the stacks itself; explicit paths mean the caller chose the scope. check_docs.py's PROSE list globs stacks/*/README.md instead of naming the estate's, so a version pin in a stack README goes stale loudly (#73). Absences are reported as passes, not skips. A skip means "could not check, and therefore proved nothing", and validate.sh counts them precisely so a run cannot claim to have checked what it did not (#68). "This stack has no Alertmanager" is a verified fact about the tree, not a gap in the run — inflating SKIPPED with by-design absences is how that count stops being read. Two bugs found while writing it, both in code this commit adds: `mapfile < <(...)` does not carry the subshell's exit status, so the stack-list guard would never have fired; and `((n)) && cp ...` under `set -e` exits the script when n is zero, which would have killed check_loki_rules.sh on a stack with dashboards and no rules. Verified by introducing each fault: an empty stacks/ directory, a renamed SERVICES entry, a version pin in the lab README, and a rule file with its tests removed. All four fail with the sentence that names the cause. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The CI digest-pin-by-digest step can incorrectly pass if scripts/stacks.sh --paths fails due to the current process-substitution pipeline structure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR resolves #263 by making stack validation iterate over every directory under stacks/ (failing on malformed stack directories), so CI and make validate cover all stacks rather than being pinned to stacks/observability.
Changes:
- Introduces
scripts/stacks.shas the single source of truth for stack enumeration (and validatescompose.yamlpresence). - Updates
scripts/validate.sh, CI workflow, and supporting scripts to run validation per-stack (including per-stack.envseeding and per-stack checks). - Updates documentation to reflect multi-stack validation and stronger cross-stack assertions.
File summaries
| File | Description |
|---|---|
| stacks/lab/README.md | Updates lab stack docs to reflect that CI/validate now cover the stack. |
| stacks/lab/grafana/dashboards/README.md | Updates dashboard docs to reflect multi-stack dashboard validation behavior. |
| scripts/validate.sh | Iterates validations across all stacks listed by scripts/stacks.sh and adjusts per-stack checks. |
| scripts/stacks.sh | New: defines/validates the stack list by scanning stacks/*/compose.yaml. |
| scripts/seed-validation-env.sh | Adds optional stack argument; derives guard requirements from that stack’s compose.yaml. |
| scripts/check_loki_rules.sh | Adds --stack support and treats “no rules/dashboards” as a pass (not a skip). |
| scripts/check_docs.py | Expands PROSE checks to include every stacks/*/README.md. |
| scripts/check_dashboards.py | Adds --stack support and treats “no dashboards” as a pass for non-emit modes. |
| scripts/check_compose_health.py | Adds --cross-stack mode and splits per-stack vs cross-stack assertions. |
| Makefile | Makes digest pin/verify targets iterate across all stacks. |
| docs/roadmap.md | Updates roadmap narrative to reflect #263 being addressed and the stronger checks. |
| .github/workflows/ci.yml | Removes pinned STACK env and loops validation steps over all stacks. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+142
to
+144
| done < <(./scripts/stacks.sh --paths | while read -r sd; do | ||
| awk '$1 == "image:" { print $2 }' "$sd/compose.yaml" | ||
| done) |
Comment on lines
+398
to
+402
| declared: set[str] = set() | ||
| for entry in listed: | ||
| compose_path = REPO / entry / "compose.yaml" | ||
| compose = yaml.safe_load(compose_path.read_text(encoding="utf-8")) | ||
| declared |= set(compose.get("services") or {}) |
| # A .env is required for the ${VAR:?} guards; seeded by the same script CI | ||
| # uses, so a variable added there cannot pass locally and fail in CI. Written | ||
| # to a temp file rather than ${STACK}/.env so a local run never leaves an .env | ||
| # to a temp file rather than ${sd}/.env so a local run never leaves an .env |
Comment on lines
+35
to
+39
| validators multi-stack. A dashboard committed to this directory is checked by | ||
| `make validate` and by CI the same way the estate's seven are: JSON validity, | ||
| unique uid, provisioned datasource references, and its PromQL and LogQL | ||
| parsed by promtool and by Loki's own ruler. Today it reports "no dashboards | ||
| — nothing to check", which is a pass and not a skip. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #263.
Stacked on #276 — this PR is
based on that branch, because #263 exists to validate what #276 adds. Its own
commit is the last one; review that.
The problem, restated from the issue
STACK ?= observabilityreached the lifecycle and secrets targets and stopped.Every validator carried its own copy of
stacks/observability:validate.sh,check_docs.py,check_dashboards.py,check_compose_health.py,check_loki_rules.sh,seed-validation-env.sh,pin-digests.shandci.yml.stacks/labtherefore landed as a stack CI hadnever seen — compose not
config-checked, rules notpromtool-tested,dashboards not checked against its own datasources, images pinned and
digest-verified by nothing.
The answer to the issue's open question
Iterate, not matrix. A GitHub Actions matrix would put stack knowledge back
into
ci.yml, andlint.shalready asserts thatci.yml, theMakefileandvalidate.shdelegate to the same scripts rather than carrying their owncopies. Iterating keeps one definition and one set of commands.
scripts/stacks.shis that definition. Everything that iterates reads it —including the Python checkers, via
subprocess, because a secondimplementation in Python would be a second definition of what a stack is, and
that is the definition deciding what gets checked at all.
A stack is a directory under
stacks/holding acompose.yaml. Adirectory without one is an error, not a skip — the issue's acceptance
criterion. Silently skipping a malformed directory is indistinguishable from
the defect being fixed.
Three checks got stronger, not just wider
Rules without unit tests now fail.
check rulesparses PromQL and neverasks whether an expression can be true —
ContainerHighMemorywas unfireablefor months while passing it (#63). A new stack arriving with rules and no tests
is that waiting to happen, and the moment to say so is when the rules land.
The
reload-config.shcross-check split in two.SERVICESis the unionacross stacks, and
reload-config.shnow skips what a stack does not declare,so "not defined in this compose file" stopped being a defect on its own. Per
file, an entry the stack does declare must still carry the healthcheck the
probe stands on — now applied to every stack rather than only the estate's.
Across the complete set, every entry must be declared somewhere, which is
what still catches an array naming a service nothing has.
ABSENT_BINARIESgotthe same treatment. The completeness half is only claimed when the script
discovered the stacks itself; explicit paths mean the caller chose the scope and
no repo-wide claim can follow from a subset.
check_docs.py'sPROSElist globsstacks/*/README.mdinstead of namingthe estate's, so a version pin in any stack README goes stale loudly (#73).
Absences are passes, not skips
A skip means "could not check, and therefore proved nothing", and
validate.shcounts them precisely so a run cannot claim to have checked whatit did not (#68). "This stack has no Alertmanager" is a verified fact about the
tree, not a gap in the run. Inflating
SKIPPEDwith by-design absences is howthat number stops being read — the skip count is still 2, the same two as
before.
Two bugs in my own new code, found and fixed
mapfile -t STACKS < <(./scripts/stacks.sh)does not carry thesubshell's exit status, so
if ! mapfile ...always succeeds — thestack-list guard would never have fired. Now a command substitution.
((n_committed)) && cp ...underset -eexits the script when thecount is zero, which would have killed
check_loki_rules.shon a stack withdashboards and no rules.
Verification
make validate— all checks passed, 2 skipped (the same two as before).make check-digestsnow covers both stacks — the lab's four images wereverified against the registry for the first time.
Each new guard was proved by introducing the fault, not by reading the code:
mkdir stacks/emptySERVICESentryreloads blackbox-exportr, which no stack definesstacks/lab/README.mdonly compose.yaml may carry a versionlab.test.yaml1 rule file(s) and no promtool tests … (#63)Also corrected
stacks/lab/README.mdand its dashboards README said "nothing validates thisstack" and "
check_dashboards.pydoes not enforce them here". Both were truewhen written and are now false, so both were rewritten — along with what
make validatestill does not prove about that stack, which is that it hasnever actually been run.
🤖 Generated with Claude Code