Update transform pkg for kustomize and multistage#135
Conversation
- Add TransformArtifact, PatchTarget, IgnoredOperation, ResourceGroup types - Add Kustomize serialization package (serializer, naming, kustomization) - Add resource grouping logic for multi-doc YAML generation - Add WhiteoutReport and IgnoredPatchReport structures - Comprehensive unit tests for all new functionality Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Reports functionality removed to simplify implementation. Core kustomize, grouping, and types functionality preserved. All tests still passing.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds new transform utilities and types: grouping Unstructured resources by computed type keys, reading/writing multi-document YAML, deterministic Kustomize generation and filename/patch naming, JSONPatch-to-YAML serialization, and comprehensive unit tests for each feature. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
transform/kustomize/serializer.go (3)
78-93:versionparameter is not sanitized.All other components (
kind,name,group,namespace) are sanitized, butversionis used directly in the filename. While API versions are typically safe strings likev1orv1beta1, custom CRDs could theoretically have unusual version strings.💡 Optional: Sanitize version for consistency
if namespace != "" { // Namespaced resource sanitizedNamespace := sanitizeComponent(namespace) sanitizedGroup := sanitizeComponent(group) + sanitizedVersion := sanitizeComponent(version) if group == "" { // Core resource: <namespace>--<version>--<kind>--<name>.patch.yaml filename = fmt.Sprintf("%s--%s--%s--%s.patch.yaml", sanitizedNamespace, - version, + sanitizedVersion, sanitizedKind, sanitizedName) } else { // Non-core resource: <namespace>--<group>-<version>--<kind>--<name>.patch.yaml filename = fmt.Sprintf("%s--%s-%s--%s--%s.patch.yaml", sanitizedNamespace, sanitizedGroup, - version, + sanitizedVersion, sanitizedKind, sanitizedName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transform/kustomize/serializer.go` around lines 78 - 93, The filename construction uses the raw variable version while other components use sanitized values (sanitizedNamespace, sanitizedKind, sanitizedName, sanitizedGroup); update the code in serializer.go where filename is built so that version is sanitized the same way as kind/name (e.g., produce sanitizedVersion using the existing sanitizer and use sanitizedVersion in both core and non-core fmt.Sprintf calls) to avoid unsafe characters in generated filenames.
34-41: Silent error handling forValueInterface()may mask issues.When
op.Kind() != "remove", errors fromValueInterface()are silently ignored. While this is fine for operations that legitimately have no value, it could mask unexpected errors foradd/replaceoperations that should have a value.💡 Optional: Log or handle missing values for non-remove operations
Consider logging a warning or returning an error when
add/replace/testoperations fail to retrieve their value, as these operations require a value per RFC 6902.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transform/kustomize/serializer.go` around lines 34 - 41, The current code silently ignores errors from op.ValueInterface() when op.Kind() != "remove"; change it so that for non-remove operations (e.g., "add", "replace", "test") you do not swallow unexpected errors—detect if err != nil and either return that error up the call stack or emit a warning log including op.Kind() and the operation path (use op.Path() or similar) before proceeding; keep the existing behavior of omitting "value" for legitimate missing-values but ensure add/replace/test failures are surfaced via an error return or explicit log so issues aren't masked.
117-133: Inconsistency withSanitizeFilenameinnaming.go.
sanitizeComponentandSanitizeFilenamehandle similar concerns but differ:
sanitizeComponentreplaces spaces but doesn't trim dotsSanitizeFilenametrims dots but doesn't replace spacesConsider consolidating into a single utility or documenting the intentional differences.
♻️ Option: Reuse SanitizeFilename or consolidate logic
// sanitizeComponent sanitizes a single component for use in filename // Replaces characters that are problematic in filenames with safe alternatives func sanitizeComponent(s string) string { - // Replace problematic characters - s = strings.ReplaceAll(s, "/", "-") - s = strings.ReplaceAll(s, "\\", "-") - s = strings.ReplaceAll(s, ":", "-") - s = strings.ReplaceAll(s, "*", "-") - s = strings.ReplaceAll(s, "?", "-") - s = strings.ReplaceAll(s, "\"", "-") - s = strings.ReplaceAll(s, "<", "-") - s = strings.ReplaceAll(s, ">", "-") - s = strings.ReplaceAll(s, "|", "-") - s = strings.ReplaceAll(s, " ", "-") - - return s + // Replace spaces (component-specific) then apply common sanitization + s = strings.ReplaceAll(s, " ", "-") + return SanitizeFilename(s) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transform/kustomize/serializer.go` around lines 117 - 133, sanitizeComponent currently replaces spaces but doesn't trim leading/trailing dots, while SanitizeFilename trims dots but doesn't replace spaces; update sanitizeComponent to reuse SanitizeFilename (or consolidate both into a single shared utility) so they have consistent behavior: ensure the final implementation both replaces unsafe characters (/, \, :, *, ?, ", <, >, |, spaces) and trims leading/trailing dots (use SanitizeFilename from naming.go or move its logic into a new helper and call it from sanitizeComponent), keeping the unique symbol names sanitizeComponent and SanitizeFilename unchanged for callers.transform/kustomize/naming.go (1)
28-44: Consider edge case where sanitization could produce an empty string.If
SanitizeFilenamereceives an input consisting entirely of problematic characters and dots/spaces (e.g.,"..."), it will return an empty string after trimming. This could cause issues when using the result as an actual filename.💡 Optional: Add empty string guard
func SanitizeFilename(filename string) string { // Replace problematic characters filename = strings.ReplaceAll(filename, "/", "-") filename = strings.ReplaceAll(filename, "\\", "-") filename = strings.ReplaceAll(filename, ":", "-") filename = strings.ReplaceAll(filename, "*", "-") filename = strings.ReplaceAll(filename, "?", "-") filename = strings.ReplaceAll(filename, "\"", "-") filename = strings.ReplaceAll(filename, "<", "-") filename = strings.ReplaceAll(filename, ">", "-") filename = strings.ReplaceAll(filename, "|", "-") // Remove leading/trailing dots and spaces filename = strings.Trim(filename, ". ") + if filename == "" { + return "unnamed" + } + return filename }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transform/kustomize/naming.go` around lines 28 - 44, SanitizeFilename can return an empty string for inputs made solely of replaced chars or dots/spaces; update the SanitizeFilename function to detect when the final filename is empty and return a safe fallback (e.g., "untitled" or "unnamed") instead, so callers always receive a usable filename; keep the existing replacement and Trim logic but add a final check that if filename == "" then set filename = "untitled" (or another chosen default) before returning.transform/kustomize/naming_test.go (1)
104-143: Consider adding edge case tests forSanitizeFilename.The test suite covers common cases well, but could benefit from additional edge cases:
- Empty string input
- Input consisting entirely of characters to be replaced (e.g.,
"///...")- Very long filenames
💡 Optional: Additional test cases
{ name: "empty string", input: "", expected: "", }, { name: "only problematic characters", input: "...", expected: "", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transform/kustomize/naming_test.go` around lines 104 - 143, Add edge-case table-driven tests to TestSanitizeFilename: include an "empty string" case (input "" expected ""), a "only problematic characters" case (e.g., input "///..." or "...", expected ""), and a "very long filename" case (generate a long string with allowed and disallowed chars and assert SanitizeFilename returns the sanitized long result without truncation or panics). Update the tests in naming_test.go to call SanitizeFilename for these new cases and assert equality using the existing pattern (t.Run per case and assert.Equal).transform/kustomize/serializer_test.go (1)
11-91: Consider adding test cases formoveandcopyoperations.The
SerializePatchToYAMLfunction has special handling formoveandcopyoperations (extracting thefromfield), but there are no test cases covering these operation types.💡 Suggested test case
{ name: "move operation", patchJSON: `[ {"op": "move", "from": "/spec/old", "path": "/spec/new"} ]`, expectedYAML: `- from: /spec/old op: move path: /spec/new `, expectedError: false, }, { name: "copy operation", patchJSON: `[ {"op": "copy", "from": "/spec/source", "path": "/spec/dest"} ]`, expectedYAML: `- from: /spec/source op: copy path: /spec/dest `, expectedError: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transform/kustomize/serializer_test.go` around lines 11 - 91, Add test cases to TestSerializePatchToYAML to cover "move" and "copy" ops because SerializePatchToYAML handles a "from" field specially; update the tests in serializer_test.go (inside TestSerializePatchToYAML) to include two new entries using jsonpatch.DecodePatch and asserting SerializePatchToYAML output: one with op "move" having "from": "/spec/old" and "path": "/spec/new" and the expected YAML containing "from: /spec/old\n op: move\n path: /spec/new\n", and another with op "copy" having "from": "/spec/source" and "path": "/spec/dest" and the expected YAML containing "from: /spec/source\n op: copy\n path: /spec/dest\n"; keep expectedError false and follow the same test pattern used by existing cases that call SerializePatchToYAML.transform/kustomize/kustomization.go (1)
24-31: Type duplication with no practical impact.
PatchTargetis defined identically in bothtransform/types.goandtransform/kustomize/kustomization.go, but the packages are independent—neither imports the other. SinceNewPatch()accepts individual string parameters rather than aPatchTargetobject, no conversion between types is required in the current codebase.If the codebase evolves to require type compatibility between packages, consider consolidating
PatchTargetto a shared location or adding an explicit conversion function. For now, this is a code organization issue rather than a functional problem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transform/kustomize/kustomization.go` around lines 24 - 31, PatchTarget is duplicated between transform/types.go and transform/kustomize/kustomization.go; remove the duplicate by keeping the canonical struct in transform/types.go and update transform/kustomize/kustomization.go to import and use types.PatchTarget (remove the local PatchTarget type), and update any references in functions like NewPatch (and any code in kustomize package that used the local type) to use the shared types.PatchTarget; alternatively, if you prefer not to share the type, add an explicit conversion function (e.g., ToTypesPatchTarget) in the kustomize package that converts the local struct to transform/types.PatchTarget and use that where cross-package compatibility is needed.transform/types.go (1)
113-135: SimplifysplitAPIVersionusingstrings.SplitN.The manual character-by-character parsing can be replaced with the standard library:
♻️ Proposed simplification
+import "strings" + // splitAPIVersion is a helper to split apiVersion string func splitAPIVersion(apiVersion string) []string { - result := []string{} - current := "" - - for i, char := range apiVersion { - if char == '/' { - if current != "" { - result = append(result, current) - current = "" - } - } else { - current += string(char) - } - - // Add the last part - if i == len(apiVersion)-1 && current != "" { - result = append(result, current) - } - } - - return result + return strings.SplitN(apiVersion, "/", 2) }Note:
strings.SplitNwithn=2returns at most 2 elements, matching the expected behavior forapiVersionparsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transform/types.go` around lines 113 - 135, The manual character-by-character parsing in splitAPIVersion is unnecessary; replace it with strings.SplitN(apiVersion, "/", 2) to return at most two components (group and version) and return an empty slice for empty input; update the splitAPIVersion function to import "strings" and use strings.SplitN(apiVersion, "/", 2) so behavior matches existing expectations while simplifying the implementation.transform/grouping_test.go (1)
182-236: Consider adding edge-case tests forsplitYAMLDocuments.The current tests cover common patterns but miss some edge cases that could occur in real YAML files:
- Document starting with
---(no leading newline)- Multiple consecutive separators (
---\n---\n)- Document with only
---(no content)📝 Optional additional test cases
{ name: "document starting with separator", input: `--- apiVersion: v1 kind: Service`, expectedCount: 1, }, { name: "consecutive separators", input: `apiVersion: v1 kind: Service --- --- apiVersion: v1 kind: ConfigMap`, expectedCount: 2, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transform/grouping_test.go` around lines 182 - 236, Add edge-case table-driven tests to grouping_test.go for splitYAMLDocuments: include a case where the input starts with `---` (e.g., "`---\napiVersion: v1\nkind: Service`"), a case with consecutive separators (e.g., contains "`---\n---\n`" between docs), and a case where a document is only a separator (e.g., input equals "`---`" or contains an isolated "`---`" line); update the tests slice in TestSplitYAMLDocuments to include these entries and assert expectedCount accordingly so splitYAMLDocuments handles leading separators, consecutive separators, and empty-document-only separators.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transform/grouping.go`:
- Around line 145-149: The slice-advance logic uses len(separator)
unconditionally, causing an off-by-one when idx was found for altSeparator;
update the loop that computes idx/remaining to track which separator matched
(separator vs altSeparator) and advance remaining by the actual matched
separator's length (use len(altSeparator) when altSeparator matched, otherwise
len(separator)) so you don't skip a byte; locate the code that finds idx and
sets remaining and change the index-advance branch to use the matchedSeparator
length.
- Around line 14-34: GroupResourcesByType builds groupMap and then iterates it
(map iteration is non-deterministic); to ensure deterministic output, sort the
groups by TypeKey before returning. After building groups (or by collecting keys
from groupMap), sort the slice of ResourceGroup by the TypeKey field (or collect
keys, sort them, and append groups in that order) so GroupResourcesByType always
returns groups in a stable, alphabetical order.
- Around line 132-136: The block that appends remaining to docs under the
condition "if idx == -1" is unreachable because when idx == -1 the subsequent
altSeparator search either sets idx != -1 or the inner branches always continue
or break; remove the dead branch (the if idx == -1 { docs = append(docs,
remaining); break }) from grouping.go to clean up the function that uses
variables idx, altSeparator, remaining and docs, leaving the existing handling
of the altSeparator branch intact so behavior is unchanged.
---
Nitpick comments:
In `@transform/grouping_test.go`:
- Around line 182-236: Add edge-case table-driven tests to grouping_test.go for
splitYAMLDocuments: include a case where the input starts with `---` (e.g.,
"`---\napiVersion: v1\nkind: Service`"), a case with consecutive separators
(e.g., contains "`---\n---\n`" between docs), and a case where a document is
only a separator (e.g., input equals "`---`" or contains an isolated "`---`"
line); update the tests slice in TestSplitYAMLDocuments to include these entries
and assert expectedCount accordingly so splitYAMLDocuments handles leading
separators, consecutive separators, and empty-document-only separators.
In `@transform/kustomize/kustomization.go`:
- Around line 24-31: PatchTarget is duplicated between transform/types.go and
transform/kustomize/kustomization.go; remove the duplicate by keeping the
canonical struct in transform/types.go and update
transform/kustomize/kustomization.go to import and use types.PatchTarget (remove
the local PatchTarget type), and update any references in functions like
NewPatch (and any code in kustomize package that used the local type) to use the
shared types.PatchTarget; alternatively, if you prefer not to share the type,
add an explicit conversion function (e.g., ToTypesPatchTarget) in the kustomize
package that converts the local struct to transform/types.PatchTarget and use
that where cross-package compatibility is needed.
In `@transform/kustomize/naming_test.go`:
- Around line 104-143: Add edge-case table-driven tests to TestSanitizeFilename:
include an "empty string" case (input "" expected ""), a "only problematic
characters" case (e.g., input "///..." or "...", expected ""), and a "very long
filename" case (generate a long string with allowed and disallowed chars and
assert SanitizeFilename returns the sanitized long result without truncation or
panics). Update the tests in naming_test.go to call SanitizeFilename for these
new cases and assert equality using the existing pattern (t.Run per case and
assert.Equal).
In `@transform/kustomize/naming.go`:
- Around line 28-44: SanitizeFilename can return an empty string for inputs made
solely of replaced chars or dots/spaces; update the SanitizeFilename function to
detect when the final filename is empty and return a safe fallback (e.g.,
"untitled" or "unnamed") instead, so callers always receive a usable filename;
keep the existing replacement and Trim logic but add a final check that if
filename == "" then set filename = "untitled" (or another chosen default) before
returning.
In `@transform/kustomize/serializer_test.go`:
- Around line 11-91: Add test cases to TestSerializePatchToYAML to cover "move"
and "copy" ops because SerializePatchToYAML handles a "from" field specially;
update the tests in serializer_test.go (inside TestSerializePatchToYAML) to
include two new entries using jsonpatch.DecodePatch and asserting
SerializePatchToYAML output: one with op "move" having "from": "/spec/old" and
"path": "/spec/new" and the expected YAML containing "from: /spec/old\n op:
move\n path: /spec/new\n", and another with op "copy" having "from":
"/spec/source" and "path": "/spec/dest" and the expected YAML containing "from:
/spec/source\n op: copy\n path: /spec/dest\n"; keep expectedError false and
follow the same test pattern used by existing cases that call
SerializePatchToYAML.
In `@transform/kustomize/serializer.go`:
- Around line 78-93: The filename construction uses the raw variable version
while other components use sanitized values (sanitizedNamespace, sanitizedKind,
sanitizedName, sanitizedGroup); update the code in serializer.go where filename
is built so that version is sanitized the same way as kind/name (e.g., produce
sanitizedVersion using the existing sanitizer and use sanitizedVersion in both
core and non-core fmt.Sprintf calls) to avoid unsafe characters in generated
filenames.
- Around line 34-41: The current code silently ignores errors from
op.ValueInterface() when op.Kind() != "remove"; change it so that for non-remove
operations (e.g., "add", "replace", "test") you do not swallow unexpected
errors—detect if err != nil and either return that error up the call stack or
emit a warning log including op.Kind() and the operation path (use op.Path() or
similar) before proceeding; keep the existing behavior of omitting "value" for
legitimate missing-values but ensure add/replace/test failures are surfaced via
an error return or explicit log so issues aren't masked.
- Around line 117-133: sanitizeComponent currently replaces spaces but doesn't
trim leading/trailing dots, while SanitizeFilename trims dots but doesn't
replace spaces; update sanitizeComponent to reuse SanitizeFilename (or
consolidate both into a single shared utility) so they have consistent behavior:
ensure the final implementation both replaces unsafe characters (/, \, :, *, ?,
", <, >, |, spaces) and trims leading/trailing dots (use SanitizeFilename from
naming.go or move its logic into a new helper and call it from
sanitizeComponent), keeping the unique symbol names sanitizeComponent and
SanitizeFilename unchanged for callers.
In `@transform/types.go`:
- Around line 113-135: The manual character-by-character parsing in
splitAPIVersion is unnecessary; replace it with strings.SplitN(apiVersion, "/",
2) to return at most two components (group and version) and return an empty
slice for empty input; update the splitAPIVersion function to import "strings"
and use strings.SplitN(apiVersion, "/", 2) so behavior matches existing
expectations while simplifying the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80d1b51e-e1fe-422a-82b2-9aeebffc42bf
📒 Files selected for processing (10)
transform/grouping.gotransform/grouping_test.gotransform/kustomize/kustomization.gotransform/kustomize/kustomization_test.gotransform/kustomize/naming.gotransform/kustomize/naming_test.gotransform/kustomize/serializer.gotransform/kustomize/serializer_test.gotransform/types.gotransform/types_test.go
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transform/grouping.go`:
- Around line 96-104: After unmarshaling into obj, guard against nil/empty
results before constructing an unstructured.Unstructured: if obj == nil ||
len(obj) == 0 { continue } so you skip YAML documents that are only separators
or null; update the loop that creates resource :=
unstructured.Unstructured{Object: obj} and the append to resources to only run
when obj is non-nil/non-empty to avoid downstream panics from methods like
GetKind().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Signed-off-by: Marek Aufart <maufart@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
transform/grouping.go (1)
134-136: Avoid magic number when skipping the prefix separator.Line 135 uses
4directly. Usinglen(altSeparator2)keeps this resilient to future separator changes.🧹 Proposed cleanup
- if bytes.HasPrefix(remaining, altSeparator2) { - remaining = remaining[4:] // Skip "---\n" + if bytes.HasPrefix(remaining, altSeparator2) { + remaining = remaining[len(altSeparator2):] // Skip "---\n" continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transform/grouping.go` around lines 134 - 136, Replace the magic number used when slicing off the prefix separator: instead of using the literal 4 in the block that checks bytes.HasPrefix(remaining, altSeparator2) (in transform/grouping.go), use len(altSeparator2) so the code becomes remaining = remaining[len(altSeparator2):]; this keeps the slicing resilient if altSeparator2 changes; keep the bytes.HasPrefix(remaining, altSeparator2) check as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transform/grouping.go`:
- Around line 69-71: The file write uses os.WriteFile(filename, buf.Bytes(),
0644) which is too permissive for manifests that may contain Secrets; change the
write to use more restrictive permissions (e.g., 0600 or 0640) so only the owner
(or owner+group) can read the file; update the call site where os.WriteFile is
used (referencing os.WriteFile, filename and buf.Bytes()) to pass the tighter
mode and consider making the mode configurable or document the choice.
---
Nitpick comments:
In `@transform/grouping.go`:
- Around line 134-136: Replace the magic number used when slicing off the prefix
separator: instead of using the literal 4 in the block that checks
bytes.HasPrefix(remaining, altSeparator2) (in transform/grouping.go), use
len(altSeparator2) so the code becomes remaining =
remaining[len(altSeparator2):]; this keeps the slicing resilient if
altSeparator2 changes; keep the bytes.HasPrefix(remaining, altSeparator2) check
as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Signed-off-by: Marek Aufart <maufart@redhat.com>
Related to migtools/crane#188
Summary by CodeRabbit
New Features
Tests