fix: Refuse a profile the compose file does not declare #47
No reviewers
Labels
No labels
priority/P0
priority/P1
priority/P2
priority/P3
size/L
size/M
size/S
size/XL
size/XS
state/done
state/in-progress
state/in-review
state/needs-refinement
state/ready
type/bug
type/chore
type/feature
type/refactor
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
amtronics/platform-actions!47
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/39-guard-monitoring-profile"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Issue
Closes #39
Problem
deploy-vpsarmsCOMPOSE_PROFILES=monitoringfrom the presence of the three Grafana credentials alone, without looking at the compose file it is about to deploy. Compose treats a profile nothing declares as selecting no services and still exits 0 — so an application whose compose has nomonitoringservices deploys, reports success, and collects nothing, while its absent-metrics alert fires against a perfectly healthy service.That is
ahmad/imamah#134generically: one repository's instance was fixed there, the behaviour was not.Solution
Before
up, the deployment refuses a profile the compose file does not declare, naming both.Review notes
The check is on the host, not where the profile is armed, and that is deliberate. The arming happens in a local step; the only reliable way to ask which profiles a compose file declares is
docker compose config --profiles, and the action cannot assume a docker CLI inside the job container. Grepping the YAML would be dependency-free and wrong in both directions — a false negative would refuse a good deployment. The remote script runs where docker is guaranteed and the real parser is available.Verified against a stub
docker compose, all four states:Mutation — the condition neutered to
if false, keeping the grammar valid and confirmed withbash -n:The fourth acceptance criterion is not met as written, and the criterion is mine. I wrote "when the action's tests run, then all three cases above are covered". This repository has no behavioural test harness for actions —
check-actions.pyparses eachaction.ymland runsbash -nover therun:bodies, and cannot execute one. The three cases are covered by the verification above rather than by a committed test, which is weaker: it does not run again when someone edits this action next year.Building that harness is a real gap and I have filed it separately rather than smuggling it in here.
Risks and trade-offs
This turns a silent misconfiguration into a failed deployment. That is the intent, and it is the right way round — an unmonitored deploy that believes it is monitored is worse than one that stops. But it does mean a target whose compose legitimately lacks the profile, while the credentials happen to be present, now fails where it used to proceed. The message says both remedies, and the credentials being present on a target that cannot use them is itself worth knowing.
${COMPOSE_PROFILES//,/ }splits on commas, which is how compose accepts multiple profiles. Onlymonitoringis ever armed today, so the loop is future-proofing rather than a live case — cheap, but it is one more thing to be wrong if compose's syntax ever grows.validategreen one5a7d6e. Verification against each acceptance criterion.Both remedies are named, because "it failed" without "and here is what to do" is how a guard becomes something people delete.
exit 0, and nothing else in the step changed — the arming logic is untouched; this only reads what it produced.exit 0with the block skipped entirely, since it is guarded onCOMPOSE_PROFILESbeing non-empty.A fourth state, not in the criteria but worth covering because it is the one a second profile would produce: the compose declares
debugand the deploy asks formonitoring— fails, and reportsDeclared profiles: debug, so the message distinguishes "declares nothing" from "declares something else".Not met as written, and I wrote it. This repository has no harness that can execute an action —
check-actions.pyparses and runsbash -n, nothing more. The four states above are covered by a stub-driven verification, which does not run again when someone edits this action later. Filed as #48 rather than quietly widening this pull request, and the mutation is what stands in for a test meanwhile:Not proven here: that a real deployment refuses. That needs a host, credentials and a compose file without the profile — which is
imamahbefore its #134, and nothing available to me. The stub exercises the logic; the shape of the failure in production is inferred from compose's documented behaviour, which is the same inference the bug itself rests on.WIP: fix: Refuse a profile the compose file does not declareto fix: Refuse a profile the compose file does not declareOne finding, inline and non-blocking. The guard does what it claims, and the acceptance criteria you can meet are met — CI green.
The most useful thing I can report is that this breaks nothing on merge. Your risks section flags a target whose compose lacks the profile while the credentials happen to be present, which would newly fail. I checked whether that target exists, across every consumer in the fleet:
The two sets are identical, so no current deployment is newly refused. (
ahmad/imamahis absent from both, consistently — it has its own deploy action rather than this one.expiro,bayan-web,stem,suhba-webandsuhba-docsdeclare no profile and arm none.)Verified against a stub
docker, all four documented states plus the not-armed case:On the fourth criterion you declared unmet: saying so, naming why (
check-actions.pyparses andbash -ns but cannot execute an action), and filing the harness separately is the right handling. It is worth being blunt about what that costs, since the trade recurs in this repository: the verification above holds today and will not run again when someone edits this guard next year. The behaviour most at risk from that is the one in my comment — an error path nobody exercises by hand.On placing the check on the host rather than where the profile is armed: agreed, and the reasoning generalises.
config --profilesis the real parser; a grep over the YAML would be wrong in both directions, and the false-negative direction refuses a good deployment, which is the worse half.@ -325,0 +333,4 @@# the only reliable answer; grepping the YAML would be wrong in both# directions.if [ -n "${COMPOSE_PROFILES:-}" ]; thenDECLARED="$(docker compose -f docker-compose.vps.yml config --profiles 2>/dev/null || true)"2>/dev/null || truecollapses "docker could not answer" into "the answer is nothing", and the message that follows then blames the compose file. Driven against a stubdocker, the four documented cases behave exactly as your table says — and so do two that are not in it:In both, the compose file may declare
monitoringperfectly well. The deployment fails either way — the next line isdocker compose pull, which would fail too — so this is a diagnosis fault, not a correctness one. But it is the specific kind this fleet keeps paying for: a red that names the wrong cause sends whoever reads it to edit a compose file that was never wrong, and the real error (yaml: mapping values are not allowed,Cannot connect to the Docker daemon) was discarded to/dev/nullbefore anyone saw it.Concrete fix — keep the output, distinguish the two failures:
That still stops the deployment, and it stops it saying what actually went wrong.
Minor, while here:
grep -qxtreats${want}as a basic regular expression, so a profile name containing.would match more than itself.grep -qxFis what you use in the reporters in this same repository, and it costs one letter.fix: Refuse a profile the compose file does not declareto WIP: fix: Refuse a profile the compose file does not declareBoth taken, fixed in
dd17f63.The swallowed error is the better find, and it is the failure mode I was warning about elsewhere in this same change.
2>/dev/null || truemade "docker could not answer" indistinguishable from "the answer is nothing", so the message blamed a compose file that might declare the profile perfectly well — while the real cause went to/dev/null. Now:The four documented states are unchanged and still behave:
The
-Fpoint was not cosmetic, and I checked which direction it fails in. A profile whose name contains.matched a different declared profile — so the previous form could have let an undeclared profile through, which is the failure this guard exists to prevent rather than a harmless over-match:No profile in use today contains a regex metacharacter, so nothing was live — but the guard would have been silently weaker than it reads, which is the same class of defect as the one it was written to catch.
Thank you for checking the merge is safe across the fleet. Enumerating which consumers declare
profiles: [monitoring]against which passgrafana-prom-*— and finding the sets identical — is the evidence my risks section asserted without measuring. I wrote "a target whose compose legitimately lacks the profile now fails"; you established that no such target exists, which is a stronger statement than the one I hedged.On the harness cost you named: agreed, and it is the right thing to keep saying out loud. The verification in this pull request holds today and will not run again, and the behaviour most exposed by that is exactly the error path you found — nobody exercises it by hand twice. That is
#48.validategreen ondd17f63; back to ready.The delta since the reviewed head is the two findings, both verified above: the six states (four documented plus the invalid-compose and unreachable-daemon paths that previously reported the wrong cause), and the literal-match case showing the old form let an undeclared profile through.
Everything else stands as verified, including the fourth criterion still being met by stub rather than by a test that runs again —
#48.WIP: fix: Refuse a profile the compose file does not declareto fix: Refuse a profile the compose file does not declareApproved. Both halves of the finding are closed. Re-reviewed only the delta (
e5a7d6eb5d..dd17f639c3); CI green.Driven against a stub
docker, eight cases — the four you documented, the two I raised, and two the fix newly makes possible:The last two are the reason I checked rather than just reading: capturing
2>&1means adocker composethat succeeds while warning on stderr now folds that warning intoDECLARED. It is benign in both directions —grep -qxmatches any line, so a warning cannot mask a profile that is declared, and no plausible warning is the literal stringmonitoring, so it cannot invent one that is not. The only cost is a warning line appearing in theDeclared profiles:message on the failure path, where the extra context is more use than harm.The
-Fis not cosmetic, and I should have said so more plainly the first time:Without it a profile name containing
.matches a different declared profile, which would silently pass a mismatched deployment — the exact failure this guard exists to stop, reintroduced through the check itself.Nothing further. The earlier verification stands: the four consumers that arm
monitoringare still exactly the four that declare it, so this remains a no-op for every live deployment.