Repository navigation
fix(config): restrict list-valued keys, teach cache.root consumers (0.22.2) - #181
Merged
Merged
Conversation
….22.2)
`remote.root` became a list of four roots at some point. Every dt older than
0.20.0 reads it with get_value and calls Path() on the result, so that one
config edit retroactively broke 26 already-published container images. Each
aborts on startup with a bare TypeError, and being immutable, none of them can
be fixed. The modulefiles now refuse to load with an explanation instead.
The same trap is still live for cache.root, which is one line away in the same
function. `dt config add` accepted *any* key -- there was no whitelist -- so
`dt config add cache.root /somewhere` is an ordinary thing to do by analogy
with remote.root, and it would have produced the identical failure. Confirmed
by probe: _discover_dt_config quietly yields Endpoint.url as a list, which then
raises in classify_url ("'list' object has no attribute 'strip'") and in
check_endpoints (TypeError), a long way from the cause.
Two changes, at both ends:
**Writing.** add_to_list now refuses keys outside LIST_VALUED_KEYS
(remote.root, cache.root, secrets.gcp.locations), and the CLI surfaces that as
a usage error. This is the end that matters: a config outlives the code that
reads it, so guarding only the reader leaves every released version exposed.
**Reading.** The three cache.root consumers -- endpoint discovery, cache path
resolution, and the doctor check -- go through get_str_list. Discovery emits
one endpoint per root, since a second cache is worth checking; the other two
take the first, matching remote.root's documented convention that the first
entry is the one used when creating something new. A scalar still behaves
exactly as before.
Note `dt config set` is deliberately not restricted: it cannot tell a YAML list
from a string that looks like one, and forcing a list is a reasonable escape
hatch. Documented as such.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Follow-up to the
remote.rootincident: the same trap is still live one line away, and the guard was at the wrong end.What happened
remote.rootbecame a list of four roots at some point. Every dt older than 0.20.0 reads it withget_valueand callsPath()on the result, so that one config edit retroactively broke 26 already-published container images. Each aborts on startup with a bareTypeError, and being immutable, none of them can be fixed. Those modulefiles now refuse to load with an explanation instead.cache.rootis the same bug, not yet triggereddt config addaccepted any key — there was no whitelist. Sodt config add cache.root /somewhereis an ordinary thing to do by analogy withremote.root, and produces the identical failure. Confirmed by probe against the current code:Note the shape: the read succeeds.
get_valuehands the list on quite happily, and it detonates later, in another module, with nothing pointing back at the config. That is why this class of bug is expensive to diagnose.Three consumers were affected —
dt auth list/check/setupvia endpoint discovery,cache.resolve_cache_path, anddoctor.check_cache_root.The fix, at both ends
Writing.
add_to_listrefuses keys outsideLIST_VALUED_KEYS(remote.root,cache.root,secrets.gcp.locations); the CLI surfaces it as a usage error naming the keys that do take lists. This is the end that matters — a config outlives the code that reads it, so guarding only the reader leaves every already-released version exposed, exactly as happened here.Reading. The three
cache.rootconsumers go throughget_str_list. Discovery emits one endpoint per root, since a second cache is worth checking; the other two take the first, matchingremote.root's documented convention that the first entry is the one used when creating something new. A scalar behaves exactly as before.dt config setis deliberately not restricted — it can't tell a YAML list from a string that looks like one, and forcing a list is a reasonable escape hatch. Documented.Design decision worth a second opinion
I made
cache.rootlist-valued (first = primary) rather than rejecting lists outright. A DVC repo has one cache, so the alternative — refuse it and keepcache.rootstrictly scalar — is also defensible. I went with list-tolerant because it matchesremote.root, and because a config that already has a list should degrade rather than crash. Easy to flip.Tests
2169 passed, 2 skipped, 0 failed under PBS via
qxub. 24 new tests intests/unit/test_list_valued_config.pycovering both ends: which keys may become lists, that the CLI refuses the rest without a traceback, and that everycache.rootconsumer handles a list, a scalar, and an absent value.🤖 Generated with Claude Code