Fail loudly on a stray argument, an unresolvable fact, and a node subnet - #102
Merged
Conversation
Three ways a mistake stayed quiet.
A positional argument panicked. The usage library indexes its argument list
for every leftover it finds, and DART declares none, so `dart suite.yaml`
died with an index-out-of-range instead of a message. Flags are parsed before
the library runs, which makes the leftovers visible while they can still be
reported; a suite path given without -c now says exactly that.
A {{ fact ... }} reference in a suite that gathers no facts was passed
through as literal text. Fact templates were only rendered when some node
offered facts, so `command: echo v={{ fact "db" "ipv4" }}` with
`evaluate: {exit_code: 0}` ran a nonsense command and reported a pass. The
store is now always non-nil, so an unresolvable reference fails wherever it
appears, and the message names where facts come from. Teardown-only renders
against an empty store for the same reason.
subnet on a node's networks entry consumed nothing. A node attaches to an
existing bridge rather than defining one, so the value read as configuration
while changing nothing; it is now a configuration error pointing at
lxd.networks, which is what creates the bridge.
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.
Three more cases from the documentation sweep where a mistake stayed quiet.
1. A positional argument panicked
The usage library populates positional arguments by indexing its own argument list —
*s.arguments[len(s.arguments)-1]— and DART declares none, so any leftover argument indexes[-1]. The most likely mistake is a suite path given without-c, and it produced a stack trace.Flags are now parsed before the library runs, which makes the leftovers visible while they can still be turned into a message:
Note: the underlying bug is in
github.com/bgrewell/usage— a one-line guard on the empty-argument-list case would fix it for every consumer. Worth doing upstream; this change means DART no longer depends on it.2. A fact reference in a fact-free suite asserted nothing
Before:
Pass: 00001. The literal{{ fact "db" "ipv4" }}went into the command unrendered,echoexited 0, and the test reported success while asserting nothing about any address.The cause: fact templates were rendered only when
HasAnyFactswas true, which left the store nil in a suite oflocalnodes and skipped template processing entirely. The identical reference in a suite that did have facts failed hard — so the check existed, it just never ran when it was most needed.The store is now always non-nil, so an unresolvable reference fails wherever it appears, with a message naming where facts come from:
--teardown-onlyrenders against an empty store for the same reason — a literal{{ fact ... }}reaching a shell is not a useful outcome.3.
subneton a node'snetworksentry consumed nothingA node attaches to an existing bridge; it does not define one.
subnetthere was decoded and dropped, so the value read as configuration while changing nothing. It is now a configuration error pointing at the place that does create the bridge:Caught by
--check.nameandipremain valid on a node-level entry.Verification
New tests cover the empty-store failure and its message, that a populated store still renders, that an unknown node still names itself specifically, and — importantly — that text without fact references is untouched by an empty store, so always rendering cannot break suites that use no facts. Plus the subnet rejection and that
name/ipstill validate.All example suites still pass
--check;go build,go vet,gofmt, and the full test suite are green; the docs site builds. The three Notes describing these behaviours are replaced by what actually happens now.