fix: drop null-valued labels/annotations keys in metadata tidy - #1067
Closed
yxxhero wants to merge 2 commits into
Closed
fix: drop null-valued labels/annotations keys in metadata tidy#1067yxxhero wants to merge 2 commits into
yxxhero wants to merge 2 commits into
Conversation
Charts commonly render a bare `labels:` (null) key when a conditional labels block is empty. deleteStatusAndTidyMetadata only pruned map-typed values, so a null labels/annotations key survived pruning and showed up as a confusing "- labels:" diff entry against the other side that has no such key. pruneNestedMap now removes the key when its value is null, in addition to removing it when the pruned map ends up empty. Also adds a CI reproduction script exercising no-labels charts, dropped label chart versions, bare/null labels blocks, custom resources, flux-style labels, --take-ownership, --dry-run=server and empty labels maps against a real kind cluster. Fixes databus23#1064 Signed-off-by: yxxhero <aiopsclub@163.com>
…fests
Kubernetes treats a null or empty labels/annotations map exactly like an
absent key, but charts frequently render the empty key anyway (e.g. via
a conditional labels block that renders a bare "labels:" line).
A raw textual diff between such a manifest and one that omits the key
reports a meaningless change:
metadata:
finalizers:
- externalsecrets.external-secrets.io/externalsecret-cleanup
- labels:
name: webhook-token
This is the confusion reported in databus23#1064: after the managed-by label is
pruned, the leftover empty key still shows as "- labels:".
stripEmptyMetadataKeys now drops "labels:"/"annotations:" lines from
each parsed manifest whenever the parsed value is null or an empty
mapping. The removal is line-based so surrounding text keeps its
original formatting (# Source comments, key order, indentation); the
parsed document is consulted only to confirm the key really carries no
content, so keys with entries (including nested spec.template.metadata
labels) are untouched.
The CI reproduction script now asserts that the plain diffs for the
bare-null-labels and empty-flow-map-labels chart variants stay clean.
Fixes databus23#1064
Signed-off-by: yxxhero <aiopsclub@163.com>
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.
Summary
Fixes #1064
helm diffshows a confusing labels-only diff entry like:metadata: finalizers: - externalsecrets.external-secrets.io/externalsecret-cleanup - labels: name: webhook-tokenwhen one side of the diff renders an empty
labelskey and the other omits it.Root cause
Kubernetes treats a null or empty
labels/annotationsmap exactly like an absent key, but charts frequently render the empty key anyway, e.g. via a conditional block:Two gaps left that noise in the diff:
manifest.Parsekept the raw text of both sides untouched for the plain (text) diff, so a stored manifest containing a barelabels:(null) orlabels: {}produced a meaningless- labels:entry against a new chart revision that omits the key.deleteStatusAndTidyMetadata/pruneNestedMap(live-object tidy used by--three-way-mergeand--take-ownership) only handled map-typed values, so a null"labels": nullvalue survived pruning with the same effect.Fix
stripEmptyMetadataKeysdropslabels:/annotations:lines from each parsed manifest whenever the parsed value is null or an empty mapping. The removal is line-based so surrounding text keeps its original formatting (# Sourcecomments, key order, indentation); the parsed document is only consulted to confirm the key really carries no content, so keys with entries (including nestedspec.template.metadata.labels) are untouched.pruneNestedMapremoves the key when its value is null, in addition to the existing removal when the pruned map ends up empty.Testing
{}labels lines are stripped byParsewhile labels with content and nested pod-template labels are kept.scripts/repro-issue-1064.shruns against the kind cluster in CI and covers (plain + three-way-merge):managed-bylabel, diffed against a chart version that dropped itlabels:(null) key vs a version without the block — the exact Bug: helm-diff shows- labels, if resource contains onlyapp.kubernetes.io/managed-bylabel #1064 symptom (- labels:)--take-ownership--dry-run=serverlabels: {}map — produces- labels: {}without the fixSigned-off-by: yxxhero aiopsclub@163.com