Skip to content

fix(override): merge ulimits into the base value - #907

Merged
ndeloof merged 1 commit into
compose-spec:mainfrom
vsaraikin:fix/merge-ulimit-base
Aug 11, 2026
Merged

ndeloof merged 1 commit into
compose-spec:mainfrom
vsaraikin:fix/merge-ulimit-base

Conversation

@vsaraikin

@vsaraikin vsaraikin commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Set one ulimit key in the base file and a different one in the override, and the base key disappears:

# base                # override
ulimits:              ulimits:
  locks:                locks:
    soft: 20000           hard: 65535
    hard: 40000

# got                 # want
locks:                locks:
  hard: 65535           soft: 20000
                        hard: 65535

mergeUlimit receives the base value and the override value, but it reads both mappings out of the override:

func mergeUlimit(_ any, o any, p tree.Path) (any, error) {
	over, ismapping := o.(map[string]any)
	if base, ok := o.(map[string]any); ok && ismapping {
		return mergeMappings(base, over, p)

So it merges the override with itself. The base argument is named _ and never read. Every other function in mergeSpecials takes its first argument as the base, this one is the exception.

Test_mergeYamlUlimits already covered the case, but its expectation encoded the old behaviour, so it is updated to expect the base soft to survive. Reverting merge.go alone makes it fail. Full suite is green

mergeUlimit read both the base and the override mapping from the same
argument, so it merged the override with itself and dropped the base.
A service that sets ulimits.locks.soft in the base file and only
ulimits.locks.hard in the override file lost the soft limit.

Take the base mapping from the base argument, which was declared and
then ignored. The existing test encoded the old behaviour and is
updated to expect the base value to survive.

Signed-off-by: Vladimir Saraikin <saraikin.v@phystech.edu>
@vsaraikin
vsaraikin requested a review from ndeloof as a code owner August 7, 2026 18:13
@ndeloof
ndeloof enabled auto-merge (rebase) August 11, 2026 14:50

@ndeloof ndeloof left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified locally: the updated test fails with the previous merge.go (the old expectation encoded the buggy output) and the override+loader suites are green with the fix. Merge semantics for the scalar/mapping mismatch cases are unchanged. I'll follow up with an additional test covering short/long syntax combinations.

@ndeloof
ndeloof merged commit 97e56ed into compose-spec:main Aug 11, 2026
8 checks passed
glours pushed a commit to ndeloof/compose-go that referenced this pull request Sep 2, 2026
Follow-up to compose-spec#907: exercise every combination of the short (single
limit) and long (soft/hard mapping) ulimit syntaxes between base and
override. Mappings merge key by key; a syntax mismatch makes the
override value win as a whole β€” the short-over-long case (the
'return o, nil' path in mergeUlimit) was previously untested.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
glours pushed a commit that referenced this pull request Sep 2, 2026
Follow-up to #907: exercise every combination of the short (single
limit) and long (soft/hard mapping) ulimit syntaxes between base and
override. Mappings merge key by key; a syntax mismatch makes the
override value win as a whole β€” the short-over-long case (the
'return o, nil' path in mergeUlimit) was previously untested.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants