Repository navigation
feat(ansible): scope check/deploy to a subset of hosts with LIMIT - #40
Merged
Merged
Conversation
`make check` / `make deploy` hardcoded the playbook invocation, so the moment
`decdn_nodes` holds more than one host every run was fleet-wide with no
supported way to say "just this node". That is the wrong default for bringing
up a new node (whose first converge creates the operator account and lets
ssh_hardening disable root login) and for re-running one node after a config
change or a failed play. The workaround was dropping to a raw ansible-playbook
command, which silently loses any flags the targets grow later.
Add two optional, empty-by-default variables to both targets:
make deploy LIMIT=decdn-node-1
make deploy LIMIT='decdn-node-1,decdn-node-2'
make deploy LIMIT=decdn-node-1 ANSIBLE_ARGS='--tags decdn_node -vv'
Defaults are byte-identical to before, so existing callers are unaffected.
The --limit value is single-quoted, unlike the sketch in the issue: unquoted,
`LIMIT='*'` glob-expands against ansible/ before ansible ever sees it.
Per AGENTS.md rule 4 ("fail loud"), guard the two ways a run can look scoped
but silently be fleet-wide -- an exported LIMIT/ANSIBLE_ARGS reaching make
from the operator's shell (`?=` does not override the environment), and a
LIMIT that expands empty (`make deploy LIMIT=$NODE` with NODE unset, which
drops --limit entirely and converges the whole fleet). Both now abort at parse
time, scoped to check/deploy via MAKECMDGOALS so `make lint` is never blocked.
An explicit command-line LIMIT still wins over the environment.
Not fixed, documented instead: a well-formed argument that selects nothing
exits 0. `--tags decdn-node` (hyphen, vs the real `decdn_node`) runs zero
tasks and reports success, as does a --limit matching a host outside
decdn_nodes. A malformed flag fails loudly; these do not. An empty-PLAY-RECAP
gate is a behaviour change beyond this issue's scope -- see the "Watch the
PLAY RECAP" callout in ansible/README.md.
Closes #38
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WaE3ZP2xQdjd9juzRYKLwi
There was a problem hiding this comment.
🟡 Changes recommended
A new Makefile error hint can misdirect make check users toward running make deploy, which is risky for an infrastructure repo.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds safe, optional host scoping to the Ansible check/deploy Makefile targets so operators can converge a single node (or subset) without falling back to raw ansible-playbook commands, while keeping fleet-wide behavior as the default.
Changes:
- Introduce
LIMIT(scoped--limit) andANSIBLE_ARGS(extra flags passthrough) formake checkandmake deploy, including parse-time “fail loud” guards. - Document scoped usage patterns and shell-quoting pitfalls in Ansible and repo-level docs.
- Add brief one-line usage reminders to top-level
README.mdandAGENTS.md.
File summaries
| File | Description |
|---|---|
ansible/Makefile |
Adds LIMIT/ANSIBLE_ARGS support and guardrails to prevent accidental fleet-wide runs. |
ansible/README.md |
Documents scoped check/deploy flows, quoting guidance, and operational gotchas. |
README.md |
Adds a short note that LIMIT scopes and ANSIBLE_ARGS passes through. |
AGENTS.md |
Mirrors the same short note for contributor guidance. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The parse-time guard fires for both check and deploy, but its hint hardcoded `make deploy LIMIT=<host>` -- so an operator who hit it during a `make check` dry-run was pointed at the mutating target. Echo $(MAKECMDGOALS) instead. The other two guard messages name no target and were already goal-agnostic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WaE3ZP2xQdjd9juzRYKLwi
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 #38.
Problem
ansible/Makefile'scheckanddeploytargets hardcoded the full playbook invocation, so the momentdecdn_nodesholds more than one host every run is fleet-wide with no supported way to say "just this node". That is the wrong default for the two most common operations — bringing up a new node (whose first converge creates the operator account and letsssh_hardeningdisable root + password login) and re-running one node after a config change, a failed play, or a binary bump. The workaround was dropping to a rawansible-playbook … --limit fra1command, which silently loses any flags the make targets grow later.Change
Two optional, empty-by-default variables on both targets:
Defaults are byte-identical to before —
make -n check/make -n deployrender the original commands — so existing callers are unaffected.The
--limitvalue is single-quoted, unlike the sketch in the issue. Unquoted,LIMIT='*'glob-expands againstansible/before ansible ever sees it; the quoting is load-bearing.Fail-loud guards
Per AGENTS.md rule 4, the two ways a run can look scoped but silently be fleet-wide now abort at parse time:
make deploy LIMIT=$NODE(unset)*** LIMIT was supplied but expanded empty … refusing to run fleet-wide by accidentexport LIMIT=x; make deploy*** LIMIT is set in your environment (x) and would silently scope this runexport ANSIBLE_ARGS=--check; make deploy*** ANSIBLE_ARGS is set in your environment and would silently alter this runenv LIMIT=x make deploy LIMIT=decdn-node-1?=is kept deliberately — switching to=would make$(origin LIMIT)returnfileand silently disarm the environment guard. Guards are scoped tocheck/deployviaMAKECMDGOALS, so an unrelatedmake lintis never blocked.Known gap (documented, not fixed)
A well-formed argument that selects nothing exits 0:
--tags decdn-node(hyphen, vs the realdecdn_node) runs zero tasks and reports success, as does a--limitmatching a host outsidedecdn_nodes. Malformed flags fail loudly; these do not. Live risk because this repo mixes separators constantly (decdn-node.service, hostdecdn-node-1, roledecdn_node). An empty-PLAY-RECAP gate is a behaviour change beyond this issue's scope — there is a "Watch the PLAY RECAP" callout inansible/README.mdand aNOTE:in the Makefile instead. Happy to file a follow-up issue.Also out of scope per the issue: requiring an explicit
LIMITor typed confirmation for a fleet-wide deploy.Verification
All four guards verified firing (exit 2) and both happy paths verified unchanged.
ansible-lintpasses at production profile (0 failures, 42 files); pre-commit clean. A livemake checkagainst real hosts was not run — no reachable node from here, and no YAML in either role changed.Empirically checked while reviewing: a typo'd
--limitdoes not fail silently — ansible exits 1 with[ERROR]: … leaves us with no hosts to targeton both 2.21.3 and the repo's documented 2.15 floor.Docs
ansible/README.mdgets theLIMITsection with the new-node case; one-line mirrors inREADME.mdandAGENTS.md. Note this also corrects a pre-existing-in-this-branch error caught in review: the converge does not editansible_user— no role writes it — so the text now points at Setup for the manual switch rather than crediting it to automation.🤖 Generated with Claude Code
https://claude.ai/code/session_01WaE3ZP2xQdjd9juzRYKLwi