fix: ensure deterministic variable interpolation#860
fix: ensure deterministic variable interpolation#860yunus25jmi1 wants to merge 3 commits intocompose-spec:mainfrom
Conversation
TODO: Remove replace once compose-go#860 is merged See: compose-spec/compose-go#860 Signed-off-by: Md Yunus <[email protected]>
interpolation/interpolation.go
Outdated
| out := map[string]interface{}{} | ||
|
|
||
| for key, value := range config { | ||
| keys := make([]string, 0, len(config)) |
There was a problem hiding this comment.
perhaps the slices and maps packages could be used here; something like;
keys := maps.Keys(m)
slices.Sort(keys)There was a problem hiding this comment.
Oh! That won't work because maps.Keys returns an iterator; this probably works though;
keys := slices.Sorted(maps.Keys(config))|
Thanks for the suggestion @thaJeztah! I've updated the code to use Changes:
All tests pass. |
|
Ah, nice, thanks! There was nothing wrong with your original implementation (and in some cases open-coding these things could still be used for performance critical paths) but the slices and maps packages can make the code cleaner, easier to understand and maintain, which is a big plus :-) |
|
@ndeloof review the PR. |
|
I think Nicolas is off for a few days, but a maintainer will probably review this change after the weekend / after Easter. FWIW; (I haven't dug deep into the code) I think this change would make failures more deterministic, but I think there may still be situations where it could fail even though technically it's valid; this PR sorts the env-vars in a deterministic way, but expansion may fail if an expansion depends on another env-var that's sorted "later"? Haven't tested it yet, but considering something like this; - ${AAAA:-${BBBB:-${ZZZZ}}}
- ${BBBB:-${ZZZZ}}
- ZZZZ=defaultThose are sorted, but if those are interpolated in alphabetic order, they would likely still fail. Basically;
So the correct order would be to expand them in topological order;
|
This fixes the non-deterministic behavior when multiple required
variables (e.g., ${TIMEZONE:?}, ${TRAEFIK_ACME_PATH:?}) are missing.
Go's map iteration order is randomized, causing different error messages
to be reported on each run.
By sorting keys before iteration, we ensure consistent error reporting.
Fixes: docker/compose#13712
Signed-off-by: Md Yunus <[email protected]>
Refactored to use slices.Sorted(maps.Keys()) as suggested by reviewer. This is more idiomatic Go than manual slice creation and sorting. Fixes: docker/compose#13712 Signed-off-by: Md Yunus <[email protected]>
…ple) Signed-off-by: Md Yunus <[email protected]>
|
Thanks @thaJeztah for the suggestion! I've updated the code to use I also investigated your comment about nested variable dependencies (e.g., I've added test cases to verify this behavior. Let me know if there are any other changes or suggestions needed before the PR can be merged! |
|
Hi @ndeloof @thaJeztah and maintainers, This PR is ready for review. All feedback from @thaJeztah has been addressed:
This fix is aligned with docker/compose PR #13713 (docker/compose#13713) which addresses the same non-deterministic variable interpolation issue. Once this PR is merged, we can update docker/compose to use the new compose-go version with the fix. Please let me know if any additional changes are needed! Signed-off-by: Md Yunus [email protected] |
Description
This fixes the non-deterministic behavior when multiple required variables (e.g., ${TIMEZONE:?}, ${TRAEFIK_ACME_PATH:?}) are missing in docker-compose.yaml.
Root Cause
Go's map iteration order is randomized. When iterating over service environment variables or volumes during interpolation, different keys can be processed in different orders on each run. This causes the error message for missing required variables to vary between runs.
Fix
Added sort.Strings(keys) before map iteration in two places:
This ensures deterministic variable interpolation order, so the same missing variable is always reported first.
Testing
Related
docker compose config --quietproduces non-deterministic result for required variables docker/compose#13712