fix(deploy): stop updating the bot as a side effect of any playbook run - #65
Open
zaytcevcom wants to merge 26 commits into
Open
fix(deploy): stop updating the bot as a side effect of any playbook run#65zaytcevcom wants to merge 26 commits into
zaytcevcom wants to merge 26 commits into
Conversation
A playbook run made for an unrelated change (token rotation, webhook_domain, a limit tweak) currently re-resolves the moving :latest tag and ships whatever landed on main since the last deploy. Nothing pins that down, so the pull directives can silently come back after being removed. Assert the invariant over the files that decide it — the shipped compose file, the role template and the role tasks — so a reintroduced `--pull always` or `pull_policy: always` fails `task tests`, the gate CI runs. Comment lines are skipped so the rationale may name the directive it forbids. This commit is intentionally red; the next one removes the directives.
`docker compose up -d --pull always` plus `pull_policy: always` made every playbook run re-resolve the moving :latest tag against the registry. A run started for something unrelated — rotating a token, setting webhook_domain, flipping enable_dind, changing a limit — therefore also deployed every commit merged into main since the last run. Updates must be a decision, not a side effect of editing configuration. Drop both directives so compose reuses the image the host already has (it still fetches it on a first install, where nothing is present locally). To move a deployment forward the operator now runs `docker compose pull` and `up -d` explicitly; nothing is downgraded, since the installation stays on exactly the image it is running. --force-recreate stays: with the tag held still it only rebuilds the container, which is what makes a rendered .env take effect on compose versions older than v2.14 that do not detect that change themselves.
Published images carried org.opencontainers.image.version="latest", which tells a reader nothing about which build a host is actually running — the one question the label exists to answer. docker/metadata-action sorts the tag list by priority and stamps the first entry into that label (src/tag.ts sorts, src/meta.ts keeps the first enabled value as version.main). Both raw entries share the default priority 200, so the listed order decided it and :latest came first. Put the immutable 0.1.<run_number> ahead of :latest so the label names the exact build. The published tag set is unchanged (:latest is still emitted, as a partial version), and a vX.Y.Z tag push keeps priority since type=ref outranks raw. Kept both publish workflows mirrored; ci.yml's dry-run image jobs build without metadata-action, so nothing there can drift.
Removing the implicit pull left no supported way to move a deployment forward: a plain run now reuses the image the host already has, so an operator who wants a newer build sees a green, "changed" playbook and stays on the old version, with `docker compose pull` on the host as the only remaining path. Give the update back as a deliberate action instead: bot_image_pull (default false) gates a `docker compose pull bot` that runs before the (re)start, so `-e bot_image_pull=true` fetches the tag and the following --force-recreate puts the new image into service. Everything else keeps the current behaviour. Only the bot service is fetched — the switch is about the bot image, and the pinned sidecars stay on what the host has. It is a variable, not an Ansible tag: a tag would be a second, partly overlapping switch, and selecting only the pull task with --tags would skip the (re)start that makes the new image take effect. changed_when is false because the fetch alone changes nothing on the host; the (re)start task already reports whether the deployment actually moved.
"The role pulls the prebuilt image" described the old behaviour, where every run re-resolved the tag. It now installs the image once and keeps it, so a reader following the README has no way to learn why a re-run leaves the bot on the old version, nor how to move it forward. State both facts in all eight translations: the version is stable across ordinary runs, and -e bot_image_pull=true is the update. Pinning bot_image is unchanged.
Comments in the role state things that are not true, and each one would mislead an operator deciding whether their rollback path exists. - The prune units claim a "recently pulled image stays available for rollback". `docker image prune --filter until=` compares the image's CREATION time, not when the host pulled it (docker docs: "only remove images created before given timestamp"), so an image CI built eight days ago is prunable the moment nothing runs it. Say so, and name the consequence: rolling back after an update usually means pulling that tag again. Images in use by a container are still exempt. - The compose template claims `docker compose up` "cannot" move the deployment to a newer build. It only means compose does not reach the registry itself: if the local tag was already moved, --force-recreate does recreate the container on the newer image — which is exactly how the opt-in update works. - The restart handler pointed at `docker compose up -d --build`, a command the role has never run, under a task name that no longer exists.
The Ansible role no longer re-resolves the tag on every run — it installs the image once and only fetches a newer one when the operator asks. ":latest — what deploys track" now describes behaviour that this branch removed, in the one place a reader looks to decide which tag to deploy. Describe what actually happens instead: the tag is resolved at install time and moves only on an explicit pull. Both publish workflows kept mirrored.
The image version label was decided by the order of two lines in the tags list: both raw entries sat at the default priority 200, so metadata-action's sort left whichever was listed first. Nothing pins that. The action is tracked at the moving @v6 and gets bumped weekly, so any reformatting of this block would put :latest back on top and quietly restore version="latest", with no check to catch it. Give the immutable 0.1.<run_number> priority=250 so it outranks :latest on its own (raw defaults to 200; type=ref stays at 600, so a vX.Y.Z tag push still wins) and shorten the comment that explained the ordering trick. Tag output is unchanged.
The guard promised more than it checked: it only knew `--pull always` in one spelling and `pull_policy: always`, over three files. A plain `docker compose pull`, `docker pull`, the community.docker.docker_image modules or the argv form ["docker", "compose", "up", "-d", "--pull", "always"] all walked past it, as did anything added to deploy/playbook.yml or the role's handlers. State the invariant precisely instead: an UNCONDITIONAL pull is forbidden, while a pull gated by its own task-level `when:` is the opt-in update path and passes. Compose files have no such gate, so any directive there still fails. The gate must sit on the pull task itself — a condition inherited from an enclosing `block:` does not count, since a reader of the task cannot see it. Two supporting checks: a table pins the spellings the patterns must match (and the legitimate lines they must not), so a narrowed pattern fails loudly rather than silently; and bot_image_pull is asserted to default to false, because a default of true turns that `when:` into an unconditional pull wearing a gate. Reading a manifest now reports and continues instead of aborting the whole test, so one unreadable file cannot hide a pull in the others.
adapters/telegram/** is a publish trigger and the Dockerfile copies adapters/ into the build context, so editing a test that only guards the deploy — deploy_pull_test.go sits there because `go test` needs it next to the files it reads — kicks off the full multi-arch publish and invalidates the COPY layer, rebuilding a byte-identical image. `go build` ignores _test.go, so excluding them costs nothing: .dockerignore drops them from the context and both workflows negate them in paths (negations last, since the final matching pattern wins). Verified by building the compile stage of both images with the exclusion in place — green, and /src holds 72 .go files and no test.
The guard accepted any task carrying a `when:`, and found it by scanning forward from the enclosing list item without regard for indentation. Two consequences: - The verdict depended on YAML key order. A pull inside a `block:` passed when the block's `when:` was written after the block and failed when it was written before it, although a mapping's key order carries no meaning — red CI on correct code, green CI on the same playbook re-spelled. - `when: true` passed. The guard asserted that a gate exists, not that it is the opt-in one, so an unconditional pull wearing a gate went through. A pull is now accepted only when the task that holds it carries its own `when:` (a key at that task's own indentation) naming bot_image_pull. Both block spellings are rejected identically: the gate has to be visible on the task that pulls. Also widened and tightened the matching around it: trailing comments are stripped before matching (a comment naming a directive no longer fails the build, and a directive no longer hides behind one), the compose module's own `pull: always` and the docker_compose(_v2) modules are recognised, and the opt-in default accepts every spelling Ansible reads as false (no/False/off/0). The docblock states what the guard does NOT check: it compares the gate's name, not its meaning, so `when: bot_image_pull | bool or true` would still pass.
…it fetched Pulling only the bot left the sidecars with no update path at all. Their tags float within a major (docker:27-dind, caddy:2-alpine), and dind runs privileged: before this branch the per-run `--pull always` kept it current, and dropping that without replacing it would mean a privileged container that never takes an upstream fix. The opt-in fetch now covers every service in the stack. Nothing becomes automatic — the operator still decides when a run pulls. The `changed_when: false` rationale was also wrong: it claimed the (re)start task reports whether the deployment moved, but that task runs `--force-recreate` and therefore always reports changed. The fetch is registered instead and its own output is echoed verbatim in "Report status", so an operator can see whether a new image actually arrived rather than inferring it from a flag that never varies. Same correction in the comments: an ordinary run does not go to the registry and so cannot pick up a new build — it (re)starts on whatever the tag points to on that host. It does not freeze the version, and the defaults no longer promise it.
"An ordinary re-run never changes the running version" was an absolute the role cannot honour. If the local tag has already been moved — an opt-in run whose (re)start was interrupted, or an operator's own pull, which the role suggests for a rollback — the next ordinary run does land on that image. What the role really guarantees is narrower and still the point: an ordinary run does not contact the registry, so it cannot pick up a new build; it restarts the container on the image the tag already points to on that host. All eight translations now say that, each in its own language.
"Negations come last" does not describe the file: two literal patterns follow the negations. The rule that matters is GitHub's — the last matching pattern wins — so a negation has to sit after the pattern it carves out of, which it does; the entries after them overlap with neither. The tag list said :latest "moves only on an explicit pull". The pull fetches the image; the deployment moves when the following (re)start recreates the container on it, so "only after an explicit pull" is the accurate phrasing.
The two sidecar comments said their images move only on an -e bot_image_pull=true run. That is the role's only path, but not the host's: an operator can pull by hand, exactly as the rollback note suggests. Say "the role moves it only ..." so the comments claim what the role does rather than what can ever happen.
The role's notes claimed an ordinary run cannot reach the registry, but compose's default "missing" policy fetches an image the host does not have: on the first install, and after a `compose down` leaves the image unreferenced until the weekly prune reclaims it. That is exactly the silent version move this change set exists to prevent, so it has to be documented rather than denied — and defaults/main.yml contradicted itself about it two paragraphs apart. Say what the role does instead of what cannot happen, and list the cases that still land a deployment on another image next to every claim that used to exclude them. The sidecar note is scoped to the role, which is the boundary it can speak for; a pull run by hand on the host is outside it.
The report claimed the fetch's output was "the only honest answer to did a new image arrive". It is not: `docker compose pull` prints "<service> Pulling"/"Pulled" for every service whether or not anything was downloaded, and a line per layer per status transition on top — so the stderr_lines+stdout_lines join collapsed hundreds of progress lines into one multi-kilobyte debug string that still needed interpreting. Put the signal in the task instead: "Pull complete" is printed only for a layer that was actually downloaded, so changed_when on it makes Ansible's own flag mean "a new image arrived", and the report shrinks to one line derived from that flag. Verified against captured non-TTY output of a real `docker compose pull` in both states, and under async/poll.
"An ordinary re-run never contacts the registry" is not true of a host that has lost the image: after a `compose down` the weekly prune reclaims it, and the next ordinary run has compose fetch the tag again. Describe what the role does — it doesn't go looking for a newer build — instead of promising something the deployment's own housekeeping can break. Also name what `-e bot_image_pull=true` actually moves: the variable reads as if it were about the bot alone, while the fetch and the recreate cover the sidecars too, including the privileged dind. All eight translations carry the same two corrections.
zaytcevcom
commented
Jul 27, 2026
zaytcevcom
left a comment
zaytcevcom
left a comment
Contributor
Author
There was a problem hiding this comment.
Review
- Summary: no blocker and no major. The deploy change is coherent and the guard genuinely catches the two regressions it was built for — I re-added
pull_policy: alwaysto the template and--pull alwaysto the(Re)startin throwaway copies and both turn the suite red, while a legitimate second gated pull (when: bot_image_pull | default(false) | bool) stays green. Five smaller items inline, mostly guard coverage and doc claims the code doesn't quite deliver. - Must fix before merge: —
- Risks/questions: medium — deploy semantics change for every already-provisioned host on the first run after merge, and no playbook was ever executed (not even
--syntax-check), with no ansible-lint/yamllint in CI to catch an Ansible-level mistake. The task file itself reads correct: the fetch is ordered before the(Re)start,image_fetchis handled in both the skipped and the undefined case, and a failed fetch aborts before anything is recreated, so a bad opt-in run leaves the old containers up rather than a half-migrated host. I could not construct a post-merge scenario that leaves a host worse off than before the PR. - Inline comments: 5
Also checked, no finding
- Ordering/idempotency: compose file and
.envare rendered before the fetch, so the first post-merge run writes a template withoutpull_policy: alwaysbefore theupthat would have honoured it. Registry unreachable with the defaultbot_image_pull=falseis now a successful run where it used to fail — a strict improvement. .dockerignore/ path filters: the Dockerfile builds./cmd/flock-telegramand never runsgo test, so dropping**/*_test.gofrom the context cannot change the image (one cache-busting rebuild after merge, then stable).ci.ymlhas nopaths:filter, so the new guard test runs on every PR — the gate is not self-excluding.ansible.cfg, prune unit, 8 translations: all consistent with the role; the 7 non-English READMEs are faithful renderings of the English bullet.
Two nits not worth an inline
changed_whenon the fetch task readsimage_fetch.stderr + image_fetch.stdout. If the async job hits the 1800s cap the result carries neither key, so the run dies on'dict object' has no attribute 'stderr'instead of the real "async task did not complete" message.| default('')on both would keep the useful error. The pre-existingcompose_upchanged_whenhas the same shape, so this is the established pattern rather than something this PR introduced.--checkwas already broken before this PR (until: bot_state.stdout == "running"on a command task that check mode skips), and stays broken. Worth knowing before anyone tries the--checksuggested in the residual-risk list — it will fail on the wait task, not on the new ones.
| } | ||
|
|
||
| // Every file that decides whether a deploy may replace the running image. | ||
| var deployManifests = []deployManifest{ |
Contributor
Author
There was a problem hiding this comment.
MINOR: the manifest set misses the one shipped file that can silently revert this PR
- Problem: the comment above calls this "every file that decides whether a deploy may replace the running image", but
deploy/inventories/example/group_vars/all/vars.ymlis tracked, is the file the README tells operators tocp -r, andgroup_vars/allbeats role defaults. I appendedbot_image_pull: trueto it and ran the suite: still green. Every inventory derived from the example would then fetch on every ordinary run again — exactly the defect this PR removes — with nothing in CI catching it, becauseTestBotImagePullIsOptInreads onlydefaults/main.yml. - Solution: have
TestBotImagePullIsOptInalso readdeploy/inventories/example/group_vars/all/vars.ymland assert it either does not setbot_image_pullat all or sets it to a false value.
Contributor
Author
There was a problem hiding this comment.
Confirmed and fixed in fa273b4 (+ d60d6ce).
Reproduced your mutation first: bot_image_pull: true appended to deploy/inventories/example/group_vars/all/vars.yml left the suite green.
TestBotImagePullIsOptIn no longer reads a single file. It walks the shipped deploy tree over an fs.FS and asserts that every shipped assignment of the knob is a false value, plus that defaults/main.yml still defines one. Walking rather than listing was deliberate: group_vars, host_vars, a second inventory and a play's own vars: all outrank a role default, so a hardcoded list is one forgotten file away from the same hole. Both syntaxes count — a YAML mapping key and an INI [group:vars] entry — and comments are stripped first, so the -e bot_image_pull=true quoted in the role's own docs is not read as an assignment. Inventories other than example are skipped: .gitignore keeps them out of the repo, and an operator's local copy is not ours to police.
Mutation results, each applied to the tree and reverted:
| mutation | verdict |
|---|---|
bot_image_pull: true in inventories/example/group_vars/all/vars.yml |
FAIL, names the file and line |
bot_image_pull: false there |
pass (an explicit opt-out is legitimate) |
| key absent there | pass |
bot_image_pull=true in inventories/example/inventory.ini |
FAIL |
vars: {bot_image_pull: yes} in playbook.yml |
FAIL |
bot_image_pull: false deleted from defaults/main.yml |
FAIL, "no bot_image_pull default" |
bot_image_pull: true in an untracked inventories/mybot/ |
pass (gitignored, not shipped) |
host_vars/ does not exist in the repo, so nothing was invented for it — the walk covers it the day it appears. The comment on deployManifests no longer claims to be "every file that decides whether a deploy may replace the running image"; it now says it is the set of files that can NAME a pull directive, and points at this test for the other half.
| --- | ||
| # Convenience handler (e.g. for `--tags restart`). The main converge task | ||
| # already runs `docker compose up -d --build`, which is idempotent. | ||
| # Convenience handler (e.g. for `--tags restart`). The converge task |
Contributor
Author
There was a problem hiding this comment.
MINOR: the rewritten comment still advertises an entry point the role does not have
- Problem: the second half was corrected, but
--tags restartwas kept.grep -rnE '^\s*(tags|notify|listen)\s*:' adapters/telegram/deploy/returns nothing — no task carries arestarttag and nothing notifies this handler, soansible-playbook --tags restartselects zero tasks and this handler cannot run by any route. Tagging the handler would not help either: a handler still only fires when notified. Under AC5 this is precisely the class of claim the PR set out to remove. - Solution: either drop the
--tags restartexample and note the handler is currently unreachable, or add a small task withtags: ["restart"]runningdocker compose restart, so the documented invocation actually exists.
Contributor
Author
There was a problem hiding this comment.
Confirmed and fixed in b1a746e.
grep -rnE '^\s*(tags|notify|listen)\s*:' adapters/telegram/deploy/ returns nothing here too — no task carries a tag, nothing notifies the handler, so it is unreachable by any route and tagging it would not change that.
Took the first of your two options: the comment now states the handler is not reachable as the role stands and says why neither --tags restart nor tagging it would help, that it is kept as the restart entry point for a task that comes to need one, and that nothing is lost meanwhile because converging already restarts the stack idempotently. Adding a real tagged restart task would introduce a second, partly overlapping update entry point, which defaults/main.yml explicitly argues against a few lines above — out of scope for this PR. Deleting the handler is also out of scope here; flagging it as a candidate for a follow-up rather than doing it under this branch.
| # - the host no longer having an image for the tag — the first install, or a `compose down` | ||
| # that left it unreferenced until the weekly prune reclaimed it. Compose fetches what is | ||
| # missing in order to start the stack, which resolves the tag against the registry again. | ||
| # Pin a tag (e.g. :v1) to make the version explicit in the inventory; to move a |
Contributor
Author
There was a problem hiding this comment.
MINOR: on a pinned tag -e bot_image_pull=true is not the way forward — it is a no-op
- Problem: the two clauses point at different update paths but read as one instruction. If
bot_imageis pinned to an immutable tag the tag never moves, so the opt-in fetch downloads nothing and the running version does not change; the operator gets the new "nothing downloaded" report and reasonably concludes the switch is broken. The real move on a pinned tag is to editbot_imageand run normally — compose's defaultmissingpolicy fetches the now-absent image, no flag needed.bot_image_pullonly means anything for a floating tag, which is the case the rest of this block describes. - Solution: split it — pinned tag: change
bot_imageand re-run; floating tag: re-run with-e bot_image_pull=true.
Contributor
Author
There was a problem hiding this comment.
Agreed and fixed in 552d9af (READMEs in 86fc150).
The two routes are now written as two, and the block says which one applies when:
- PINNED to an immutable tag: edit
bot_imageand run normally — the new tag has no image on the host, so the (re)start fetches it. The switch is explicitly called out as not the tool here: a pinned tag does not move, so the fetch reports nothing downloaded and the running version does not change. - FLOATING (
:latest,:v1): the tag has to be re-resolved, which is what-e bot_image_pull=truedoes.
Also added one clause to the bot_image_pull block itself, since the switch is not a total no-op on a pinned deployment: the pinned bot tag resolves to the image already present and downloads nothing, while the floating sidecar tags (dind, Caddy) are still re-resolved.
The 8 READMEs had the same conflation — "to update, re-run with -e bot_image_pull=true; set bot_image to pin a tag" — so they carry the same distinction now, one clause each in their own language.
| // stripComment removes a YAML end-of-line comment: a "#" that starts the line or follows | ||
| // whitespace, and is not inside a quoted scalar. Quote tracking keeps a "#" that is part | ||
| // of a value (e.g. a colour or a fragment URL in quotes) from truncating a real directive. | ||
| func stripComment(line string) string { |
Contributor
Author
There was a problem hiding this comment.
NIT: two false positives that invite deleting the guard rather than fixing it
- Problem: both reproduced against the current tree. (a) The quote tracking is fooled by an apostrophe in an unquoted YAML scalar — the quote never closes, so the trailing
#is not stripped andmsg: Updating is the operator's call # not a docker compose pullfails the build for a comment stating the directive is not used. (b) Any informational line naming the manual command trips the guard: appending- "to update by hand: docker compose pull && docker compose up -d"to theReport statusmsg list — a very natural follow-up to this PR — turns CI red. - Solution: (a) if a quote is still open when the scan reaches end of line, fall back to cutting at the first whitespace-preceded
#; (b) skip lines that belong to a task whose module isansible.builtin.debug.
Contributor
Author
There was a problem hiding this comment.
Both reproduced and fixed in 1382586.
Proof before the fix, applied to the real role file: adding a Say what the role does not do debug task with msg: Updating is the operator's call # not a docker compose pull, plus - "to update by hand: docker compose pull && docker compose up -d" in the Report status msg list, made TestDeployNeverPullsImplicitly report both lines. Same tree, new guard: green. Both cases are pinned as tests.
(a) Deviated slightly from your suggestion, and it is worth flagging. Falling back to "cut at the first whitespace-preceded # when a quote is left open" fixes your line, but on cmd: echo "a # b" && docker compose pull combined with an apostrophe it cuts inside the quoted string and hides a real directive — a false negative, which for a guard is the more expensive mistake. Instead a quote now only opens a scalar where one can begin (line start, or after whitespace or a flow punctuator), which is what YAML actually does: an apostrophe inside a word is a literal character, so nothing is left open and the comment is cut normally. Both directions are pinned: msg: Updating is the operator's call # not a docker compose pull must not match, and cmd: echo "it's # here" && docker compose pull must still match.
(b) Lines belonging to a debug task are skipped — it executes nothing, so its msg is the role talking about a command rather than running one. Attributing such a line to its task needed one more change: the walk-back to the enclosing task now steps over list items that hold scalars, because a msg list entry is a value, not a task. Three cases pin it: the FQCN form, the short debug: form, and a command task quoting the same instruction, which is still reported.
Honest limitation, and it is now in the docblock: skipping the whole task rather than only its msg means a vars: on a debug task is unscanned too, so a command smuggled through a lookup('pipe', …) there would pass. The guard catches accidents, not evasion — it already said it is not an evaluator. The alternative was scanning vars: and having the guard fire on the operator-facing prose that lives there today, which is the failure mode you flagged.
Regression checked in the same round: --pull always back on the (re)start task and pull_policy: always back in the compose template both still fail the guard.
| - **dind sidecar:** `docker compose --profile dind up -d` gives the team dockerized linters/tests (set `DOCKER_HOST=tcp://dind:2375`). Ansible deploys enable dind by default and inject `DOCKER_HOST` automatically. | ||
| - **Per-chat isolation:** each chat gets `/workspace/chat_<id>` (1:1 → private; group → one shared workspace); chats are fully isolated and run in parallel, capped by `MAX_CONCURRENT_CHAT_RUNS`. In groups, set `REQUIRE_GROUP_MENTION=true` to respond only when @mentioned or replied to. | ||
| - **Ansible deploy** (Telegram): one-command VPS provision from `adapters/telegram/deploy` — copy `inventories/example` to your own `inventories/<name>/` (gitignored), fill inventory/vars/vault, then `ansible-playbook -i inventories/<name>/inventory.ini playbook.yml`. The role pulls the prebuilt image; set `bot_image` to pin a tag. | ||
| - **Ansible deploy** (Telegram): one-command VPS provision from `adapters/telegram/deploy` — copy `inventories/example` to your own `inventories/<name>/` (gitignored), fill inventory/vars/vault, then `ansible-playbook -i inventories/<name>/inventory.ini playbook.yml`. The role installs the prebuilt image on the first run and then keeps it: an ordinary re-run doesn't go looking for a newer build — it restarts the container on the image that tag already points to on that host. To update, re-run with `-e bot_image_pull=true`, which refreshes the whole stack (bot, dind, Caddy) and recreates the containers; set `bot_image` to pin a tag. |
Contributor
Author
There was a problem hiding this comment.
NIT: the 8 READMEs are the only surface that states this without its exception
- Problem:
defaults/main.yml,docker-compose.yml.j2anddocker-prune.service.j2each name the case where an ordinary run still fetches — the host having no local copy of that tag left. All 8 READMEs say "an ordinary re-run doesn't go looking for a newer build" unqualified, which is the same class of absolute claim the earlier rounds removed everywhere else, on the most operator-facing surface there is. Secondary: "(bot, dind, Caddy)" — Caddy is only in the stack whenwebhook_domainis set, dind only whenenable_dind. The 7 translations are otherwise faithful to the English; I compared them line by line. - Solution: one clause — "…unless this host no longer has a copy of that tag" — mirrored into the other 7 files.
Contributor
Author
There was a problem hiding this comment.
Fixed in 81b26b3, and the pinned-tag point in 86fc150.
All 8 READMEs now carry the exception in their own language: the ordinary re-run restarts on the image the tag already points to locally unless this host no longer has a copy of that tag (first install, or a compose down plus the weekly prune), where Compose fetches it again in order to start the stack — matching the wording in defaults/main.yml, the compose template and the prune unit.
Secondary point taken as well: "(bot, dind, Caddy)" now reads as the bot plus the sidecars in use, since Caddy only exists when webhook_domain is set and dind when enable_dind is.
Swept the whole set afterwards (8 READMEs, defaults/main.yml, tasks/main.yml, handlers/main.yml, both templates, both publish workflows, the guard test): no unqualified claim about pulling or updating is left — every remaining "never" is about unrelated things (the whitelist advice, the arbiter's loop cap). The workflows already stated "or when the host has no copy of the tag left to start from", so they agree.
The guard read bot_image_pull only from the role defaults, which is the weakest source Ansible has. The example inventory's group_vars/all/vars.yml — the directory the README tells operators to copy — outranks it, so setting the switch to true there would restore the implicit update in every derived deployment while the default still read false and the suite stayed green. Walk the shipped deploy tree instead of listing files: group_vars, host_vars, a second inventory and a play's own vars: are all sources that outrank a default, and a fixed list is one forgotten file away from letting the switch back on. Both YAML and INI spellings count as an assignment; comments are stripped first so the update command quoted in the role's own documentation is not mistaken for one. Inventories other than the example are gitignored and stay the operator's business.
Two false positives, both reproducible against the role as it stands. A quote was tracked from anywhere in a line, so an apostrophe in an unquoted scalar opened a scalar that never closed and the trailing comment was never cut: a sentence saying the role does NOT run a pull failed the build. And a debug msg naming the manual command — the natural place to tell an operator how to update by hand — was read as a directive. A guard that reddens CI over prose is a guard people delete rather than fix, so both are worth closing. A quote now only opens a scalar where one can begin (line start, after whitespace or a flow punctuator), which leaves an apostrophe inside a word as the literal character YAML reads it as. Falling back to "cut at the first # once a quote is left open" would have been simpler but can cut inside a quoted string and hide a real directive, which is the more expensive mistake. Lines belonging to a debug task are skipped: debug executes nothing, so its msg is the role talking about a command, not running one. Attributing a line to its task needed the walk-back to step over list items that hold scalars — a msg list entry is a value, not a task.
The handler's comment offered `--tags restart` as a way to invoke it, but no task in the role carries a tag and nothing notifies the handler, so that command selects zero tasks and the handler cannot run by any route. Tagging the handler would not change that — a handler fires on a notify, not on a tag selection. Say what is actually true: the handler is unreachable today and kept for a task that comes to need it, while converging already restarts the stack idempotently. Same class of claim this branch removes elsewhere — a documented invocation that does not exist costs an operator a debugging session.
The two clauses pointed at different mechanisms but read as one instruction: pin a tag, then re-run with -e bot_image_pull=true to move forward. On a pinned tag that fetch is a no-op — the tag does not move, so nothing is downloaded and the running version stays put, while the run's own report says "nothing downloaded" and the operator concludes the switch is broken. Spell the routes out separately: a pinned tag moves by editing bot_image and running normally, because the new tag has no image on the host and the (re)start fetches it; the switch is for a floating tag, which has to be re-resolved against the registry. Also say what the switch does on a pinned bot_image, since it still refreshes the sidecars, whose tags float.
The 8 READMEs were the only surface left stating "an ordinary re-run doesn't go looking for a newer build" without its exception, which the role defaults, the compose template and the prune unit all name: a host can be left with no copy of the tag — a first install, or a `compose down` whose image the weekly prune then reclaimed — and compose fetches it again to have something to start. Being the most operator-facing surface, that is the worst place for the unqualified version. The same sentence also listed the refreshed stack as "bot, dind, Caddy"; Caddy only exists when webhook_domain is set and dind when enable_dind is, so it now reads as the sidecars in use. All 8 languages carry both corrections.
A `poll:`-ed async job that runs past its 1800s budget comes back without stdout or stderr, so the changed_when added with this task raised "'dict object' has no attribute 'stderr'" and buried Ansible's own "did not complete within 1800s". An operator whose registry is slow got a Jinja error about a dict instead of the timeout that caused it. Defaulting both to an empty string leaves the normal path untouched and lets the real failure through.
The READMEs offered the opt-in switch as THE update path and mentioned pinning in the same breath, which leaves an operator on a pinned tag re-running with the switch and watching nothing happen — the tag has nothing newer to resolve to. The role defaults now separate the two routes; the 8 READMEs carry the same distinction so the operator-facing surface does not contradict them.
Reading walked paths with os.ReadFile trips gosec G304 (file inclusion via a variable) and fails the lint gate. Rooting the walk at an fs.FS reads through fs.ReadFile instead, which is not a filesystem-wide open, and confines the scan to the deploy tree by construction rather than by convention. Paths in the failure messages are unchanged.
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.
Summary
An Ansible deploy updated the bot as a side effect of any playbook run. Re-running the
playbook to rotate a token, change
webhook_domain, toggleenable_dindor adjust a rate limitsilently swallowed everything merged into
mainsince the last converge — and the operator got nosignal that the running version had changed.
Two independent mechanisms caused it, and both had to go:
docker compose up -d --pull always --force-recreatein the role's converge task;pull_policy: alwaysin the rendereddocker-compose.yml— which would have kept the behaviouralive on any
up, including a manual one on the host and the--tags restarthandler.After this change a deploy behaves like an ordinary container deployment: it starts the stack on the
image the host already has and does not go looking for a newer build. Moving to a new version is an
explicit act.
A host reboot was already safe (
docker restartdoes not re-resolve a tag) — that was verified, notassumed; the defect was specific to
compose up.Related issue
None.
Type of change
What changed, per acceptance criterion
--pull alwaysandpull_policy: alwaysare gone. A guard test fails if either returns, in any spelling(
--pull=always, argv form,pull: always,docker pull,docker compose pull,community.docker.docker_compose_v2), acrosstasks/main.yml,handlers/main.yml,playbook.yml, the shipped compose file and the template.bot_image_pull(defaultfalse)gates a
docker compose pulltask placed before the(Re)start, so a fetched image isactually put into service:
ansible-playbook … -e bot_image_pull=true. The guard accepts a pull only when the pulltask's own
when:names that knob — a condition inherited from an enclosingblock:does notcount, and the knob's default is pinned to a false value.
pull_policyoverride and nobuild:section, soCompose's default
missingpolicy fetches an absent image on the firstup. Verified on liveDocker, not inferred.
org.opencontainers.image.versionwasthe literal string
latest, becausedocker/metadata-actiontakes the version from thehighest-priority tag and
type=raw,value=latestwon by default. The immutable0.1.<run_number>now carriespriority=250— abovelatest(200), clear of the tie withsha(100), and still below avX.Y.Ztag ref (600). The set of published tags is unchanged;:latestis still published.both templates and both workflows were corrected and cross-checked against each other.
How it was verified
Beyond the gate, the load-bearing claims were checked against reality rather than from memory:
up -d --force-recreatere-landed it on the new image — reproducing the defect;docker restartdid not, which is why reboots were never the problem.
docker image prune --filter until=filters by image creation time, not pull time(docs.docker.com). The role's comment claimed the opposite, which mattered once the local store
became the source of truth. Corrected.
docker/metadata-action@v6priorities read from the action's own README and sources.changed_whenfor the fetch report was chosen after capturing real non-TTYdocker compose pulloutput: an up-to-date run printsPulling/Pulledper service but zeroPull complete, so presence of output proves nothing. The flag keys onPull complete, and wasrendered through real Ansible (core 2.21) in four states — never ran / skipped / ran-current /
ran-downloaded — including with
async+poll.old-vs-new comparison proving it no longer flips verdict on YAML key order.
yaml.safe_loadand the report expression rendered throughJinja, since CI has no ansible-lint or yamllint.
Pre-PR review
Three adversarial rounds (round 1 with two independent critics: operational risk, and CI/consistency).
No round found a blocker. Rounds 1 and 2 returned CHANGES_NEEDED; round 3 was clean and an
arbiter verified the result independently, including its own guard-test mutations. What the critics
caught and fixed:
silent no-op: the operator saw
changed/bot is runningand stayed on the old image with zerofeedback. That is what the
bot_image_pullopt-in exists for.plain
docker compose pulltask), then — after widening — returned opposite verdicts forsemantically identical YAML depending on whether
when:sat before or afterblock:, andaccepted
when: true. Now it checks the gate structurally, at the task's own key indentation."
:latesttracks main", then "an ordinary re-run never changes the running version", then "nevercontacts the registry". Each had a reachable counterexample. Every operator-facing statement now
names its exceptions instead.
dindsidecar on a floating tag with no update path at all — it used to be refreshed by the
--pull alwaysthis PR removes. The switch now refreshes the whole stack.Residual risks — what needs a human
--check. Thewhen:gate,register/skippedhandling,changed_whenand theasync+pollpairing are verified byreading, by static guard and by rendering — not by running. CI has no ansible-lint or yamllint,
so nothing catches Ansible-level mistakes. Worth one
ansible-playbook --syntax-check.compose down→the weekly prune reclaims the now-unreferenced image → the next ordinary run re-resolves the
floating
:latestand lands on a newer build. It is documented in three places but onlyclosed by pinning
bot_imageto an immutable tag (:0.1.<n>or:sha-…). That default isyour call — this PR deliberately leaves
bot_imageon:latest.usually gone by the next run; rolling back means pulling again — and
:latesthas moved on.dindno longer patches itself. That is the intended consequence of "nothingupdates automatically", and the opt-in deliberately refreshes the whole stack rather than the bot
alone — but the cadence for security updates is now a human responsibility.
org.opencontainers.image.versionis0.1.<run_number>rather thanlatest, and that the new!**/*_test.gopath filter did notskip a build that should have run. GitHub's path-filter semantics were taken from documentation,
not observed live.
publish-*.ymlcombinestags: ["v*"]withpaths:. A tagpushed to an existing commit generally yields no changed files, so that release path may never
have triggered. Flagged rather than fixed — it needs a live check.
changed_whenparses human-readable Compose output (Pull complete), so it is version- andlocale-sensitive, and misses a tag re-pointed at layers already present. Reporting only; it does
not affect correctness.
publish-vk.ymlis changed symmetrically even though VK has no Ansible deploy — the label fixapplies to both images, but VK publishing behaviour rides along with this PR.
Checklist
task lint,task tests, andtask buildpass locally