You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
override: fix two merge regressions introduced in v2.15.0 - #937
Fixes two regressions of the override merge, both introduced in v2.15.0 and found by the buildx bake tests added in docker/buildx#4117 (they pass on v2.14.0 and fail on v2.16.0).
null override.networks: null, depends_on: null or models: null in an override failed with cannot convert <nil> into a mapping, since #915 made convertIntoMapping reject unknown types. null is a valid value that leaves the base in place, so it now converts to an empty mapping (not a nil one: a null base merged with an override listing entries would panic).
Last entry wins. Since #924, an override entry equal to one already present is dropped, which breaks KEY=VALUE lists where the last entry for a key takes effect: [MODE=debug, MODE=release] over [MODE=release] ended on debug (environment, labels, build.args, ...). These lists are now merged by key, the way EnforceUnicity does: an entry replaces the earlier entry of the same key in place, new keys are appended, and keys the base repeats collapse to their last value. It has to happen at merge time because EnforceUnicity ignores jobs and the labels of networks, volumes, secrets and configs, and the schema rejects repeated items there. Two empty lists merge into an empty list, not a nil one, for the same reason. Deduplication is unchanged for env_file, dns, tmpfs, extra_hosts, etc.
Keys are read up to the first =, as EnforceUnicity already does, so with interpolation disabled the merge works on the raw strings: ${K}=1 and ${K}=2 merge, while entries that only resolve to the same key after interpolation (${K1}=1, ${K2}=2, or a bare ${ENTRY}) are kept side by side, in order, so the last one still wins once interpolated. Results on services are identical to v2.16.0 in that mode.
Each fix has YAML-in/YAML-out tests in override/ and there is an end-to-end multi-file test in loader/tests. A separate commit fixes the three lint issues already present on main.
Fix the three issues golangci-lint reports on main:
- cli: put the Deprecated notice of ProjectFromOptions in its own
paragraph (gocritic deprecatedComment)
- loader: use reflect.Pointer instead of reflect.Ptr (govet inline)
- tree: drop redundant parentheses in matcher_test.go (gofumpt)
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
A null override of networks, depends_on or models failed with "cannot
convert <nil> into a mapping" since the conversion started rejecting
unknown types, although null is a valid value that leaves the base in
place. Convert it to an empty mapping rather than a nil one, so that a
null base merged with an override listing entries does not panic either.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Dropping an override entry equal to one already present broke the "last
entry for a key wins" rule of KEY=VALUE lists: [MODE=debug, MODE=release]
over [MODE=release] ended on debug. Merge these lists by key instead: an
override entry replaces the base entry of the same key in place and new
keys are appended. EnforceUnicity does the same, but it ignores jobs and
the labels of networks, volumes, secrets and configs, and the schema
rejects repeated items, so the merge itself must not leave duplicates.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
This PR fixes two override-merge regressions introduced in v2.15.0: null mapping-like values failing to merge and repeated key-value entries losing their last-entry-wins behavior.
Changes:
Accept null values when merging mapping-like attributes.
Merge key-value lists by key, with regression and multi-file tests.
Resolve existing lint issues.
File
Description
tree/matcher_test.go
Simplifies test assertions for lint compliance.
override/merge.go
Updates null conversion and key-value merge behavior.
A file may repeat a key with different values, such as [MODE=debug,
MODE=release]. The override replaced only the last occurrence and could
leave two identical entries, which the schema rejects on paths
EnforceUnicity does not cover, such as jobs. Merge base and override
through the same by-key loop so repeated keys collapse.
Merging two empty lists now returns an empty list rather than nil, which
the schema also rejects on those paths.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
With interpolation disabled the merge sees raw strings. Cover the keys it
collapses (same text, including a default value holding an equal sign)
and the ones it leaves side by side because they could only resolve to
the same key once interpolated, so the last entry still wins after that.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
mergeKeyValueSequence reimplements the same "compact by key, last one wins" loop that override/uncity.go's enforceUnicity already implements (uncity.go:86-105 vs merge.go:219-235). Not a bug, but a maintenance cost: a future fix to the compaction/tie-break rule applied to one and not the other would silently reintroduce the class of bug this PR just fixed, for whichever path is missed. Worth factoring out a shared helper?
Reject null IPAM config items instead of converting to empty objects
override/merge.go:330
This converter also handles individual networks.*.ipam.config items. If a base has config: [{subnet: 10.0.0.0/24}] and the override has config: [null], converting the null item to {} makes mergeIPAMConfig output [{}] instead of rejecting the invalid item, silently dropping the base subnet. The schema rejects null IPAM items but accepts {}, so validation after merging cannot catch this. Reject null for IPAM items while still treating null as empty for service networks, models, and depends_on.
Nothing kept the by-key compaction of KEY=VALUE lists in sync with the
one EnforceUnicity applies, so a fix to one could miss the other. Share
it through uniqueEntries and build the merge on it.
Reading null as an empty mapping was done for every mapping conversion,
which made an ipam.config item of null silently drop the subnet of the
base. Do it only for networks, models and depends_on, and reject null
IPAM items as before. Also document that raw keys resolving to the same
name once interpolated are not reconciled, as already for services.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Traced the full merge-regression fix through 4 iterations (KEY=VALUE by-key merge, base-side key compaction, non-interpolated entries, and now the shared uniqueEntries helper + the ipam.config null rejection). Verified empirically at each step: full test suite + golangci-lint clean, and the original data-loss repro (a null entry in a mixed ipam.config list silently dropping the base subnet) now correctly errors instead. The mergeKeyValueSequence/enforceUnicity duplication is resolved via the shared uniqueEntries helper. LGTM.
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
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.
ndeloof
left a comment
Fixes two regressions of the override merge, both introduced in v2.15.0 and found by the buildx bake tests added in docker/buildx#4117 (they pass on v2.14.0 and fail on v2.16.0).
nulloverride.networks: null,depends_on: nullormodels: nullin an override failed withcannot convert <nil> into a mapping, since #915 madeconvertIntoMappingreject unknown types.nullis a valid value that leaves the base in place, so it now converts to an empty mapping (not a nil one: a null base merged with an override listing entries would panic).Last entry wins. Since #924, an override entry equal to one already present is dropped, which breaks KEY=VALUE lists where the last entry for a key takes effect:
[MODE=debug, MODE=release]over[MODE=release]ended ondebug(environment,labels,build.args, ...). These lists are now merged by key, the wayEnforceUnicitydoes: an entry replaces the earlier entry of the same key in place, new keys are appended, and keys the base repeats collapse to their last value. It has to happen at merge time becauseEnforceUnicityignoresjobsand the labels of networks, volumes, secrets and configs, and the schema rejects repeated items there. Two empty lists merge into an empty list, not a nil one, for the same reason. Deduplication is unchanged forenv_file,dns,tmpfs,extra_hosts, etc.Keys are read up to the first
=, asEnforceUnicityalready does, so with interpolation disabled the merge works on the raw strings:${K}=1and${K}=2merge, while entries that only resolve to the same key after interpolation (${K1}=1,${K2}=2, or a bare${ENTRY}) are kept side by side, in order, so the last one still wins once interpolated. Results onservicesare identical to v2.16.0 in that mode.Each fix has YAML-in/YAML-out tests in
override/and there is an end-to-end multi-file test inloader/tests. A separate commit fixes the three lint issues already present on main.🤖 Generated with Claude Code