feat: Add support for key+ syntax #2088

nocturo · 2025-06-25T13:30:30Z

This PR adds support for key+ syntax that is used in several other places for appending to lists and maps instead of overwriting them and keeping your code DRY (in case you want to append to some map you've defined before).

How would you use it?

helmfile.yaml:

repositories:
  - name: bitnami
    url: https://charts.bitnami.com/bitnami

releases:
  - name: myapp
    namespace: default
    chart: bitnami/nginx
    version: 15.0.0
    values:
      - base.yaml
      - override.yaml

base.yaml:

replicaCount: 2
image:
  tag: "1.21"
  pullPolicy: IfNotPresent

service:
  type: ClusterIP
  port: 80

ingress:
  enabled: false

resources:
  limits:
    cpu: 100m
    memory: 128Mi
  requests:
    cpu: 100m
    memory: 128Mi

labels:
  app: myapp
  environment: dev

override.yaml:

replicaCount+: 3
image+:
  tag: "1.25"

service+:
  type: LoadBalancer
  port: 443

ingress+:
  enabled: true
  hosts:
    - host: myapp.example.com

resources+:
  limits:
    cpu: 200m
    memory: 256Mi

labels+:
  environment: prod
  team: platform

and we get these values out:

image:
  pullPolicy: IfNotPresent
  tag: "1.25"
ingress:
  enabled: true
  hosts:
    - host: myapp.example.com
labels:
  app: myapp
  environment: prod
  team: platform
replicaCount: 3
resources:
  limits:
    cpu: 200m
    memory: 256Mi
  requests:
    cpu: 100m
    memory: 128Mi
service:
  port: 443
  type: LoadBalancer

Let me know if this is something that can benefit the project.

yxxhero · 2025-06-25T21:36:57Z

please fix DCO

yxxhero · 2025-06-25T21:37:10Z

@nocturo

nocturo · 2025-06-26T07:49:26Z

Done! @yxxhero

yxxhero · 2025-06-26T10:43:03Z

@nocturo please fix tests issues.

yxxhero · 2025-06-29T00:31:59Z

@nocturo still failed.

z0rc · 2025-07-08T15:58:52Z

Please document this behaviour, so users would actually able to discover and use it.

yxxhero · 2025-07-10T22:34:31Z

@nocturo ping

nocturo · 2025-07-14T09:26:19Z

Hi @yxxhero @z0rc. I've addressed the linter issue and added some documentation about the feature as requested. If it's not a correct place to document it, let me know where would you like to see it so that I can correct it.

yxxhero · 2025-07-14T10:10:08Z

@nocturo awesome！thanks for your work.

yxxhero · 2025-07-14T13:22:07Z

pkg/state/state.go

 	return generatedFiles, nil
 }

+// mergeAppendValues merges two values for the same key, preserving key+ keys for later processing
+func mergeAppendValues(existing, incoming any) any {


could you add unittest for this func?

Added a unit test for it, is it suitable?

yxxhero · 2025-07-14T22:04:25Z

@nocturo please fix DCO issue.

nocturo · 2025-07-15T10:48:57Z

@yxxhero fixed DCO

yxxhero · 2025-07-15T23:46:27Z

@mumoshu WDYT?

mumoshu

Awesome work @nocturo!
Looks great and feeling very close to merge this!
But I have several comments regarding unused and potentially unnecessary functions.
Could you confirm?
If we could remove those unnecessary parts, we could also slim down the additional tests.

mumoshu · 2025-07-16T01:58:04Z

pkg/yaml/append_processor_test.go

+			var result map[string]any
+			err := UnmarshalWithAppend([]byte(tt.yamlData), &result)


This should also cover cases when result is non-empty, like you already did (thanks!) for TestAppendProcessor_MergeWithAppend, but for UnmarshalWithAppend this time.

mumoshu · 2025-07-16T02:06:03Z

pkg/yaml/append_processor.go

+	return &AppendProcessor{}
+}
+
+func (ap *AppendProcessor) ProcessMap(data map[string]any) (map[string]any, error) {


I don't quite understand why ProcessMap is needed!
I think the key idea is the use the new MergeWithAppend instead of mergo.Merge when merging src values to dst values, and MergeWithAppend does not depend on this ProcessMap (and the utility processValue), right?
Could it be possible that you can just remove this ProcessMap and the new merge-with-append feature just work?

mumoshu · 2025-07-16T02:10:02Z

pkg/state/state.go

@@ -1683,11 +1682,12 @@ func (st *HelmState) WriteReleasesValues(helm helmexec.Interface, additionalValu
 				return []error{fmt.Errorf("reading %s: %w", f, err)}
 			}

-			if err := yaml.Unmarshal(srcBytes, &src); err != nil {
+			if err := yaml.UnmarshalWithAppend(srcBytes, &src); err != nil {


Why do we need the special Unmarshal function here? The merge happens on MergeWithAppend, which is aware of the new key+ syntax.
Isnt this special unmarshal function unnecessary, given we could just unmarshal using yaml.Unmarshal so that you get the new src map whose keys may or may not contain the + suffix, which is then merged using optional list-appending feature by the new MergeWithAppend.

mumoshu · 2025-07-16T02:12:27Z

pkg/yaml/yaml.go

+	return Unmarshal(processedYAML, v)
+}
+
+// NewDecoderWithAppend creates and returns a function that is used to decode a YAML document


I feel like this new NewDecoderWithAppend has no use and that's why this looks unused?
If I'm not mistaken, this could be removed?

mumoshu · 2025-07-16T02:13:14Z

pkg/yaml/yaml.go

@@ -63,3 +63,76 @@ func Unmarshal(data []byte, v any) error {

 	return v2.Unmarshal(data, v)
 }
+
+// UnmarshalWithAppend unmarshals YAML data with support for key+ syntax


As commented elsewhere, I feel like this is not necessary and can be removed. Merge-with-list-appending happens elsewhere so you won't need to touch unmarshaler and decoder?

nocturo · 2025-07-17T08:27:46Z

I've simplified the code further, hopefully a much cleaner solution!

Signed-off-by: Nemanja Zeljkovic <[email protected]>

JuryA · 2025-07-19T08:10:15Z

Hi, interesting feature—thanks for sharing!

I was just reading the PR description and got a bit confused by the statement that the key+ syntax allows for appending to lists and maps instead of overwriting them. To my knowledge, standard YAML maps don’t really have an “append” operation—unless you’re referring to ordered maps (like !!omap), which aren’t commonly supported in most YAML parsers.

Also, in the examples provided, I don’t actually see any lists being appended—only maps being merged. As far as I can tell, what’s shown in the PR is already default behavior for maps in many YAML tools, so it’s not clear what benefit the + suffix adds in this context.

The overall idea sounds very useful, especially for lists (where an append would be really handy!), but as written, the explanation around maps and lists could be a bit misleading.

If I’m missing something, please let me know—just wanted to flag this for clarity!

JuryA

👎 I Strongly Oppose Introducing a Custom Append Processor

This code duplicates and oversimplifies—therefore misrepresents—functionality that is already better handled by mergo:

Redundant Wheel Reinvention – mergo.Merge(&dst, src, mergo.WithOverride, mergo.WithAppendSlice) already does this (and much more), including recursive map merging and safe slice appending
Incorrect “Append” Semantics – When srcValue or destValue aren't slices, the result is a destructive overwrite (dest[baseKey] = srcValue). This breaks user expectations, as “append” should imply non-destructive extension
Panic Risk – The type assertion srcValue.([]any) panics if the slice holds a different type; mergo handles this safely via reflect and error-return logic
Maps ≠ Lists – Go maps are explicitly unordered (per the Go spec); “appending” to a map makes no conceptual sense—only key merging is possible, not preserving order
Maintainability & Test Coverage – mergo is battle-tested (Docker, K8s, Datadog) ; this custom implementation has minimal test coverage and increases bug surface (+95 LOC in this PR)
Unnecessary Complexity – Now there are two merge paths (Mergo + AppendProcessor), increasing cognitive overhead and behavioral divergence risk
Breaking Change with No Functional Gain – This is a definitive breaking change that offers no meaningful functionality in return. It disrupts expected behavior without justification.

⚠️ Insufficient Tests

The current pkg/yaml/append_processor_test.go includes only four happy-path cases:
append slice→slice, recursion, creating a new slice, and overwriting a non-slice. It fails to cover:

Type Collisions – e.g., []string vs []any; current implementation panics on type assert
Append on Maps or Scalars – silently overwrites; completely untested.
Immutability of src – delete mutates the input, but tests don’t verify this side effect.
Nil vs Empty Slices – subtle but important; diverges from mergo.WithAppendSlice behavior
Property-Based Edge Cases – would easily reveal issues above; just use gopter or rapid

The result is a false positive CI: the function never returns an error, and tests don't provoke panic conditions—so everything "passes."

💡 Recommendation

Retain only key+ detection, but delegate merging to mergo (WithOverride, WithAppendSlice); for special cases, use a custom mergo.Transformer
Add negative tests (type mismatches, maps, nil slices) and property-based tests.
Eliminate input mutation → return a merged result without side effects.

TL;DR: Instead of 100 lines of risky custom code, use one clean call to mergo.Merge(...)—with proper test coverage. It’s safer, more robust, and fully maintainable.

JuryA · 2025-07-19T21:02:12Z

pkg/yaml/append_processor.go

+					srcSlice := srcValue.([]any)
+					dest[baseKey] = append(destSlice, srcSlice...)
+				} else {
+					dest[baseKey] = srcValue


Am I seeing this correctly? Is this just a hard overwrite of the value instead of a merge? 🫨

JuryA · 2025-07-19T21:03:13Z

pkg/yaml/append_processor.go

+					dest[baseKey] = srcValue
+				}
+			} else {
+				dest[baseKey] = srcValue


Is this the same case here as well — a hard overwrite instead of a merge?

mumoshu · 2025-07-21T10:41:21Z

@JuryA Thanks!

The core problem for us is that we don't want a global setting to toggle mergo.WithAppendSlice, nor do we wish to implement one, so if you could improve this to flexibly update the merge options depending on a "marker" like + this strives to introduce, that would definitely be better, yes!

My concern is that I think @nocturo and you have divergent ideas on how to implement it/move this forward?
Would you mind try implementing it in a way you proposed, if you have some time? That way we can compare what works better in practice, before moving on!

Thank you in advance for your support!

nocturo requested review from mumoshu, itscaro, yxxhero, xiaomudk and zhaque44 as code owners June 25, 2025 13:30

nocturo force-pushed the add/key_append branch from 5e9e867 to 6fb32e0 Compare June 26, 2025 07:48

nocturo force-pushed the add/key_append branch from 6fb32e0 to f4174a5 Compare June 27, 2025 12:06

yxxhero force-pushed the add/key_append branch from f976fed to a91476a Compare July 14, 2025 10:20

yxxhero reviewed Jul 14, 2025

View reviewed changes

nocturo force-pushed the add/key_append branch from 090980e to 4c86f1a Compare July 15, 2025 10:48

mumoshu requested changes Jul 16, 2025

View reviewed changes

nocturo requested a review from mumoshu July 17, 2025 08:28

nocturo added 5 commits July 19, 2025 07:53

Add support for key+ syntax

2eae3bd

Signed-off-by: Nemanja Zeljkovic <[email protected]>

Fix linter

8e0984e

Signed-off-by: Nemanja Zeljkovic <[email protected]>

Add documentation for the key+ syntax

eac80eb

Signed-off-by: Nemanja Zeljkovic <[email protected]>

Add unit test

82a7593

Signed-off-by: Nemanja Zeljkovic <[email protected]>

Remove unneeded code and cleanup

65af1ca

Signed-off-by: Nemanja Zeljkovic <[email protected]>

yxxhero force-pushed the add/key_append branch from 3036f7b to 65af1ca Compare July 18, 2025 23:53

JuryA reviewed Jul 19, 2025

View reviewed changes

		var result map[string]any
		err := UnmarshalWithAppend([]byte(tt.yamlData), &result)

Uh oh!

feat: Add support for key+ syntax #2088

Are you sure you want to change the base?

feat: Add support for key+ syntax #2088

Conversation

nocturo commented Jun 25, 2025

Uh oh!

yxxhero commented Jun 25, 2025

Uh oh!

yxxhero commented Jun 25, 2025

Uh oh!

nocturo commented Jun 26, 2025

Uh oh!

yxxhero commented Jun 26, 2025

Uh oh!

yxxhero commented Jun 29, 2025

Uh oh!

z0rc commented Jul 8, 2025

Uh oh!

yxxhero commented Jul 10, 2025

Uh oh!

nocturo commented Jul 14, 2025

Uh oh!

yxxhero commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yxxhero commented Jul 14, 2025

Uh oh!

nocturo commented Jul 15, 2025

Uh oh!

yxxhero commented Jul 15, 2025

Uh oh!

mumoshu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nocturo commented Jul 17, 2025

Uh oh!

JuryA commented Jul 19, 2025

Uh oh!

JuryA left a comment

Choose a reason for hiding this comment

⚠️ Insufficient Tests

💡 Recommendation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mumoshu commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mumoshu commented Jul 21, 2025 •

edited

Loading