-
Notifications
You must be signed in to change notification settings - Fork 325
Conversation
demolitionmode
commented
Jun 19, 2026
Thank you for approving the workflow, the error we're seeing is the same in the test output (Warning: failed to build structured entry for prod, web, Deployment (apps) (kind: Deployment, changeType: MODIFY): Invalid JSON Document).
The proposed fix has now been pushed for review.
Results of local testing
Using released version (3.15.10)
(I should've included this in the original description, apologies)
Unstructured output showing the expected changes
helm diff upgrade -n apps helm-diff-test chart
apps, k8s-cron, CronJob (batch) has changed: # Source: helm-diff-chart/templates/cron.yaml apiVersion: batch/v1 kind: CronJob metadata: name: k8s-cron namespace: apps labels: app: helm-diff-test spec: schedule: "@yearly" jobTemplate: spec: template: metadata: labels: app: helm-diff-test spec: restartPolicy: "Never" containers: - name: k8s-cron - image: "hello-world" + image: "hello-world:1" affinity: nodeAffinity: requiredDuringSchedulingIgnoredDuringExecution: nodeSelectorTerms: - matchExpressions: - key: node-type operator: In values: - - standard + - dedicated apps, web, Deployment (apps) has changed: # Source: helm-diff-chart/templates/deployment.yaml apiVersion: apps/v1 kind: Deployment metadata: name: web namespace: apps labels: app: helm-diff-test spec: selector: matchLabels: app: helm-diff-test replicas: 1 template: metadata: labels: app: helm-diff-test spec: containers: - name: app image: "hello-world" affinity: nodeAffinity: requiredDuringSchedulingIgnoredDuringExecution: nodeSelectorTerms: - matchExpressions: - key: node-type operator: In values: - - standard + - dedicated
Structured output failing:
helm diff upgrade -n apps helm-diff-test chart --output structured
helm diff upgrade -n apps helm-diff-test chart --output structured
Warning: failed to build structured entry for apps, k8s-cron, CronJob (batch) (kind: CronJob, changeType: MODIFY): Invalid JSON Document
Warning: failed to build structured entry for apps, web, Deployment (apps) (kind: Deployment, changeType: MODIFY): Invalid JSON Document
[
{
"name": "apps, k8s-cron, CronJob (batch)",
"changeType": "MODIFY",
"resourceStatus": {
"oldExists": false,
"newExists": false
}
},
{
"name": "apps, web, Deployment (apps)",
"changeType": "MODIFY",
"resourceStatus": {
"oldExists": false,
"newExists": false
}
}
]
Using patched binary
Structured output shows it's able to process arrays of strings properly now
HELM_NAMESPACE="apps" \ HELM_BIN="/opt/homebrew/bin/helm" \ ./helm-diff upgrade helm-diff-test chart \ --output structured
[
{
"apiVersion": "batch/v1",
"kind": "CronJob",
"namespace": "apps",
"name": "k8s-cron",
"changeType": "MODIFY",
"resourceStatus": {
"oldExists": true,
"newExists": true
},
"changes": [
{
"path": "spec.jobTemplate.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].values",
"field": "0",
"change": "replace",
"oldValue": "standard",
"newValue": "dedicated"
},
{
"path": "spec.jobTemplate.spec.template.spec.containers[0]",
"field": "image",
"change": "replace",
"oldValue": "hello-world",
"newValue": "hello-world:1"
}
]
},
{
"apiVersion": "apps/v1",
"kind": "Deployment",
"namespace": "apps",
"name": "web",
"changeType": "MODIFY",
"resourceStatus": {
"oldExists": true,
"newExists": true
},
"changes": [
{
"path": "spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].values",
"field": "0",
"change": "replace",
"oldValue": "standard",
"newValue": "dedicated"
}
]
}
]
@yxxhero
yxxhero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review — Approve with minor suggestions
Solid, minimal fix for a real bug, and the test reproduces the actual K8s case (nodeSelectorTerms.matchExpressions.values). Build/Test + Lint pass. Leaving a few suggestions below.
The bug (context)
diffArrayNodes calls createNodePatch for every differing array element pair. createNodePatch delegates to jsonpatch.CreateMergePatch, which rejects non-object JSON documents — so two strings like "standard" vs "dedicated" yield Invalid JSON Document and the whole resource's structured entry is dropped.
What's good
- Correct and minimal; mirrors the existing
subPatch == nilreplace path (structured.go:278-288), so output shape stays consistent. - The
DeepEqualcheck above already filters equal strings, so no redundant changes are emitted. - Test asserts path, field, change type, old/new values and updates the change count 2 → 3.
Suggestions
1. Scope — other primitives have the same bug. createNodePatch will also fail for arrays of numbers (float64) and booleans (bool) — e.g. a list of replica counts. The PR only fixes strings. Either widen the guard to all scalars, or add a comment noting the limitation so a future contributor isn't surprised:
// createNodePatch only supports JSON objects; handle scalars directly.2. Flatten the nested if (style). The current double-nested if ok { if ok { ... } } can be flattened by lifting the type assertions out of the condition:
oldStr, oldIsStr := oldVal.(string) newStr, newIsStr := newVal.(string) if oldIsStr && newIsStr { path, field := splitTokens(next) *changes = append(*changes, FieldChange{ Path: path, Field: field, Change: "replace", OldValue: oldStr, NewValue: newStr, }) continue }
Behavior is identical — when oldVal is a string but newVal isn't, it still falls through to createNodePatch.
3. Duplication (optional). The replace FieldChange block now appears twice in this function (plus similar add/remove blocks). A small appendReplaceChange(changes, next, old, new) helper would reduce drift risk — optional given the existing style.
4. Test coverage (nice-to-have). Top-level string arrays and mixed-type arrays aren't covered. The add/remove paths for strings already work via the existing switch arms; just worth a test to lock it in.
yxxhero
commented
Jun 19, 2026
@demolitionmode thanks so much. please review my comments.
demolitionmode
commented
Jun 19, 2026
@demolitionmode thanks so much. please review my comments.
Thanks, they're great suggestions, I'll work on getting them implemented
Uh oh!
There was an error while loading. Please reload this page.
Summary
We've run into an issue when using
--output structuredin that changes to lists of strings (eg. those innodeSelectorTerms.matchExpressions.values) does not get rendered properly.Current behaviour
Running the below command results in failure.
Warning raised:
Resulting output for that resource:
[ { "name": "apps, k8s-cron, CronJob (batch)", "changeType": "MODIFY", "resourceStatus": { "oldExists": false, "newExists": false } }, ... ]Suspected cause
It appears that whilst generating a structured diff, maps and arrays of maps are handled correctly, but arrays of strings are not; they seem to be passed to createNodePatch regardless, which tries to marshal and unmarshal these strings (which causes an error during the unmarshal).
Proposed fix
Inside diffArrayNodes, we can check if the two values are strings, then append a change immediately without a need for any further processing.
Notes
This PR's first commit will just be adding to the relevant test case, which should show the behaviour mentioned above (once the workflow runs). The proposed fix will be pushed in a subsequent commit