Repository navigation
fix(deploy): detect critical stacks when com.compose.tier is quoted - #99
Merged
Merged
Conversation
detect-critical-stacks.sh grepped for the literal string 'com.compose.tier: infrastructure', but every consumer repo writes label values double-quoted (com.compose.tier: "infrastructure"), so no stack was ever auto-detected as critical and the health-check gate never ran. Match both labels through one anchored has_label helper that accepts unquoted, double- and single-quoted values, map or list (key=value) form, surrounding whitespace and a trailing comment, while rejecting commented-out lines and other values. Adds scripts/testing/test-detect-critical-stacks.sh (25 cases).
Reviewer's GuideCritical-stack detection now uses a shared, line-anchored ERE matcher that correctly handles quoted Compose labels and list/map syntax while rejecting comments and near-matches. A standalone 25-case regression harness and updated documentation cover the behavior; reviewers should also verify the separately identified health-check gate failure before merging. Sequence diagram for critical-stack detection in deployment preparationsequenceDiagram
participant Prepare
participant Detector
participant ComposeFiles
participant CriticalStacks
Prepare->>Detector: detect-critical-stacks.sh
Detector->>ComposeFiles: Scan each compose file
Detector->>Detector: has_label(file, key, value)
alt tier=infrastructure or critical=true
Detector->>CriticalStacks: Add stack
else No anchored label match
Detector->>CriticalStacks: Skip stack
end
Detector-->>Prepare: Output critical stack JSON
Flow diagram for critical-stack label detectionflowchart TD
A[Compose file] --> B[has_label]
B --> C{Anchored label match}
C -->|tier=infrastructure| D[Mark stack critical]
C -->|critical=true| D
C -->|No match| E[Leave stack non-critical]
B --> F[Accept quoted or unquoted map/list forms]
B --> G[Reject comments and near-matches]
D --> H[Add stack to CRITICAL_STACKS]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/deployment/detect-critical-stacks.sh" line_range="84" />
<code_context>
# Method 1: Check for tier=infrastructure label
- if grep -q "com.compose.tier: infrastructure" "$COMPOSE_FILE" 2>/dev/null; then
+ if has_label "$COMPOSE_FILE" 'com\.compose\.tier' 'infrastructure'; then
IS_CRITICAL=true
fi
</code_context>
<issue_to_address>
**Deploys take whole-tree rollback**
When a detected critical stack requires Compose interpolation variables that the health-check does not load, the health-check's `docker compose ps` exits nonzero under `set -e`, so `failed_stacks` is never written. Rollback receives no culprit and selects a whole-tree rollback.
Run the health check with the stack's required Compose interpolation environment and ensure it records the failed stack.
</issue_to_address>
### Comment 2
<location path="scripts/deployment/detect-critical-stacks.sh" line_range="57" />
<code_context>
+has_label() {
+ local file="$1" key="$2" value="$3"
+ local q="[\"']?"
+ grep -qE "^[[:space:]]*(-[[:space:]]*)?${q}${key}${q}[[:space:]]*[:=][[:space:]]*${q}${value}${q}[[:space:]]*(#.*)?$" \
+ "$file" 2>/dev/null
+}
</code_context>
<issue_to_address>
**Critical stack gate gets wrong set**
When a Compose file uses flow-style labels or has a matching key/value outside a service's `labels` field, `has_label` scans text lines instead of parsing service labels, so it misses valid flow-style declarations and accepts matching text in unrelated sections or lists. `CRITICAL_STACKS` is then wrong, causing a real critical stack to miss the health gate or a noncritical stack to enter it.
Parse the Compose YAML and inspect each service's labels, including flow-style maps and lists.
Also at `scripts/deployment/detect-critical-stacks.sh:58`.
</issue_to_address>
### Comment 3
<location path="scripts/deployment/detect-critical-stacks.sh" line_range="57" />
<code_context>
+has_label() {
+ local file="$1" key="$2" value="$3"
+ local q="[\"']?"
+ grep -qE "^[[:space:]]*(-[[:space:]]*)?${q}${key}${q}[[:space:]]*[:=][[:space:]]*${q}${value}${q}[[:space:]]*(#.*)?$" \
+ "$file" 2>/dev/null
+}
</code_context>
<issue_to_address>
**Nonmatching labels become critical**
When a plain label value contains the target text followed immediately by `#` or a quote character, `has_label` treats `#` as a comment without requiring preceding whitespace and makes opening and closing quotes independently optional. A value such as `infrastructure#suffix` or `infrastructure'` therefore matches the target exactly, and the stack is incorrectly marked critical.
Recognize YAML scalar boundaries correctly: pair quote delimiters and treat `#` as a comment only when YAML permits it.
Also at `scripts/deployment/detect-critical-stacks.sh:58`.
</issue_to_address>
### Comment 4
<location path="scripts/deployment/detect-critical-stacks.sh" line_range="84" />
<code_context>
# Method 1: Check for tier=infrastructure label
- if grep -q "com.compose.tier: infrastructure" "$COMPOSE_FILE" 2>/dev/null; then
+ if has_label "$COMPOSE_FILE" 'com\.compose\.tier' 'infrastructure'; then
IS_CRITICAL=true
fi
</code_context>
<issue_to_address>
**Empty stacks pass the health gate**
When a detected critical stack has no containers returned by Compose, such as when its containers were never created or the project has no matching containers, `docker compose ps -a` can succeed with no containers, after which the `jq` filter counts zero unhealthy services and the step writes `status=healthy`. A critical stack with no containers therefore passes without any service being checked.
Treat an empty container list as a health-check failure and include that stack in `failed_stacks`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 4 findings to address first, and a matcher mistake could omit a critical stack from health-check rollback gating or gate a noncritical one, changing deployment behavior and potentially allowing an unhealthy production stack to remain deployed or triggering an unnecessary rollback. Reverting prevents future misclassification, but any outage or deployment action that already occurred would require separate remediation.
Blocking findings: scripts/deployment/detect-critical-stacks.sh:84, scripts/deployment/detect-critical-stacks.sh:57, scripts/deployment/detect-critical-stacks.sh:57, scripts/deployment/detect-critical-stacks.sh:84
Review follow-up: `infrastructure#x` and `infrastructure'` matched because the trailing comment did not require preceding whitespace and the opening/closing quotes were independently optional. Build explicit map/list alternatives with paired quotes, require whitespace after the map colon and before `#`, and document that flow-style label maps are not recognised. Five more negative cases (30 total).
…ics and teardown
Run docker compose ps/logs/down as `-p <stack>` from / instead of inside the
stack dir. These calls run without `op run`, so in-dir they parsed the compose
file with empty ${VARS}; a file whose syntax depends on a var (traefik's
extra_hosts "${DOMAIN}:...") failed to parse. That crashed the critical-stack
health check before failed_stacks was written (whole-tree rollback), blanked
dump_failed_stack diagnostics, and broke teardown/rollback `down` for that stack.
The health check also now fails a critical stack with zero containers instead
of passing vacuously.
owine
added a commit
that referenced
this pull request
Oct 10, 2026
… calls (#101) * fix(deploy): resolve compose project from container labels for no-env calls Follow-up to #99 review. ps/logs/down that run without `op run` now go through a compose_p helper (installed per job into $RUNNER_TEMP) that reads the project name from the containers' com.docker.compose.project.working_dir label, falling back to the stack name. `-p "$stack"` silently missed when Compose normalises the dir name (dots, uppercase, leading _/-) or a compose file sets a top-level name:, leaving teardown/rollback a no-op (exit 0) and orphaning containers. Also: teardowns no longer require the compose file to exist (down targets the project label; missing file is now a notice), the health check validates stack names like the other loops, redundant export -f removed, and ensure-shared-networks documents that label-based down removes a project-labelled network even after it is marked external. * fix(deploy): compose_p acts on every project found under the stack dir Sourcery review: when containers from more than one Compose project share a stack's working_dir label (e.g. leftovers after a project rename), compose_project returned only the first, so health checks missed the others and down left them running. compose_projects now returns all distinct matches and compose_p runs the command for each (stdin from /dev/null so compose can't eat the loop input), failing if any project's command fails. * fix(deploy): never fall back to a same-named project from another dir; review nits Independent review of #101: with no containers in the stack dir, compose_p fell back to `-p <stack>`, which matches a same-named project running from any directory — teardown (now ungated on the compose file) could down it and the health check could pass on its containers. compose_projects now returns nothing in that case (down no-op, health check fails as "no containers"). The foreign check captures docker output before grep -q to avoid SIGPIPE under pipefail. Nits: update the stale project-name comment, drop the unused helper source in Deploy dockge, and build the health-check failed_stacks JSON with jq.
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
1 participant
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.
Warning
Do not merge on its own. Turning detection on exposes a separate bug in the
health-checkjob: the gate step will crash on docker-piwine'straefikstack, and every docker-piwine deploy will then do a whole-tree rollback. Details are under "Impact analysis" below. The gate needs a follow-up fix first, or in the same change.Bug
scripts/deployment/detect-critical-stacks.shmatched the literal stringcom.compose.tier: infrastructure. Both consumer repos quote label values (com.compose.tier: "infrastructure", the house "Option C" style), so the grep never matched.critical_stackswas always[], and thehealth-checkjob looped over nothing. The critical-stack gate has been effectively disabled in docker-piwine and docker-piwine-office.Method 2 (
com.compose.critical: (true|"true")) had the same problem. It was unanchored, so it matched commented-out lines and missed'true'and list form.Fix
Both labels now go through one
has_label <file> <key> <value>helper, which uses an anchored ERE:"double"and'single'quoted values; map form (key: value); list form (- key=value,- "key=value"); a quoted key; extra whitespace; a trailing# comment; CRLF line endings# com.compose.tier: …), other values (application,infrastructure-lite,infrastructure#x), unpaired or mismatched quotes,key:valuewith no space after the colon, longer keys (com.compose.tierx), and the value word appearing inside some other keylabels: {com.compose.tier: …}) are not recognised. Neither consumer repo uses themThe script's logging and output contract are unchanged. CLAUDE.md now documents the quoted form and the new test.
Tests
scripts/testing/test-detect-critical-stacks.sh: 30 cases, all passing. 14 of the original 25 fail against the old script.shellcheckis clean on both changed scripts.test-classify-rollback-scope.sh(19/19),test-detect-stack-changes.sh(8/8).test-workflow.sh --dry-runandvalidate-compose.shalso pass, but they don't exercise detection.Impact analysis
Detection run against each consumer's
origin/main:traefik-plugin-fetchexited 0, which the selector allows)Why traefik crashes: the gate runs
cd $LIVE_REPO_PATH/$stack && docker compose ps -a --format json | jq -s .with noop runand no env. In that state,${DOMAIN}etc. interpolate to"", and traefik'sextra_hosts: "${DOMAIN}:172.30.250.2"fails to load:docker compose psexits 1. Underset -euo pipefailthat kills the step beforefailed_stacksis written.health-checkfails, and the rollback job getsHEALTH_FAILED=[], which means zero culprits and therefore a whole-tree rollback. I reproduced this read-only on piwine with the exact loop; it exits 1 at traefik. Run 38019353982's un-op runteardown shows"variable is not set"warnings, which confirms the runner has noDOMAIN/TZ/… in its env.Suggested follow-up for the gate (not in this PR):
docker compose -p "$stack" ps -a --format json, and run it from a directory with no compose file such as$RUNNER_TEMP. Do the same fordump_failed_stack'sps/logs. This reads container labels only and needs no env. On piwine it returns all 6 traefik containers with exit 0.ps -p <unknown>exits 0 with no output, which would otherwise pass vacuously.Other notes:
health-checktakes one snapshot, with no retry/wait andtimeout-minutes: 5. Stacks this deploy changed have already passedup --wait(--wait-timeout= 90% ofservice-startup-timeout). A container whose health is stillstartingwhilerunningpasses the selector, so slow starters (Traefik's ACME-gated healthcheck withstart_period: 180s, Authelia) are not flaky here.unhealthyin an untouched critical stack (for example Traefik's healthcheck grepsacme.jsonfor the wildcard cert, so it goes unhealthy if renewal breaks it) fails the deploy. The C1 rule then forces a whole-tree rollback.Rollout
Consumers call
deploy.yml@main, so this affects the next deploy in both repos. Once the gate follow-up has landed: runworkflow_dispatchon a deploy in each repo, then check that thepreparelog showsDetected N critical stack(s)(piwine: beszel, monitoring, traefik; office: dozzle) and thathealth-checkreportsstatus=healthy.Summary by Sourcery
Restore reliable critical-stack detection and make deployment health checks and cleanup robust when Compose files require unavailable environment variables.
Bug Fixes:
Enhancements:
Deployment:
Documentation:
Tests: