Repository navigation
fix: keep metadata added outside the chart when it renders a null map - #1102
Merged
yxxhero merged 2 commits intoNov 7, 2026
Merged
Conversation
helm install, upgrade and rollback merge the release's ownership metadata (meta.helm.sh/release-name, meta.helm.sh/release-namespace and the app.kubernetes.io/managed-by label) into every object before computing the three-way patch. helm-diff computed the patch from the rendered manifest alone, so when a chart renders `annotations:` or `labels:` with nothing under it, the patch nulled the whole map. The server-side dry-run then dropped every annotation or label added outside the chart and reported their removal, while the real upgrade keeps them. Add the same ownership metadata to the new manifest before createPatch, via a WithRelease option that the upgrade command sets. The custom resource path is unaffected: it tidies these keys away before comparing the manifests. Behavioral change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rename the script so scripts/issues/run.sh picks it up, and link the issue. Structural change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified in the changes aligning three-way patch generation with Helm ownership metadata.
No open issues 🎉
What changed in this PR
Addresses #1101 by aligning three-way patch generation with Helm’s ownership metadata handling, preventing false removal reports for externally added annotations and labels when a chart renders null metadata maps.
Changes:
- Passes the release name and namespace into manifest generation.
- Adds Helm ownership metadata to a copy of the target object before computing patches.
- Adds unit and cluster regression tests for metadata preservation and chart-owned annotation drift.
| File | Description |
|---|---|
scripts/issues/1101.sh |
Exercises null-map preservation, client merge mode, drift detection, and Helm upgrade behavior. |
manifest/generate.go |
Adds release options and injects ownership metadata before patch creation. |
manifest/generate_test.go |
Verifies strategic patches preserve externally added annotations and labels. |
cmd/upgrade.go |
Resolves the release namespace and passes release identity to manifest generation. |
Review effort: Balanced
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #1101
Problem
A chart that renders
annotations:orlabels:with nothing under it leaves the map null.On the strategic merge path, the three-way patch helm-diff computes then deletes the whole map, so
--three-way-mergereports every annotation or label added outside the chart as removed.helm upgradekeeps them, because it merges the release's ownership metadata into every object before it computes its own patch (setMetadataVisitor,pkg/action/validate.go), so the map it sends is never null.Fix
Add the same ownership metadata to the new manifest before
createPatch, so the patch matches the one the upgrade sends.A new
WithRelease(name, namespace)option tomanifest.Generatecarries the release identity;helm diff upgradesets it.The custom resource path is unaffected:
cleanMetadataForPatchalready removes these keys, and any map they leave empty, before comparing, so the added metadata does not count as a chart change.The Helm keys are pruned from the output as before.
Tests
TestCreatePatch_KeepsMetadataAddedOutsideTheChartUnderANullMapapplies the patch as the API server does and checks that an annotation and a label added outside the chart survive. It fails without the fix.scripts/issues/1101.shreproduces the issue on a cluster:--three-way-merge-mode client;helm upgradekeeps the metadata added outside the chart, which is the behavior the diff must agree with.make testandmake lintpass;scripts/issues/run.shpasses on a fresh kind cluster for Bug: helm-diff shows- labels, if resource contains onlyapp.kubernetes.io/managed-bylabel #1064,--three-way-mergereports removal of annotations/labels added outside the chart when the chart renders an empty map #1101, Diff is incorrect when values in values.yaml change and using local Chart #464, Diff failing when diffing helm hook jobs with --take-ownership flag #782 and helm diff --three-way-merge does not detect manual changes on Custom Resources (CRDs) #917.🤖 Generated with Claude Code