Remove last-applied-configuration from annotations#137
Conversation
Current exported data contains last-applied-configuration annotation, that is not needed for migration, so should be removed. Related to migtools/crane#253 (also more details there) Signed-off-by: Marek Aufart <maufart@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds removal of the Changes
Sequence Diagram(s)(omitted — changes are limited to two files and do not involve multi-component interaction warranting a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transform/kubernetes/kubernetes.go (1)
69-77:⚠️ Potential issue | 🔴 CriticalEscape JSON Pointer tokens in
stripFieldsbefore generating patch paths.Line 76 adds
kubectl.kubernetes.io/last-applied-configurationto fields being stripped. The/in this annotation key must be escaped per RFC 6901 when constructing JSON Patch paths. Currently,stripFieldsat line 589 generates/metadata/annotations/kubectl.kubernetes.io/last-applied-configuration, which the json-patch library decodes as four tokens (metadata→annotations→kubectl.kubernetes.io→last-applied-configuration) instead of three. This causes the patch to target a non-existent nested path and fails to remove the annotation.The same escaping issue also affects
addAnnotations()andremoveAnnotations()functions when handling keys with/or~characters.Proposed fix
+func escapeJSONPointerToken(token string) string { + token = strings.ReplaceAll(token, "~", "~0") + return strings.ReplaceAll(token, "/", "~1") +} + func stripFields(obj unstructured.Unstructured) (jsonpatch.Patch, error) { var patches jsonpatch.Patch for _, field := range fieldsToStrip { _, found, err := unstructured.NestedFieldNoCopy(obj.Object, field...) if err != nil { return patches, err } if found { - patch, err := jsonpatch.DecodePatch([]byte(fmt.Sprintf(opRemove, fmt.Sprintf(strings.Repeat("/%v", len(field)), interfaceSlice(field)...)))) + escaped := make([]interface{}, 0, len(field)) + for _, token := range field { + escaped = append(escaped, escapeJSONPointerToken(token)) + } + path := fmt.Sprintf(strings.Repeat("/%v", len(field)), escaped...) + patch, err := jsonpatch.DecodePatch([]byte(fmt.Sprintf(opRemove, path))) if err != nil { return nil, err } patches = append(patches, patch...) } } return patches, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transform/kubernetes/kubernetes.go` around lines 69 - 77, The patch generation and annotation helpers (stripFields, addAnnotations, removeAnnotations) build JSON Patch paths from raw keys but do not RFC 6901-escape JSON Pointer tokens, so keys like "kubectl.kubernetes.io/last-applied-configuration" are split incorrectly; update these functions to escape each token before joining into a path by replacing "~" with "~0" and "/" with "~1" (apply this escaping when constructing paths in stripFields and when building annotation add/remove patch ops in addAnnotations/removeAnnotations) so patch targets the correct single annotation key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@transform/kubernetes/kubernetes.go`:
- Around line 69-77: The patch generation and annotation helpers (stripFields,
addAnnotations, removeAnnotations) build JSON Patch paths from raw keys but do
not RFC 6901-escape JSON Pointer tokens, so keys like
"kubectl.kubernetes.io/last-applied-configuration" are split incorrectly; update
these functions to escape each token before joining into a path by replacing "~"
with "~0" and "/" with "~1" (apply this escaping when constructing paths in
stripFields and when building annotation add/remove patch ops in
addAnnotations/removeAnnotations) so patch targets the correct single annotation
key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdf43b0a-856f-42b5-97f2-e39f0d337b04
📒 Files selected for processing (2)
transform/kubernetes/kubernetes.gotransform/kubernetes/kubernetes_test.go
Signed-off-by: Marek Aufart <maufart@redhat.com>
|
yes, i observed this as well, but got ignored. Thanks @aufi |
2 participants
Current exported data contains last-applied-configuration annotation, that is not needed for migration, so should be removed.
Related to migtools/crane#253 (also more details there)
Summary by CodeRabbit
Improvements
Tests