Conversation
icechunk normalizes container prefixes and credential keys by appending a missing trailing slash (append-only: a doubled slash is preserved and simply never matches). parse_chunk_access stored entries verbatim and earthdata_endpoints compared them to declared container prefixes with exact string equality, so a config entry differing from the declared container only by a trailing slash would have its credential applied by icechunk for virtual reads while endpoint priming silently skipped it (or, conversely, primed an endpoint for a credential icechunk would never apply). Normalize entry keys at parse time exactly like icechunk's add_trailing and normalize declared prefixes the same way before the membership test, so priming reproduces icechunk's matching byte for byte. Extracted from #151 (commits b4ce0c0 and 2328f14): this fixes EDL behavior already on main and is independent of the where= feature. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
chuckwondo
left a comment
chuckwondo
left a comment
There was a problem hiding this comment.
Only some "nits", so nothing blocking, but please take a bit more time to review and adjust agent-generated comments for better clarity.
| # icechunk appends a missing trailing slash to container prefixes and | ||
| # credential keys (append-only — a doubled slash is preserved and | ||
| # simply never matches); entries were normalized the same way at parse | ||
| # time, so a plain membership test reproduces icechunk's matching |
There was a problem hiding this comment.
Be mindful of agent tendency for sometimes less than crystal clear wording, along with overuse of en- and em-dashes and semicolons.
hrodmn
left a comment
hrodmn
left a comment
There was a problem hiding this comment.
LGTM, although having path parsing/sanitizing logic so deep in the application code feels less than ideal. I'm not familiar with how the new EDL + Icechunk system works so maybe there's not way around it.
Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>
|
thanks for your critical feedback @chuckwondo. I'll be more careful in the future |
|
@chuckwondo @hrodmn, sorry for the churn but after further consideration I think #163 is a better approach for this problem. Please let me know if you agree or disagree. |
|
superseded by #163 |
3 participants
This PR isolates a small fix from #151 that makes sure the earthdata endpoints contain a trailing slash, so that Icechunk can properly match the prefix.