feat: omit 'app.kubernetes.io/managed-by' label on three-way-diff - #1044
Merged
Conversation
Collaborator
|
@jkroepke please not if in if. |
Contributor
There was a problem hiding this comment.
Pull request overview
Omits Helm-managed labels from three-way diff output to prevent misleading removals.
Changes:
- Removes
app.kubernetes.io/managed-by. - Cleans up empty label maps.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+39
to
+41
| if a := metadata["labels"]; a != nil { | ||
| labels := a.(map[string]interface{}) | ||
| delete(labels, "app.kubernetes.io/managed-by") |
Collaborator
|
@jkroepke Thanks for the PR! I pushed a follow-up commit to avoid the nested I extracted a small helper so the two checks are sequential rather than nested: func pruneNestedMap(target map[string]interface{}, key string, fields ...string) {
sub, ok := target[key].(map[string]interface{})
if !ok {
return
}
for _, field := range fields {
delete(sub, field)
}
if len(sub) == 0 {
delete(target, key)
}
}The call site is now flat and both the existing pruneNestedMap(metadata, "annotations",
"meta.helm.sh/release-name",
"meta.helm.sh/release-namespace",
"deployment.kubernetes.io/revision",
)
pruneNestedMap(metadata, "labels", "app.kubernetes.io/managed-by")As a bonus the comma-ok assertion is a bit safer than the previous nil-check + type assertion. I also added test coverage for the label omission and the empty-labels cleanup path. PTAL. |
Address review feedback to not use 'if in if'. Extract a deleteFromMap helper that uses a comma-ok type assertion (early return) and a final empty-check, so the two conditionals are sequential rather than nested. Apply it to both the existing annotations tidy and the new labels tidy (omitting app.kubernetes.io/managed-by) for consistency. Also add test coverage for label omission and the empty-label cleanup path. 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.
Ref: #326 (comment)
I'm using helm diff with three-way-diff in a upgrade --dry-mode and I observe changes on each resource which would remove
app.kubernetes.io/managed-by.