METAL-1583: baremetal: implement MAPI/CAPI migration #375
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@honza: This pull request references METAL-1583 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds Metal3 (Bare Metal) support across migration: registers Metal3 scheme, extends controllers and infra-init/compare logic for BareMetal, and adds bidirectional conversion implementations between MAPI BareMetal and CAPI Metal3 Machine/MachineSet types, plus related dependency and vendor updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Controller
participant MAPI as MAPI BareMetal (Machine/Set)
participant Infra as OpenShift Infrastructure
participant MAPI2CAPI as mapi2capi Metal3 Converter
participant CAPI as CAPI Machine/Set
participant Metal3 as Metal3 infra (Machine/Template/Cluster)
Controller->>MAPI: Read BareMetal Machine/Set
Controller->>Infra: Read Infrastructure
Controller->>MAPI2CAPI: FromMetal3Machine[Set]AndInfra(...)
MAPI2CAPI->>MAPI2CAPI: Decode providerSpec, map fields, collect warnings/errors
MAPI2CAPI->>Metal3: Build Metal3Machine / Metal3MachineTemplate
MAPI2CAPI->>CAPI: Build CAPI Machine/MachineSet referencing Metal3 infra
MAPI2CAPI-->>Controller: Return CAPI + Metal3 objects + warnings
sequenceDiagram
autonumber
actor Controller
participant CAPI as CAPI Machine/Set
participant Metal3 as Metal3 infra (Machine/Template/Cluster)
participant CAPI2MAPI as capi2mapi Metal3 Converter
participant MAPI as MAPI BareMetal (Machine/Set)
Controller->>CAPI: Read CAPI Machine/Set
Controller->>Metal3: Read Metal3 infra refs
Controller->>CAPI2MAPI: FromMachine[Set]AndMetal3...(...)
CAPI2MAPI->>CAPI2MAPI: Translate Metal3 fields to BareMetal providerSpec
CAPI2MAPI->>MAPI: Construct MAPI BareMetal Machine/Set (RawExtension)
CAPI2MAPI-->>Controller: Return MAPI objects + warnings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to inspect closely:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@honza: This pull request references METAL-1583 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmd/machine-api-migration/main.go (1)
199-208: Update the outdated comment.Line 199 states "Currently we only plan to support AWS" but the code now supports AWS, Bare Metal, and OpenStack. The Bare Metal addition is correct.
Apply this diff to update the comment:
- // Currently we only plan to support AWS, so all others are a noop until they're implemented. + // Currently we support AWS, Bare Metal, and OpenStack. Other platforms are a noop until implemented.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
cmd/machine-api-migration/main.go(3 hunks)pkg/controllers/common.go(3 hunks)pkg/controllers/machinesetsync/machineset_sync_controller.go(6 hunks)pkg/controllers/machinesync/machine_sync_controller.go(5 hunks)pkg/conversion/capi2mapi/metal3.go(1 hunks)pkg/conversion/mapi2capi/metal3.go(1 hunks)
🔇 Additional comments (19)
pkg/conversion/capi2mapi/metal3.go (7)
33-36: LGTM!The error variable declarations are consistent with the existing codebase patterns and provide clear error messages for nil validation.
38-50: LGTM!The type definitions follow a clean composition pattern, with the MachineSet struct properly embedding the Machine struct to enable code reuse during conversion.
52-76: LGTM!The constructor functions follow the established pattern and correctly extract Machine template data from MachineSet specs for reuse in the conversion logic.
119-154: LGTM!The ToMachine method properly validates inputs, handles errors, aggregates warnings, and follows the established conversion pattern.
156-194: LGTM!The ToMachineSet method properly validates inputs and reuses ToMachine for spec validation. The
nolint:dupldirective is expected as this pattern is replicated across platform implementations.
196-239: LGTM!The conversion helper functions properly map fields between Metal3 and BareMetal types, handle nil cases appropriately, and include helpful comments about ignored fields.
241-267: LGTM!The validation function properly identifies and reports unsupported Metal3Machine fields with clear error messages and appropriate field paths.
pkg/controllers/common.go (2)
46-47: LGTM!The Bare Metal case correctly returns Metal3Machine and Metal3Cluster types, following the established pattern for other platforms.
65-66: LGTM!The Bare Metal case correctly returns Metal3MachineTemplate and Metal3Cluster types, maintaining consistency with the Machine variant above.
cmd/machine-api-migration/main.go (1)
72-72: LGTM!The Metal3 scheme registration is correctly added and properly positioned between AWS and OpenStack, ensuring the controller runtime can work with Metal3 types.
pkg/controllers/machinesync/machine_sync_controller.go (4)
98-99: LGTM!The error variable definition follows the established pattern and is logically placed among similar error variables.
566-567: LGTM!The Bare Metal conversion case correctly calls the MAPI to CAPI converter, following the established pattern.
591-602: LGTM!The Bare Metal case properly type-asserts Metal3Machine and Metal3Cluster before calling the CAPI to MAPI converter, with appropriate error handling.
1387-1408: LGTM!The Bare Metal comparison case correctly type-asserts Metal3Machine instances and performs spec and metadata diffs, following the established pattern.
pkg/controllers/machinesetsync/machineset_sync_controller.go (5)
81-82: LGTM!The error variable definition follows the established pattern and is logically placed among similar MachineTemplate assertion errors.
674-687: LGTM!The Bare Metal case properly type-asserts Metal3MachineTemplate and Metal3Cluster before calling the CAPI to MAPI MachineSet converter, following the established pattern.
726-727: LGTM!The Bare Metal conversion case correctly calls the MAPI to CAPI MachineSet converter, maintaining consistency with other platforms.
1330-1331: LGTM!The Bare Metal case correctly returns Metal3MachineTemplateList and Metal3ClusterList, following the established pattern.
1370-1393: LGTM!The Bare Metal comparison case correctly type-asserts Metal3MachineTemplate instances and performs spec and metadata diffs, following the established pattern. The TODO comment about status comparison is consistent with other platforms.
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: "BareMetalMachineProviderSpec", | ||
| APIVersion: "baremetal.cluster.gke.io/v1alpha1", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Correct the APIVersion for BareMetalMachineProviderSpec.
Line 92 specifies APIVersion: "baremetal.cluster.gke.io/v1alpha1" which appears to be incorrect. This is an OpenShift/Red Hat bare metal provider, not a GKE provider.
Run the following script to find the correct APIVersion used in the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Find the correct APIVersion for BareMetalMachineProviderSpec in the codebase
# Search for BareMetalMachineProviderSpec type definition and its usage
rg -nP --type=go -A5 'type\s+BareMetalMachineProviderSpec\s+struct'
# Search for existing usages of BareMetalMachineProviderSpec with APIVersion
rg -nP --type=go 'APIVersion.*baremetal'Length of output: 1064
Update APIVersion for BareMetalMachineProviderSpec
In pkg/conversion/capi2mapi/metal3.go:92, change
APIVersion: "baremetal.cluster.gke.io/v1alpha1"
to
APIVersion: "baremetal.openshift.io/v1alpha1"
matching the provider’s SchemeGroupVersion.
🤖 Prompt for AI Agents
In pkg/conversion/capi2mapi/metal3.go around lines 90 to 93, the TypeMeta
APIVersion is set to "baremetal.cluster.gke.io/v1alpha1" but must match the
provider's SchemeGroupVersion; update the APIVersion value to
"baremetal.openshift.io/v1alpha1" so the TypeMeta reads APIVersion:
"baremetal.openshift.io/v1alpha1".
| if m.infrastructure == nil || m.infrastructure.Status.InfrastructureName == "" { | ||
| errs = append(errs, field.Invalid(field.NewPath("infrastructure", "status", "infrastructureName"), m.infrastructure.Status.InfrastructureName, "infrastructure cannot be nil and infrastructure.Status.InfrastructureName cannot be empty")) | ||
| } else { | ||
| capiMachine.Spec.ClusterName = m.infrastructure.Status.InfrastructureName | ||
| capiMachine.Labels[clusterv1.ClusterNameLabel] = m.infrastructure.Status.InfrastructureName | ||
| } |
There was a problem hiding this comment.
Avoid nil dereference when infra is absent.
Inside the if m.infrastructure == nil || ... branch we still evaluate m.infrastructure.Status.InfrastructureName, which panics as soon as m.infrastructure is nil. Capture the value in a local only when the struct is non-nil (or split the branch) before calling field.Invalid.
| capiMachine.Spec.ClusterName = m.infrastructure.Status.InfrastructureName | ||
| capiMachine.Labels[clusterv1.ClusterNameLabel] = m.infrastructure.Status.InfrastructureName | ||
| } |
There was a problem hiding this comment.
Initialize labels before writing to them.
capiMachine.Labels can be nil (the zero value from the conversion helper), so assigning capiMachine.Labels[...] = ... risks a panic. Ensure the map is initialized (e.g. if capiMachine.Labels == nil { capiMachine.Labels = map[string]string{} }) before writing.
🤖 Prompt for AI Agents
In pkg/conversion/mapi2capi/metal3.go around lines 125 to 127,
capiMachine.Labels may be nil and writing to it risks a panic; ensure the map is
initialized before assigning to it by checking if capiMachine.Labels == nil and,
if so, setting it to an empty map[string]string, then perform the label
assignment as shown.
|
@honza: This pull request references METAL-1583 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@honza: This pull request references METAL-1583 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vendor/sigs.k8s.io/kustomize/cmd/config/internal/generateddocs/api/docs.go (1)
147-151: Doc snippet: missing closing quote in CMD example.The Dockerfile snippet shows: CMD ["nginx-template.sh] — missing the closing quote. Should be CMD ["nginx-template.sh"].
Consider re-generating docs from upstream or fixing the generated input to avoid editing vendor manually.
Suggested correction:
- CMD ["nginx-template.sh] + CMD ["nginx-template.sh"]
🧹 Nitpick comments (4)
vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.go (2)
124-127: Library function prints to stdout; consider making this opt-in.Unconditional fmt.Println in Diff can be noisy in libraries. Prefer returning diffs to caller or gating via a flag/logger.
190-197: deleteFile swallows non-NotExist errors.Return non-ENOENT errors from Stat to avoid hiding permission/IO problems.
- if _, err := os.Stat(path); err != nil { - // return nil if file doesn't exist - return nil - } + if _, err := os.Stat(path); err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } return os.Remove(path)go.mod (1)
308-308: Add rationale comment for replace directive
This replace forces any import ofcluster-api-provider-metal3/api(including the v0.0.0-… pseudo-version pulled in by the baremetal provider) to v1.10.1 for consistent dependency resolution. Annotate this in go.mod above the directive.vendor/github.com/sergi/go-diff/diffmatchpatch/diff.go (1)
1091-1120: Potential byte vs rune mismatch in DiffXIndexThis counts len(aDiff.Text) bytes. Elsewhere, lengths often use utf8.RuneCountInString. For non‑ASCII text, byte counting can mis-map locations.
Consider switching to rune counts consistently, or clearly document that loc is byte‑based. Example change:
- if aDiff.Type != DiffInsert { chars1 += len(aDiff.Text) } - if aDiff.Type != DiffDelete { chars2 += len(aDiff.Text) } + if aDiff.Type != DiffInsert { chars1 += utf8.RuneCountInString(aDiff.Text) } + if aDiff.Type != DiffDelete { chars2 += utf8.RuneCountInString(aDiff.Text) }If API consumers pass byte offsets, keep as bytes but add a comment to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumhack/tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
e2e/go.mod(1 hunks)go.mod(2 hunks)hack/tools/go.mod(2 hunks)vendor/github.com/sergi/go-diff/AUTHORS(1 hunks)vendor/github.com/sergi/go-diff/CONTRIBUTORS(1 hunks)vendor/github.com/sergi/go-diff/LICENSE(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/diff.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/diffmatchpatch.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/match.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/mathutil.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/operation_string.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/patch.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/stringutil.go(1 hunks)vendor/modules.txt(7 hunks)vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/grep.go(1 hunks)vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.go(2 hunks)vendor/sigs.k8s.io/kustomize/cmd/config/internal/generateddocs/api/docs.go(1 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/build.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagenablehelm.go(1 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagsforfunctions.go(0 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/create/create.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbase.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbuildmetadata.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addcomponent.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addgenerator.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addmetadata.go(5 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addresource.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addtransformer.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/fix/convert.go(3 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removebuildmetadata.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeconfigmap.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removemetadata.go(1 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeresource.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removesecret.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removetransformer.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/kustfile/kustomizationfile.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/util/validate.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/localize/localize.go(6 hunks)vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.go(1 hunks)
💤 Files with no reviewable changes (1)
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagsforfunctions.go
✅ Files skipped from review due to trivial changes (5)
- vendor/github.com/sergi/go-diff/CONTRIBUTORS
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removemetadata.go
- vendor/github.com/sergi/go-diff/AUTHORS
- vendor/github.com/sergi/go-diff/diffmatchpatch/mathutil.go
- vendor/github.com/sergi/go-diff/LICENSE
🔇 Additional comments (19)
vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.go (2)
23-23: LGTM!Doc comment correctly aligned with the function name.
23-23: Inconsistency between AI summary and actual code.The AI summary states that Star/Starlark-related fields (StarPath, StarURL, StarName) and their usage were removed. However, the provided code still shows:
- Lines 86-88:
StarPath,StarURL,StarNamefields in theRunFnRunnerstruct.- Line 108: Condition still checks
r.StarPath == "" && r.StarURL == "".Yet Line 251 no longer validates star-path/url when args are provided after
--, which is inconsistent with the logic at Line 108.This suggests either:
- The AI summary is incorrect about the removal.
- The code changes are incomplete (fields declared but not removed, validation partially updated).
Since this is a vendored file, verify whether the upstream kustomize dependency intended to remove star support fully or partially.
Also applies to: 251-251
vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/grep.go (1)
57-59: Verify vendored dependency updates are intentionalThis PR modifies many files under vendor/ (go-diff, kustomize v5 commands, modules.txt). Confirm that bumping these dependencies is deliberate and pertains to the MAPI/CAPI migration. If not, revert or extract the vendor changes into a separate, focused PR.
hack/tools/go.mod (1)
11-11: Version bumps look good; ensure toolchain/builders align.kustomize v5.6.0 and cmd/config v0.19.0 match kyaml/api 0.19.x. Please ensure CI/builders use Go >=1.21 (vendor uses slices) and that go.sum/vendor are consistent post-bump.
Also applies to: 216-216
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbuildmetadata.go (1)
8-8: Use of slices.Contains is correct and simplifies logic.Looks good; ensure build uses Go >=1.21, which is already implied by the toolchain in hack/tools.
Also applies to: 64-68
vendor/github.com/sergi/go-diff/diffmatchpatch/operation_string.go (1)
1-17: Generated String() implementation is correct.Enum offset and bounds checking follow standard stringer output. No issues.
vendor/github.com/sergi/go-diff/diffmatchpatch/stringutil.go (1)
1-107: Utility functions align with canonical go-diff; LGTM.vendor/github.com/sergi/go-diff/diffmatchpatch/match.go (1)
1-161: Bitap matching implementation matches upstream behavior.Edge cases and scoring look correct.
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeresource.go (1)
77-77: LGTM: Standard library usage improves maintainability.The refactor from
kustfile.StringInSlicetoslices.Containsis correct and aligns with using the standard library for membership checks. Note that the argument order has been correctly swapped to matchslices.Contains(haystack, needle).vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/create/create.go (1)
117-117: LGTM: Consistent with standard library usage.The change to
slices.Containsis correct and consistent with the broader refactor across kustomize commands. The logic remains functionally equivalent.vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/build.go (1)
33-33: LGTM: Helm debug flag correctly integrated.The new
helmDebugflag is properly defined and propagated toHelmConfig.Debug. This enables users to enable debug output from the Helm chart inflator.Also applies to: 167-167
e2e/go.mod (1)
14-14: LGTM: Dependency version update for Bare Metal support.The
cluster-api-provider-baremetaldependency has been updated to a newer version, aligning with the Bare Metal integration work in this PR.vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagenablehelm.go (1)
34-38: LGTM: Helm debug flag registration is correct.The
--helm-debugflag is properly registered with a clear description and appropriate default value (false). This complements the flag usage inbuild.go.vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removesecret.go (1)
93-93: LGTM: Standard library usage for membership check.The refactor to
slices.Containsis correct and consistent with the broader pattern across kustomize commands. The logic remains functionally equivalent.vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addresource.go (1)
77-77: LGTM: Consistent refactor to standard library.The change to
slices.Containsis correct and aligns with the broader refactor across kustomize add/remove commands. The logic is functionally equivalent.vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbase.go (1)
1-77: Verify intentional edits to vendored dependenciesVendor directory changes should be deliberate and maintained via Go modules. Please confirm:
- These patches have been upstreamed to the kustomize repo or otherwise reconciled.
go mod vendorwas run after updating dependencies.- All modified vendor files in this PR:
base=$(git merge-base HEAD main) git diff --name-only $base HEAD | grep '^vendor/'vendor/github.com/sergi/go-diff/diffmatchpatch/diff.go (3)
86-99: Vendored diffmatchpatch package is complete All referenced symbols (DiffMatchPatch, unescaper, max, runesEqual, runesIndex, runesIndexOf, indexOf, intArrayToString) are defined in vendor/github.com/sergi/go-diff/diffmatchpatch.
1-7: Vendored dependency includes LICENSE
The filevendor/github.com/sergi/go-diff/LICENSEis present as referenced in the headers.
405-423: Comma splitting safely preserves multi-digit indices
intArrayToString emits comma-separated decimal strings (with no embedded commas), so strings.Split(aDiff.Text, ",") cannot fragment multi-digit numbers.
| blankLine1 := lineBreak1 && blanklineEndRegex.MatchString(one) | ||
| blankLine2 := lineBreak2 && blanklineEndRegex.MatchString(two) | ||
|
|
There was a problem hiding this comment.
Fix boundary regex: use blanklineStartRegex for the trailing side
blankLine2 should match the start-of-blank-line on the right segment. Using blanklineEndRegex on both sides skews semantic scoring and can worsen cleanup results.
- blankLine2 := lineBreak2 && blanklineEndRegex.MatchString(two)
+ blankLine2 := lineBreak2 && blanklineStartRegex.MatchString(two)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| blankLine1 := lineBreak1 && blanklineEndRegex.MatchString(one) | |
| blankLine2 := lineBreak2 && blanklineEndRegex.MatchString(two) | |
| blankLine1 := lineBreak1 && blanklineEndRegex.MatchString(one) | |
| blankLine2 := lineBreak2 && blanklineStartRegex.MatchString(two) |
🤖 Prompt for AI Agents
In vendor/github.com/sergi/go-diff/diffmatchpatch/diff.go around lines 755 to
757, blankLine2 is currently computed using blanklineEndRegex but should test
the start-of-blank-line on the right segment; change the computation so
blankLine2 uses blanklineStartRegex (e.g. set blankLine2 := lineBreak2 &&
blanklineStartRegex.MatchString(two)) so the boundary regexes reflect left/end
vs right/start correctly.
| buildCmd := build.NewCmdBuild(fs, &build.Help{}, &buildBuffer) | ||
| cmd := &cobra.Command{ | ||
| Use: "localize [target [destination]]", |
There was a problem hiding this comment.
Fix verification buffer handling
buildBuffer is copied when passed into runBuildCmd. We reset and read only the copy, while buildCmd keeps writing to the original that never gets cleared. As a result both builds appear empty, so verification always “succeeds” even when localization changes output. Pass the pointer through (and update the call sites) so we reset/read the actual writer.
@@
- var buildBuffer bytes.Buffer
- buildCmd := build.NewCmdBuild(fs, &build.Help{}, &buildBuffer)
+ var buildBuffer bytes.Buffer
+ buildCmd := build.NewCmdBuild(fs, &build.Help{}, &buildBuffer)
@@
- originalBuild, err := runBuildCmd(buildBuffer, buildCmd, args.target)
+ originalBuild, err := runBuildCmd(&buildBuffer, buildCmd, args.target)
@@
- localizedBuild, err := runBuildCmd(buildBuffer, buildCmd, buildDst)
+ localizedBuild, err := runBuildCmd(&buildBuffer, buildCmd, buildDst)
@@
-func runBuildCmd(buffer bytes.Buffer, cmd *cobra.Command, folder string) (buildOutput string, err error) {
- buffer.Reset()
+func runBuildCmd(buffer *bytes.Buffer, cmd *cobra.Command, folder string) (buildOutput string, err error) {
+ buffer.Reset()
@@
- return buffer.String(), nil
+ return buffer.String(), nilAlso applies to: 74-94, 133-144
| // path is an absolute path, rather than a path relative to src. | ||
| // e.g. if src is /path/to/package, then path might be /path/to/package/and/sub/dir | ||
| // we need the path relative to src `and/sub/dir` when we are copying the files to dest. | ||
| copyTo := strings.TrimPrefix(path, src) | ||
|
|
||
| // make directories that don't exist | ||
| if info.IsDir() { | ||
| return errors.Wrap(fSys.MkdirAll(filepath.Join(dst, copyTo))) | ||
| } | ||
|
|
||
| // copy file by reading and writing it | ||
| b, err := fSys.ReadFile(filepath.Join(src, copyTo)) | ||
| if err != nil { | ||
| return errors.Wrap(err) | ||
| } | ||
| err = fSys.WriteFile(filepath.Join(dst, copyTo), b) | ||
| if err != nil { | ||
| return errors.Wrap(err) | ||
| } |
There was a problem hiding this comment.
Critical: CopyDir may write outside dst due to leading separator in copyTo.
Using strings.TrimPrefix(path, src) can leave a leading path separator, causing filepath.Join(dst, copyTo) to ignore dst. Use filepath.Rel to ensure a proper relative path.
Apply this fix:
- // don't copy the .git dir
- if path != src {
- rel := strings.TrimPrefix(path, src)
- if IsDotGitFolder(rel) {
- return nil
- }
- }
-
- // path is an absolute path, rather than a path relative to src.
- // e.g. if src is /path/to/package, then path might be /path/to/package/and/sub/dir
- // we need the path relative to src `and/sub/dir` when we are copying the files to dest.
- copyTo := strings.TrimPrefix(path, src)
+ // Compute path relative to src to avoid leading separators.
+ relPath, relErr := filepath.Rel(src, path)
+ if relErr != nil {
+ return errors.Wrap(relErr)
+ }
+ // don't copy the .git dir
+ if path != src && IsDotGitFolder(relPath) {
+ return nil
+ }
+ copyTo := relPath- if info.IsDir() {
- return errors.Wrap(fSys.MkdirAll(filepath.Join(dst, copyTo)))
- }
+ if info.IsDir() {
+ return errors.Wrap(fSys.MkdirAll(filepath.Join(dst, copyTo)))
+ }- b, err := fSys.ReadFile(filepath.Join(src, copyTo))
+ b, err := fSys.ReadFile(filepath.Join(src, copyTo))- err = fSys.WriteFile(filepath.Join(dst, copyTo), b)
+ err = fSys.WriteFile(filepath.Join(dst, copyTo), b)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.go around lines 34-52,
replace the strings.TrimPrefix logic with a safe relative-path computation: call
filepath.Rel(src, path) and handle its error, then reject any returned rel that
is ".." or starts with ".."+os.PathSeparator to prevent escaping dst, and use
filepath.Clean(rel) when joining; use filepath.Join(dst, rel) for both MkdirAll
and ReadFile/WriteFile paths and wrap errors as before. Ensure you import os if
needed and return an error when the relative path is invalid.
|
@honza: This pull request references METAL-1583 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addmetadata.go (1)
1-216: Revert direct modifications to vendored kustomize code; update dependency instead
Files under vendor/sigs.k8s.io/kustomize must only change via an upstream version bump, not by direct edits. Please remove these changes and bump or upstream them appropriately.vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.go (2)
86-88: Remove orphaned Star-related fields from RunFnRunner struct.The AI summary indicates Star-based function support was removed, including flags and the
fnAnnotationForStarfunction. However, theStarPath,StarURL, andStarNamefields remain in the struct definition.These fields appear to be orphaned—they're referenced in validation logic (line 108) but are no longer settable via flags, making them dead code that cannot be reached.
Consider removing these fields in a follow-up to complete the Star removal:
type RunFnRunner struct { IncludeSubpackages bool Command *cobra.Command DryRun bool GlobalScope bool FnPaths []string Image string - StarPath string - StarURL string - StarName string EnableExec bool ExecPath string RunFns runfn.RunFns ResultsDir string Network bool Mounts []string LogSteps bool Env []string AsCurrentUser bool }
108-124: Simplify condition to remove unreachable Star checks.Line 108 checks
r.StarPathandr.StarURL, but:
- No command-line flags populate these fields (removed per AI summary)
- The switch statement (lines 119-124) only handles
ImageandExecPathcases- This creates dead code that can never execute
Simplify the condition to match the actual supported cases:
func (r *RunFnRunner) getContainerFunctions(dataItems []string) ( []*yaml.RNode, error) { - if r.Image == "" && r.StarPath == "" && r.ExecPath == "" && r.StarURL == "" { + if r.Image == "" && r.ExecPath == "" { return nil, nil }pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
369-396: Add Metal3 to filterOutdatedInfraMachineTemplates (otherwise cleanup fails on Bare Metal)Without handling Metal3MachineTemplateList, outdated InfraMachineTemplates won’t be deleted for BareMetal clusters. Add a Metal3 branch.
func filterOutdatedInfraMachineTemplates(infraMachineTemplateList client.ObjectList, newInfraMachineTemplateName string) ([]client.Object, error) { outdatedTemplates := []client.Object{} switch list := infraMachineTemplateList.(type) { case *awsv1.AWSMachineTemplateList: for _, template := range list.Items { if template.GetName() != newInfraMachineTemplateName { outdatedTemplates = append(outdatedTemplates, &template) } } + case *metal3v1.Metal3MachineTemplateList: + for _, template := range list.Items { + if template.GetName() != newInfraMachineTemplateName { + outdatedTemplates = append(outdatedTemplates, &template) + } + } case *ibmpowervsv1.IBMPowerVSMachineTemplateList: for _, template := range list.Items { if template.GetName() != newInfraMachineTemplateName { outdatedTemplates = append(outdatedTemplates, &template) } } case *openstackv1.OpenStackMachineTemplateList: for _, template := range list.Items { if template.GetName() != newInfraMachineTemplateName { outdatedTemplates = append(outdatedTemplates, &template) } } default: return nil, fmt.Errorf("%w: got unknown type %T", errUnexpectedInfraMachineTemplateListType, list) } return outdatedTemplates, nil }pkg/controllers/machinesync/machine_sync_controller.go (4)
351-354: Avoid nil event object when mirroring (mapiMachine can be nil)When creating the MAPI mirror, mapiMachine is nil. Recording an event on nil can panic. Use capiMachine as the event object when mapiMachine is nil.
for _, warning := range warns { logger.Info("Warning during conversion", "warning", warning) - r.Recorder.Event(mapiMachine, corev1.EventTypeWarning, "ConversionWarning", warning) + // mapiMachine may be nil when we're creating a mirror; fall back to capiMachine. + var eventObj runtime.Object = mapiMachine + if eventObj == nil { + eventObj = capiMachine + } + r.Recorder.Event(eventObj, corev1.EventTypeWarning, "ConversionWarning", warning) }
1056-1064: Fix error handling: don’t return an error if RemoveFinalizer succeedsCurrently always returns a non‑nil error (even when err is nil). Check err and return nil on success.
if capiMachine == nil && util.IsNilObject(infraMachine) { logger.Info("Cluster API machine and infra machine do not exist, removing corresponding Machine API machine sync finalizer") // We don't have a capi machine or infra resouorces to clean up we can // just let the MAPI operators function as normal, and remove our own sync // finalizer. - _, err := util.RemoveFinalizer(ctx, r.Client, mapiMachine, SyncFinalizer) - - return false, fmt.Errorf("failed to remove finalizer: %w", err) + if _, err := util.RemoveFinalizer(ctx, r.Client, mapiMachine, SyncFinalizer); err != nil { + return false, fmt.Errorf("failed to remove finalizer: %w", err) + } + return false, nil }
1177-1184: Fix error handling in CAPI→MAPI deletion (nil error wrap)Same issue: only return error if RemoveFinalizer fails.
if mapiMachine == nil { logger.Info("Machine API machine does not exist, removing corresponding Cluster API machine sync finalizer") // We don't have a MAPI machine just let the CAPI operators function as // normal, and remove our own sync finalizer. - _, err := util.RemoveFinalizer(ctx, r.Client, capiMachine, SyncFinalizer) - - return false, fmt.Errorf("failed to remove finalizer: %w", err) + if _, err := util.RemoveFinalizer(ctx, r.Client, capiMachine, SyncFinalizer); err != nil { + return false, fmt.Errorf("failed to remove finalizer: %w", err) + } + return false, nil }
1316-1360: Make compareMAPIMachines tolerant to non‑AWS providerSpecs; prefer Metal3 decode when availableDo not abort on AWS decode failure — try AWSProviderSpecFromRawExtension first, then BareMetalProviderSpecFromRawExtension (pkg/conversion/mapi2capi/metal3.go) and compare when available; if no provider decoder succeeds, skip the providerSpec diff and continue comparing Spec/Metadata. Mirror the same pattern in pkg/controllers/machinesetsync/machineset_sync_controller.go.
♻️ Duplicate comments (4)
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/localize/localize.go (1)
34-35: Critical: Buffer handling bug makes verification ineffective.The past review comment is still valid.
buildBufferis passed by value torunBuildCmd(lines 74, 84), andrunBuildCmdreceives it by value (line 133). Whenbuffer.Reset()executes (line 134), it only resets the local copy insiderunBuildCmd, not the original buffer thatbuildCmdwrites to. As a result:
- Both
runBuildCmdcalls read from empty local copies- Both return empty strings
- Verification always "succeeds" (
"" == "") even when builds differThis completely breaks the verification feature.
Apply the fix from the previous review:
- var buildBuffer bytes.Buffer - buildCmd := build.NewCmdBuild(fs, &build.Help{}, &buildBuffer) + var buildBuffer bytes.Buffer + buildCmd := build.NewCmdBuild(fs, &build.Help{}, &buildBuffer)- originalBuild, err := runBuildCmd(buildBuffer, buildCmd, args.target) + originalBuild, err := runBuildCmd(&buildBuffer, buildCmd, args.target)- localizedBuild, err := runBuildCmd(buildBuffer, buildCmd, buildDst) + localizedBuild, err := runBuildCmd(&buildBuffer, buildCmd, buildDst)-func runBuildCmd(buffer bytes.Buffer, cmd *cobra.Command, folder string) (buildOutput string, err error) { - buffer.Reset() +func runBuildCmd(buffer *bytes.Buffer, cmd *cobra.Command, folder string) (buildOutput string, err error) { + buffer.Reset()- return buffer.String(), nil + return buffer.String(), nilAlso applies to: 74-77, 84-87, 133-144
vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.go (1)
20-56: CRITICAL: Path traversal vulnerability confirmed (vendored code).The path traversal vulnerability flagged in the previous review remains unaddressed. Using
strings.TrimPrefix(path, src)at lines 28 and 37 can leave a leading path separator (e.g., whenpath="/abs/src/file"andsrc="/abs/src", the result is"/file"). When this result is passed tofilepath.Join(dst, copyTo)at lines 41, 45, and 49, the absolute path causesdstto be ignored, allowing writes outside the destination directory.Since this is vendored code from
sigs.k8s.io/kustomize/kyaml/copyutil, the proper fix must come from upstream. However, given the severity:
- Immediate action: Check if a newer version of kustomize/kyaml has fixed this issue and update the vendor dependency.
- If no upstream fix exists: Consider reporting this to the kustomize project or applying a local patch with the
filepath.Relapproach suggested in the previous review.Verify the upstream kustomize version for this fix:
#!/bin/bash # Check current vendored version and available updates cd vendor/sigs.k8s.io/kustomize/kyaml git log -1 --format="%H %s" 2>/dev/null || echo "Not a git submodule" # Search for any existing issues or fixes in the repository gh api graphql -f query=' { search(query: "repo:kubernetes-sigs/kustomize path:kyaml/copyutil TrimPrefix OR filepath.Rel", type: ISSUE, first: 10) { nodes { ... on Issue { title state url createdAt } } } }'pkg/conversion/capi2mapi/metal3.go (1)
90-99: Use the provider’s registered GroupVersion when populating TypeMeta.Hard-coding
baremetal.cluster.k8s.io/v1alpha1leaves us with the wrong GVK forBareMetalMachineProviderSpec. Please drive this from the scheme (baremetalv1.GroupVersion.String()) so the serialized providerSpec carries the correct API version and stays aligned with future bumps.- TypeMeta: metav1.TypeMeta{ - Kind: "BareMetalMachineProviderSpec", - APIVersion: "baremetal.cluster.k8s.io/v1alpha1", - }, + TypeMeta: metav1.TypeMeta{ + Kind: "BareMetalMachineProviderSpec", + APIVersion: baremetalv1.GroupVersion.String(), + },vendor/github.com/sergi/go-diff/diffmatchpatch/diff.go (1)
755-757: Fix boundary regex: use blanklineStartRegex for the right segmentblankLine2 should test the start-of-blank-line on the right side to balance left/end vs right/start.
- blankLine1 := lineBreak1 && blanklineEndRegex.MatchString(one) - blankLine2 := lineBreak2 && blanklineEndRegex.MatchString(two) + blankLine1 := lineBreak1 && blanklineEndRegex.MatchString(one) + blankLine2 := lineBreak2 && blanklineStartRegex.MatchString(two)
🧹 Nitpick comments (5)
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/localize/localize.go (2)
108-113: Minor: Inconsistent indentation in flag description.The flag description string has inconsistent leading whitespace (tabs) that makes the formatting uneven. Consider cleaning up the indentation for better readability.
cmd.Flags().BoolVar(&f.noVerify, "no-verify", false, - `Does not verify that the outputs of kustomize build for target and newDir are the same after localization. - If not specified, this flag defaults to false and will run kustomize build. - `) + `Does not verify that the outputs of kustomize build for target and newDir are the same after localization. +If not specified, this flag defaults to false and will run kustomize build.`)
136-140: Error guidance may confuse users in common scenarios.The error message (lines 137-139) specifically mentions "executable permissions for downloaded exec binaries," which applies to plugin/helm scenarios. However, this message is shown for ANY build error, including common issues like syntax errors in
kustomization.yamlor missing resources. Users encountering basic errors will see confusing guidance about exec permissions.Consider either:
- Making the message more generic, or
- Conditionally showing exec-permission guidance only when the error suggests permission issues
Example of a more generic message:
buildErr := cmd.RunE(cmd, []string{folder}) if buildErr != nil { - log.Printf("If your target directory requires flags to build: \n"+ - "1. Add executable permissions for the downloaded exec binaries in '%s'. \n"+ - "2. Run kustomize build with the necessary flags and self-verify the outputs.", folder) + log.Printf("Build failed for '%s'. If your target requires special flags or uses plugins/helm charts, "+ + "you may need to run kustomize build manually with appropriate flags to verify the localization.", folder) return "", errors.Wrap(buildErr) }vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.go (2)
58-131: Refactor path handling for robustness.The double
strings.TrimPrefixpattern at lines 75 and 94 is fragile and can produce incorrect results in edge cases (e.g., whensourceDir="/path"and a file is named"/pathological", the firstTrimPrefixproduces"ological"instead of failing). While this doesn't create a security vulnerability (no write operations), it can cause incorrect diff results.For consistency with the recommended fix in
CopyDirand to avoid subtle path bugs, usefilepath.Relto compute relative paths:// get set of filenames in the package source upstreamFiles := sets.String{} err := filepath.Walk(sourceDir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } // skip git repo if it exists if IsDotGitFolder(path) { return nil } - upstreamFiles.Insert(strings.TrimPrefix(strings.TrimPrefix(path, sourceDir), string(filepath.Separator))) + relPath, err := filepath.Rel(sourceDir, path) + if err != nil { + return err + } + upstreamFiles.Insert(relPath) return nil })Apply the same pattern for
localFilesat lines 83-96:// get set of filenames in the cloned package localFiles := sets.String{} err = filepath.Walk(destDir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } // skip git repo if it exists if IsDotGitFolder(path) { return nil } - localFiles.Insert(strings.TrimPrefix(strings.TrimPrefix(path, destDir), string(filepath.Separator))) + relPath, err := filepath.Rel(destDir, path) + if err != nil { + return err + } + localFiles.Insert(relPath) return nil })
190-197: LGTM with optional refinement.The deletion logic correctly treats missing files as success, which is appropriate for a cleanup helper.
For slightly better clarity, consider checking specifically for
os.ErrNotExist:+import "errors" + // deleteFile deletes file from path, returns no error if file doesn't exist func deleteFile(path string) error { - if _, err := os.Stat(path); err != nil { - // return nil if file doesn't exist - return nil + if _, err := os.Stat(path); err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil + } + return err } return os.Remove(path) }pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
1465-1516: Make compareMAPIMachineSets tolerant to non-AWS and bare-metal providerSpecs
Wrap the AWS decode in anif err==nilblock to skip non-AWS specs, then add anelse ifbranch usingmapi2capi.BareMetalProviderSpecFromRawExtensionto diff bare-metal providerSpecs. If neither decode succeeds, fall back to best-effort by comparing Spec, Metadata, and Status only.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumhack/tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (45)
cmd/machine-api-migration/main.go(3 hunks)e2e/go.mod(1 hunks)go.mod(2 hunks)hack/tools/go.mod(2 hunks)pkg/controllers/common.go(3 hunks)pkg/controllers/machinesetsync/machineset_sync_controller.go(7 hunks)pkg/controllers/machinesync/machine_sync_controller.go(6 hunks)pkg/conversion/capi2mapi/metal3.go(1 hunks)pkg/conversion/mapi2capi/metal3.go(1 hunks)vendor/github.com/sergi/go-diff/AUTHORS(1 hunks)vendor/github.com/sergi/go-diff/CONTRIBUTORS(1 hunks)vendor/github.com/sergi/go-diff/LICENSE(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/diff.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/diffmatchpatch.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/match.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/mathutil.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/operation_string.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/patch.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/stringutil.go(1 hunks)vendor/modules.txt(7 hunks)vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/grep.go(1 hunks)vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.go(2 hunks)vendor/sigs.k8s.io/kustomize/cmd/config/internal/generateddocs/api/docs.go(1 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/build.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagenablehelm.go(1 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagsforfunctions.go(0 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/create/create.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbase.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbuildmetadata.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addcomponent.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addgenerator.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addmetadata.go(5 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addresource.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addtransformer.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/fix/convert.go(3 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removebuildmetadata.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeconfigmap.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removemetadata.go(1 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeresource.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removesecret.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removetransformer.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/kustfile/kustomizationfile.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/util/validate.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/localize/localize.go(6 hunks)vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.go(1 hunks)
💤 Files with no reviewable changes (1)
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagsforfunctions.go
✅ Files skipped from review due to trivial changes (1)
- vendor/github.com/sergi/go-diff/LICENSE
🚧 Files skipped from review as they are similar to previous changes (15)
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addresource.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbuildmetadata.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/build.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/create/create.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagenablehelm.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removemetadata.go
- vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/grep.go
- vendor/github.com/sergi/go-diff/diffmatchpatch/diffmatchpatch.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/kustfile/kustomizationfile.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addcomponent.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removetransformer.go
- vendor/github.com/sergi/go-diff/diffmatchpatch/operation_string.go
- vendor/sigs.k8s.io/kustomize/cmd/config/internal/generateddocs/api/docs.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/util/validate.go
- vendor/modules.txt
🔇 Additional comments (36)
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removebuildmetadata.go (1)
7-7: Vendored dependency update: standard library modernization.This change is part of an upstream kustomize dependency update that modernizes the code to use Go's standard library
slices.Containsinstead of a custom helper function. The logic is equivalent and correct.Also applies to: 64-64
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addgenerator.go (1)
9-9: Vendored kustomize updated upstream
Bulk replacements ofkustfile.StringInSlicewithslices.Containsoccur across multiple vendored kustomize files, indicating an upstream kustomize v5 vendor update. No manual modifications are needed on this file.vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removesecret.go (1)
10-10: Do not manually modify vendored code
Vendored files must match upstream exactly. Confirm whether removesecret.go’s import of"slices"and the switch toslices.Containsexist in the kustomize/v5 upstream repository. If they do, bump the kustomize version in go.mod and re-vendor; if not, revert these edits.vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addtransformer.go (1)
9-9: Ignore intentional vendor update
Changes invendor/sigs.k8s.io/kustomize/.../addtransformer.goare from the vendoring commit (b02dfe1) migrating to Go’sslices.Contains. No action required.Likely an incorrect or invalid review comment.
vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.go (3)
133-143: LGTM!The
.gitdetection logic is correct and handles nested paths properly. The use offilepath.Cleanandfilepath.ToSlashensures consistent path normalization before checking components.
145-152: LGTM!Standard and correct usage of the diffmatchpatch library for line-based diff generation.
154-188: LGTM!The file synchronization logic is correct:
- Handles missing source files by cleaning up the destination
- Preserves destination permissions when present (reasonable default for sync operations)
- Properly propagates errors from all file system operations
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeconfigmap.go (1)
10-10: Confirm upstream origin of vendored change.
This replaceskustfile.StringInSlicewithslices.Containsin a vendored file. Please confirm:
- Which upstream kustomize version or commit includes this refactor?
- If it’s a manual patch, why modify vendored code instead of updating the dependency?
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeresource.go (1)
9-9: Confirm vendored update is upstream
Verify that go.mod’srequire sigs.k8s.io/kustomize/kustomize/v5 v5.x.xmatches this vendor change and rerungo mod vendorto ensure the slice import andslices.Containsusage came from an upstream kustomize/v5 release.vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.go (3)
251-253: Validation logic correctly updated for Star removal.The condition now properly requires either
--imageor--enable-execwith--exec-pathwhen function arguments are provided (indicated byArgsLenAtDash >= 0). This is consistent with removing Star-based function support.However, this change is in a vendored file. Ensure this modification aligns with an upstream kustomize update rather than a manual edit.
1-314: Clarify the kustomize vendor update in this PR.Changes under vendor/sigs.k8s.io/kustomize (run-fns.go, commit b02dfe1) appear to be a kustomize vendoring update with no corresponding go.mod version bump, yet the PR scope is “METAL-1583: baremetal: implement MAPI/CAPI migration.”
• Please extract this vendoring change into a standalone PR or justify its inclusion here.
23-23: Vendored kustomize update is intentional; no manual edits in run-fns.go. go mod verify passes and git log shows a “tidy, verify, and vendor” refresh commit.vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/fix/convert.go (3)
59-63: Functional change is correct.The replacement of
stringInSlicewithslices.Containsis semantically equivalent and correct. The logic continues to skip files that are not kustomization files.Note: See the earlier comment regarding modification of vendored files.
227-227: LGTM!The replacement of
indexOfwithslices.Indexis correct. Both return -1 when the element is not found, and the existing error handling at lines 228-231 correctly handles this case.
10-10: No action needed: Go version ≥1.21 confirmed.All modules specify Go ≥1.21, so the
slicesimport is supported.go.mod (3)
19-19: LGTM: Pseudo-version format is correct.The pseudo-version for
cluster-api-provider-baremetalfollows Go's pseudo-version format (v0.0.0-timestamp-commit). This is appropriate for referencing a specific commit.
308-308: Ensure replace directive version matches the require directive.The replace directive pins
metal3-io/cluster-api-provider-metal3/apito v1.10.1, matching line 13. However, if v1.10.1 doesn't exist (as suggested by learnings), both lines will need correction once the correct version is identified.
13-13: No action required – Metal3 API v1.10.1 tag confirmed Theapi/v1.10.1tag exists in the metal3-io/cluster-api-provider-metal3 repository.hack/tools/go.mod (2)
11-11: LGTM: Kustomize version update.The update from v5.4.1 to v5.6.0 is a minor version bump. This aligns with the broader Metal3 integration and ensures tooling compatibility.
216-216: LGTM: Indirect dependency update aligns with kustomize.The update of
sigs.k8s.io/kustomize/cmd/configfrom v0.14.0 to v0.19.0 is expected when upgrading kustomize to v5.6.0.cmd/machine-api-migration/main.go (3)
24-24: LGTM: Metal3 import added.The import of
metal3v1follows the established pattern for other cloud providers (AWS, OpenStack) and is necessary for Bare Metal platform support.
72-72: LGTM: Metal3 scheme registration.The scheme registration follows the same pattern as other providers (lines 71, 73-74) and correctly uses
utilruntime.Mustfor error handling.
201-201: LGTM: Bare Metal platform type added to switch.The addition of
configv1.BareMetalPlatformTypecorrectly extends the controller startup logic to support Bare Metal clusters alongside AWS and OpenStack.vendor/github.com/sergi/go-diff/AUTHORS (1)
1-26: LGTM: Standard vendor metadata file.This AUTHORS file follows the standard format for listing copyright holders in a vendored Go library. The content is appropriate for vendor documentation.
vendor/github.com/sergi/go-diff/diffmatchpatch/mathutil.go (1)
11-23: LGTM: Standard min/max helper functions.These unexported utility functions are correctly implemented and are part of the vendored go-diff library. While Go 1.21+ includes built-in min/max functions, this vendored code maintains its own implementation for compatibility.
e2e/go.mod (1)
14-14: LGTM: Consistent dependency update.The update of
cluster-api-provider-baremetalpseudo-version aligns with the root go.mod (line 19), ensuring consistency across all modules in the repository.vendor/github.com/sergi/go-diff/CONTRIBUTORS (1)
1-33: LGTM: Standard vendor metadata file.This CONTRIBUTORS file follows the standard format for listing contributors to a vendored Go library. The distinction from AUTHORS (copyright holders vs. contributors) is clearly documented in the header.
pkg/controllers/common.go (3)
22-22: LGTM: Metal3 import added.The import of
metal3v1is necessary for Bare Metal platform support and follows the established pattern for other cloud providers in this file.
46-47: LGTM: Bare Metal case added for Machine and Cluster initialization.The addition correctly returns
Metal3MachineandMetal3Clusterfor Bare Metal platforms, following the same pattern as AWS (lines 44-45) and OpenStack (lines 48-49).
65-66: LGTM: Bare Metal case added for MachineTemplate and Cluster initialization.The addition correctly returns
Metal3MachineTemplateandMetal3Clusterfor Bare Metal platforms, following the same pattern as other providers in this function.pkg/controllers/machinesetsync/machineset_sync_controller.go (3)
674-688: Metal3 CAPI→MAPI MachineSet conversion: LGTMType assertions and delegation to Metal3 converter look correct.
726-728: Metal3 MAPI→CAPI MachineSet conversion: LGTMConstructor path mirrors other providers and returns MachineSet + MachineTemplate.
1331-1332: Metal3 lists initializer: LGTMReturning Metal3MachineTemplateList and Metal3ClusterList is correct.
pkg/controllers/machinesync/machine_sync_controller.go (3)
566-568: Metal3 MAPI→CAPI Machine conversion: LGTM
592-603: Metal3 CAPI→MAPI Machine conversion: LGTM
1389-1406: Metal3 infra-machine compare: LGTMSpec and metadata diffs mirror other providers.
| typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*metal3v1.Metal3MachineTemplate) | ||
| if !ok { | ||
| return nil, errAssertingCAPIMetal3MachineTemplate | ||
| } | ||
|
|
||
| typedinfraMachineTemplate2, ok := infraMachineTemplate2.(*metal3v1.Metal3MachineTemplate) | ||
| if !ok { | ||
| return nil, errAssertingCAPIMetal3MachineTemplate | ||
| } | ||
|
|
||
| diff := make(map[string]any) | ||
|
|
||
| if diffSpec := deep.Equal(typedInfraMachineTemplate1.Spec, typedinfraMachineTemplate2.Spec); len(diffSpec) > 0 { | ||
| diff[".spec"] = diffSpec | ||
| } | ||
|
|
||
| if diffObjectMeta := util.ObjectMetaEqual(typedInfraMachineTemplate1.ObjectMeta, typedinfraMachineTemplate2.ObjectMeta); len(diffObjectMeta) > 0 { | ||
| diff[".metadata"] = diffObjectMeta | ||
| } | ||
|
|
||
| // TODO: Evaluate if we want to add status comparison if needed in the future (e.g. for scale from zero capacity). | ||
|
|
There was a problem hiding this comment.
Fix OpenStack error constant; consider consistent metadata diff
- Line 1397: On OpenStack, wrong error constant is returned on type assertion failure (IBMPowerVS instead of OpenStack). Use errAssertingCAPIOpenStackMachineTemplate.
- Optional: Use util.ObjectMetaEqual for metadata like other providers to avoid noisy diffs on transient fields.
case configv1.OpenStackPlatformType:
typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*openstackv1.OpenStackMachineTemplate)
if !ok {
- return nil, errAssertingCAPIIBMPowerVSMachineTemplate
+ return nil, errAssertingCAPIOpenStackMachineTemplate
}
typedinfraMachineTemplate2, ok := infraMachineTemplate2.(*openstackv1.OpenStackMachineTemplate)
if !ok {
return nil, errAssertingCAPIOpenStackMachineTemplate
}
diff := make(map[string]any)
if diffSpec := deep.Equal(typedInfraMachineTemplate1.Spec, typedinfraMachineTemplate2.Spec); len(diffSpec) > 0 {
diff[".spec"] = diffSpec
}
- if diffObjectMeta := deep.Equal(typedInfraMachineTemplate1.ObjectMeta, typedinfraMachineTemplate2.ObjectMeta); len(diffObjectMeta) > 0 {
+ // Optional: keep consistent with other providers
+ if diffObjectMeta := util.ObjectMetaEqual(typedInfraMachineTemplate1.ObjectMeta, typedinfraMachineTemplate2.ObjectMeta); len(diffObjectMeta) > 0 {
diff[".metadata"] = diffObjectMeta
}Also applies to: 1395-1403
🤖 Prompt for AI Agents
In pkg/controllers/machinesetsync/machineset_sync_controller.go around lines
1371-1392 (also apply to 1395-1403), fix the incorrect error constant used on
the type-assertion failure for the OpenStack provider by returning
errAssertingCAPIOpenStackMachineTemplate instead of the
IBMPowerVS/OpenStack-mismatched constant; additionally, make the metadata
comparison use util.ObjectMetaEqual (same approach as spec) to avoid noisy diffs
from transient metadata fields.
| if m.machine == nil || m.metal3Machine == nil || m.metal3Cluster == nil { | ||
| return nil, nil, errCAPIMachineMetal3MachineMetal3ClusterCannotBeNil | ||
| } | ||
|
|
||
| var ( | ||
| errors field.ErrorList | ||
| warnings []string | ||
| ) |
There was a problem hiding this comment.
Guard infrastructure access and initialize labels before writing.
m.infrastructure can be nil, so m.infrastructure.Status.InfrastructureName will panic. The same block also writes into capiMachine.Labels without ensuring the map exists. Both crash the conversion path.
- if m.infrastructure == nil || m.infrastructure.Status.InfrastructureName == "" {
- errs = append(errs, field.Invalid(field.NewPath("infrastructure", "status", "infrastructureName"), m.infrastructure.Status.InfrastructureName, "infrastructure cannot be nil and infrastructure.Status.InfrastructureName cannot be empty"))
- } else {
- capiMachine.Spec.ClusterName = m.infrastructure.Status.InfrastructureName
- capiMachine.Labels[clusterv1.ClusterNameLabel] = m.infrastructure.Status.InfrastructureName
- }
+ clusterNamePath := field.NewPath("infrastructure", "status", "infrastructureName")
+ switch {
+ case m.infrastructure == nil:
+ errs = append(errs, field.Invalid(clusterNamePath, nil, "infrastructure cannot be nil"))
+ case m.infrastructure.Status.InfrastructureName == "":
+ errs = append(errs, field.Invalid(clusterNamePath, "", "infrastructure.Status.InfrastructureName cannot be empty"))
+ default:
+ capiMachine.Spec.ClusterName = m.infrastructure.Status.InfrastructureName
+ if capiMachine.Labels == nil {
+ capiMachine.Labels = map[string]string{}
+ }
+ capiMachine.Labels[clusterv1.ClusterNameLabel] = m.infrastructure.Status.InfrastructureName
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/conversion/capi2mapi/metal3.go around lines 121 to 128, guard against
m.infrastructure being nil before accessing
m.infrastructure.Status.InfrastructureName to avoid a nil-pointer panic, and
ensure capiMachine.Labels is initialized (create the map if nil) before writing
into it; update the conversion logic to check m.infrastructure != nil then read
InfrastructureName, and right before assigning labels ensure capiMachine.Labels
= make(map[string]string) if it is nil.
|
|
||
| mapiMachineSet.Spec.Template.Spec = mapiMachine.Spec | ||
|
|
||
| // Copy the labels and annotations from the Machine to the template. | ||
| mapiMachineSet.Spec.Template.ObjectMeta.Annotations = mapiMachine.ObjectMeta.Annotations | ||
| mapiMachineSet.Spec.Template.ObjectMeta.Labels = mapiMachine.ObjectMeta.Labels | ||
|
|
There was a problem hiding this comment.
Same nil-dereference risk in MachineSet path (and need label map init).
m.infrastructure may be nil, and capiMachineSet.Labels can be nil. This mirrors the panic above; please apply the same guard/initialization here.
- if m.infrastructure == nil || m.infrastructure.Status.InfrastructureName == "" {
- errs = append(errs, field.Invalid(field.NewPath("infrastructure", "status", "infrastructureName"), m.infrastructure.Status.InfrastructureName, "infrastructure cannot be nil and infrastructure.Status.InfrastructureName cannot be empty"))
- } else {
- capiMachineSet.Spec.Template.Spec.ClusterName = m.infrastructure.Status.InfrastructureName
- capiMachineSet.Spec.ClusterName = m.infrastructure.Status.InfrastructureName
- capiMachineSet.Labels[clusterv1.ClusterNameLabel] = m.infrastructure.Status.InfrastructureName
- }
+ clusterNamePath := field.NewPath("infrastructure", "status", "infrastructureName")
+ switch {
+ case m.infrastructure == nil:
+ errs = append(errs, field.Invalid(clusterNamePath, nil, "infrastructure cannot be nil"))
+ case m.infrastructure.Status.InfrastructureName == "":
+ errs = append(errs, field.Invalid(clusterNamePath, "", "infrastructure.Status.InfrastructureName cannot be empty"))
+ default:
+ capiMachineSet.Spec.Template.Spec.ClusterName = m.infrastructure.Status.InfrastructureName
+ capiMachineSet.Spec.ClusterName = m.infrastructure.Status.InfrastructureName
+ if capiMachineSet.Labels == nil {
+ capiMachineSet.Labels = map[string]string{}
+ }
+ capiMachineSet.Labels[clusterv1.ClusterNameLabel] = m.infrastructure.Status.InfrastructureName
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/conversion/capi2mapi/metal3.go around lines 186-192, avoid
nil-dereference when copying infrastructure and labels: first guard accesses to
m.infrastructure (only use m.infrastructure fields if m.infrastructure != nil),
and ensure destination and source label maps are initialized before assignment
(if capiMachineSet.Labels is nil, create an empty map on the
template.ObjectMeta.Labels or copy after initializing template labels map). Also
defensively handle annotations similarly (initialize target map if nil) so
assignments won’t panic.
| "bytes" | ||
| "fmt" | ||
| "path" | ||
| "slices" |
There was a problem hiding this comment.
❓ Verification inconclusive
Modifying vendored dependencies creates maintenance risk.
This file is in vendor/ and should typically mirror upstream kustomize code. Direct modifications to vendored files:
- Create merge conflicts when updating dependencies
- Diverge from upstream behavior
- Are lost if the vendor directory is regenerated
Additionally, the PR objectives mention Metal3/BareMetal migration (METAL-1583), but this kustomize file appears unrelated to that work.
Please verify:
- Is this modification intentional and necessary for the baremetal migration?
- Should this change be contributed upstream to kustomize instead?
- Are the removed helper functions (
stringInSlice,indexOf) used elsewhere in the codebase?
Run this script to check for usage of the removed helpers:
🏁 Script executed:
#!/bin/bash
# Check if stringInSlice or indexOf are used elsewhere in the codebase
rg -n --type go -C 3 '\b(stringInSlice|indexOf)\s*\(' -g '!vendor/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
grep -RIn --exclude-dir=vendor --include='*.go' -E '\b(stringInSlice|indexOf)\s*\(' .Length of output: 0
Do not modify vendored kustomize code directly.
Revert all edits in vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/fix/convert.go. Vendored files must mirror upstream; if slices-based helpers are needed, contribute them upstream and then update the vendor. Removed helpers (stringInSlice, indexOf) are not used elsewhere.
🤖 Prompt for AI Agents
In vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/fix/convert.go around
line 10, the file was edited to add "slices" and removed helper functions
(stringInSlice, indexOf); do not modify vendored code — revert this file to
match upstream exactly (restore the original content and helpers if they existed
upstream), remove any local uses of the newly added slices import, and if
slices-based helpers are required, create a proper upstream PR to add them and
then update vendor via go mod vendor rather than editing vendored files
directly.
|
/test unit |
Assisted-by: claude-4.5-sonnet
|
@honza: This pull request references METAL-1583 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addmetadata.go (1)
1-215: Revert vendor file changes and use proper Go modules workflow.This is a vendored third-party file from kustomize. When go.mod changes, go.mod and vendor/modules.txt must remain in sync, and go mod vendor should be run to update the vendor directory—not the reverse.
The script output confirms: kustomize files in vendor were modified, but go.mod shows no corresponding version update to kustomize. This means these are manual edits that will be lost on the next
go mod tidy && go mod vendorrun.To fix this, choose one approach:
If these changes should be permanent: Update the kustomize dependency version in go.mod to one that includes this functionality, then run
go mod tidy && go mod vendor.If this is a temporary local patch: Add a replace directive to go.mod (e.g.,
replace sigs.k8s.io/kustomize => ./path/to/local/kustomize) rather than modifying vendor directly. When finished working, remove the replace directive from go.mod.For now, revert all changes to files under
vendor/and implement one of the above approaches instead.vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.go (1)
1-313: Now I understand the context better. Let me examine the actual code issues in detail:Now I'll examine the specific code logic to understand the incomplete removal:
Based on my analysis, the vendored dependency update IS relevant to the PR (kustomize is used for manifest generation in the codebase), but there are genuine code quality issues to address. Let me generate the rewritten review comment:
Remove dead code from incomplete starlark support removal in run-fns.go.
Starlark support for KRM functions was removed to cleanup dependencies, as this feature was deprecated 3 years ago. However, the refactoring is incomplete:
StarPath,StarURL,StarNamefields remain in theRunFnRunnerstruct (lines 86-88) but have no corresponding CLI flags to populate them- The condition at line 108 checks these fields (
r.Image == "" && r.StarPath == "" && r.StarURL == "" && r.ExecPath == "") but they can never be non-empty, making this branch partially dead code- The validation at line 251 was updated to remove star field checks, but the struct fields and condition still reference them
Remove the unused struct fields and update the condition at line 108 to reflect the removal of starlark support, leaving only:
if r.Image == "" && r.ExecPath == "".vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/localize/localize.go (1)
1-144: Vendored file contains custom modifications with buffer logic bug; changes will be overwritten on dependency updates.Two critical issues:
Custom modification to vendored code: The
noVerifyflag and verification logic (lines 45-94) do not exist in the upstream kustomize v5.6.0 (confirmed via git history search). These changes are custom patches that will be lost when the kustomize dependency is next updated.Buffer pass-by-value bug breaks verification (lines 74, 84, 133-144): The
buildBufferis declared as a value type (line 34), butbuildCmdwrites to it via pointer (&buildBuffer, line 35). However, both calls torunBuildCmd()pass the buffer by value (lines 74, 84), creating copies. InsiderunBuildCmd(),buffer.Reset()operates on the copy, not the original, so verification always reads an empty string. This causes the comparison to always succeed incorrectly (both empty outputs = identical).Remove custom modifications from vendor/ and instead propose changes to the actual kustomize upstream repository if this verification is needed.
pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go (1)
352-416: Missing BareMetal status handling in setChangedCAPIInfraMachineStatusFieldscompareCAPIInfraMachines supports BareMetal, but setChangedCAPIInfraMachineStatusFields has no BareMetal branch. Status updates for Metal3Machine will fail. Add Metal3 handling like AWS/OpenStack.
Apply:
func setChangedCAPIInfraMachineStatusFields(platform configv1.PlatformType, existingCAPIInfraMachine, convertedCAPIInfraMachine client.Object) error { switch platform { + case configv1.BareMetalPlatformType: + existing, ok := existingCAPIInfraMachine.(*metal3v1.Metal3Machine) + if !ok { + return errAssertingCAPIMetal3Machine + } + converted, ok := convertedCAPIInfraMachine.(*metal3v1.Metal3Machine) + if !ok { + return errAssertingCAPIMetal3Machine + } + util.EnsureCAPIConditions(existing, converted) + // No v1beta2-specific conditions for Metal3Machine today. + existing.Status = converted.Status + return nil case configv1.AWSPlatformType:pkg/controllers/machinesetsync/machineset_sync_controller.go (2)
367-394: Outdated template cleanup misses Metal3 listsfilterOutdatedInfraMachineTemplates doesn’t handle Metal3MachineTemplateList; BareMetal cleanup will error.
Add Metal3 case:
func filterOutdatedInfraMachineTemplates(infraMachineTemplateList client.ObjectList, newInfraMachineTemplateName string) ([]client.Object, error) { outdatedTemplates := []client.Object{} switch list := infraMachineTemplateList.(type) { case *awsv1.AWSMachineTemplateList: ... + case *metal3v1.Metal3MachineTemplateList: + for _, template := range list.Items { + if template.GetName() != newInfraMachineTemplateName { + outdatedTemplates = append(outdatedTemplates, &template) + } + } case *ibmpowervsv1.IBMPowerVSMachineTemplateList: ... case *openstackv1.OpenStackMachineTemplateList: ... default: return nil, fmt.Errorf("%w: got unknown type %T", errUnexpectedInfraMachineTemplateListType, list) }
1039-1048: Make compareMAPIMachineSets provider-aware (breaks on BareMetal today)compareMAPIMachineSets always parses AWS providerSpec; BareMetal (and others) will fail on CAPI→MAPI flow.
Apply minimal change to pass platform and parse accordingly:
- func compareMAPIMachineSets(a, b *mapiv1beta1.MachineSet) (map[string]any, error) { + func compareMAPIMachineSets(platform configv1.PlatformType, a, b *mapiv1beta1.MachineSet) (map[string]any, error) { diff := make(map[string]any) - - ps1, err := mapi2capi.AWSProviderSpecFromRawExtension(a.Spec.Template.Spec.ProviderSpec.Value) + switch platform { + case configv1.AWSPlatformType: + ps1, err := mapi2capi.AWSProviderSpecFromRawExtension(a.Spec.Template.Spec.ProviderSpec.Value) if err != nil { return nil, fmt.Errorf("unable to parse first MAPI machine set providerSpec: %w", err) } - - ps2, err := mapi2capi.AWSProviderSpecFromRawExtension(b.Spec.Template.Spec.ProviderSpec.Value) + ps2, err := mapi2capi.AWSProviderSpecFromRawExtension(b.Spec.Template.Spec.ProviderSpec.Value) if err != nil { return nil, fmt.Errorf("unable to parse second MAPI machine set providerSpec: %w", err) } - - // Sort the tags by name... - sort.Slice(ps1.Tags, func(i, j int) bool { return ps1.Tags[i].Name < ps1.Tags[j].Name }) - sort.Slice(ps2.Tags, func(i, j int) bool { return ps2.Tags[i].Name < ps2.Tags[j].Name }) - - if diffProviderSpec := deep.Equal(ps1, ps2); len(diffProviderSpec) > 0 { - diff[".providerSpec"] = diffProviderSpec - } + sort.Slice(ps1.Tags, func(i, j int) bool { return ps1.Tags[i].Name < ps1.Tags[j].Name }) + sort.Slice(ps2.Tags, func(i, j int) bool { return ps2.Tags[i].Name < ps2.Tags[j].Name }) + if diffProviderSpec := deep.Equal(ps1, ps2); len(diffProviderSpec) > 0 { + diff[".providerSpec"] = diffProviderSpec + } + case configv1.BareMetalPlatformType: + bm1, err := mapi2capi.BareMetalProviderSpecFromRawExtension(a.Spec.Template.Spec.ProviderSpec.Value) + if err != nil { + return nil, fmt.Errorf("unable to parse first MAPI machine set providerSpec: %w", err) + } + bm2, err := mapi2capi.BareMetalProviderSpecFromRawExtension(b.Spec.Template.Spec.ProviderSpec.Value) + if err != nil { + return nil, fmt.Errorf("unable to parse second MAPI machine set providerSpec: %w", err) + } + if diffProviderSpec := deep.Equal(bm1, bm2); len(diffProviderSpec) > 0 { + diff[".providerSpec"] = diffProviderSpec + } + default: + // Fallback: compare raw providerSpec payloads for other providers + if diffProviderSpec := deep.Equal(a.Spec.Template.Spec.ProviderSpec.Value, b.Spec.Template.Spec.ProviderSpec.Value); len(diffProviderSpec) > 0 { + diff[".providerSpec"] = diffProviderSpec + } + } // Remove the providerSpec from the Spec as we've already compared them. aCopy := a.DeepCopy() aCopy.Spec.Template.Spec.ProviderSpec.Value = nil bCopy := b.DeepCopy() bCopy.Spec.Template.Spec.ProviderSpec.Value = nil ... return diff, nil }And update call site:
- mapiMachineSetsDiff, err := compareMAPIMachineSets(existingMAPIMachineSet, convertedMAPIMachineSet) + mapiMachineSetsDiff, err := compareMAPIMachineSets(r.Platform, existingMAPIMachineSet, convertedMAPIMachineSet)Also applies to: 1460-1511
♻️ Duplicate comments (5)
vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.go (1)
34-52: Critical path traversal vulnerability already flagged; verify vendored version and usage.The path traversal issue using
strings.TrimPrefixhas been documented in previous reviews. Since this is vendored code, the recommended approach is to:
- Verify the version of
sigs.k8s.io/kustomize/kyamlbeing vendored- Check if a patched version exists upstream
- Verify whether
CopyDiris actually invoked in the codebaseRun this script to check usage of the vulnerable
CopyDirfunction:#!/bin/bash # Check if CopyDir from this vendored package is used anywhere in the codebase echo "=== Searching for imports of copyutil package ===" rg -n --type=go 'import.*copyutil|copyutil\.' echo "" echo "=== Searching for CopyDir function calls ===" rg -n --type=go 'CopyDir\s*\(' -g '!vendor/**' echo "" echo "=== Checking vendored kustomize version ===" fd -t f 'go.mod|go.sum' --exec grep -H 'sigs.k8s.io/kustomize/kyaml'vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/localize/localize.go (1)
34-35: Critical: Buffer passed by value breaks verification.The past review comment correctly identifies this critical bug. The
buildBufferis passed by value torunBuildCmd(lines 74, 84), but the build command writes to the original buffer (pointer passed on line 35). This means:
runBuildCmdreceives and resets a copy of the buffer (line 134)- The build command writes to the original buffer
runBuildCmdreads from the empty copy and returns empty string (line 143)- Both
originalBuildandlocalizedBuildare empty strings- Verification always succeeds (line 89) even when outputs differ
This makes the entire verification feature non-functional.
Apply the fix from the previous review (or update the vendored dependency if this is fixed upstream):
- var buildBuffer bytes.Buffer - buildCmd := build.NewCmdBuild(fs, &build.Help{}, &buildBuffer) + var buildBuffer bytes.Buffer + buildCmd := build.NewCmdBuild(fs, &build.Help{}, &buildBuffer) - originalBuild, err := runBuildCmd(buildBuffer, buildCmd, args.target) + originalBuild, err := runBuildCmd(&buildBuffer, buildCmd, args.target) - localizedBuild, err := runBuildCmd(buildBuffer, buildCmd, buildDst) + localizedBuild, err := runBuildCmd(&buildBuffer, buildCmd, buildDst) -func runBuildCmd(buffer bytes.Buffer, cmd *cobra.Command, folder string) (buildOutput string, err error) { +func runBuildCmd(buffer *bytes.Buffer, cmd *cobra.Command, folder string) (buildOutput string, err error) {Also applies to: 74-74, 84-84, 133-144
pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
1390-1411: Fix OpenStack assertion error and align metadata diffWrong constant on type assertion failure and inconsistent metadata diff.
Apply:
- if !ok { - return nil, errAssertingCAPIIBMPowerVSMachineTemplate - } + if !ok { + return nil, errAssertingCAPIOpenStackMachineTemplate + } ... - if diffObjectMeta := deep.Equal(typedInfraMachineTemplate1.ObjectMeta, typedinfraMachineTemplate2.ObjectMeta); len(diffObjectMeta) > 0 { + if diffObjectMeta := util.ObjectMetaEqual(typedInfraMachineTemplate1.ObjectMeta, typedinfraMachineTemplate2.ObjectMeta); len(diffObjectMeta) > 0 { diff[".metadata"] = diffObjectMeta }pkg/conversion/mapi2capi/metal3.go (2)
121-127: Nil-deref and missing label map initAvoid dereferencing nil infrastructure and ensure Labels map is initialized.
Apply:
- if m.infrastructure == nil || m.infrastructure.Status.InfrastructureName == "" { - errs = append(errs, field.Invalid(field.NewPath("infrastructure", "status", "infrastructureName"), m.infrastructure.Status.InfrastructureName, "infrastructure cannot be nil and infrastructure.Status.InfrastructureName cannot be empty")) - } else { - capiMachine.Spec.ClusterName = m.infrastructure.Status.InfrastructureName - capiMachine.Labels[clusterv1.ClusterNameLabel] = m.infrastructure.Status.InfrastructureName - } + clusterNamePath := field.NewPath("infrastructure", "status", "infrastructureName") + switch { + case m.infrastructure == nil: + errs = append(errs, field.Invalid(clusterNamePath, nil, "infrastructure cannot be nil")) + case m.infrastructure.Status.InfrastructureName == "": + errs = append(errs, field.Invalid(clusterNamePath, "", "infrastructure.Status.InfrastructureName cannot be empty")) + default: + capiMachine.Spec.ClusterName = m.infrastructure.Status.InfrastructureName + if capiMachine.Labels == nil { + capiMachine.Labels = map[string]string{} + } + capiMachine.Labels[clusterv1.ClusterNameLabel] = m.infrastructure.Status.InfrastructureName + }
186-192: Same infra nil-guard and label map init for MachineSet pathMirror the fix for MachineSet cluster/labels.
Apply:
- if m.infrastructure == nil || m.infrastructure.Status.InfrastructureName == "" { - errs = append(errs, field.Invalid(field.NewPath("infrastructure", "status", "infrastructureName"), m.infrastructure.Status.InfrastructureName, "infrastructure cannot be nil and infrastructure.Status.InfrastructureName cannot be empty")) - } else { - capiMachineSet.Spec.Template.Spec.ClusterName = m.infrastructure.Status.InfrastructureName - capiMachineSet.Spec.ClusterName = m.infrastructure.Status.InfrastructureName - capiMachineSet.Labels[clusterv1.ClusterNameLabel] = m.infrastructure.Status.InfrastructureName - } + clusterNamePath := field.NewPath("infrastructure", "status", "infrastructureName") + switch { + case m.infrastructure == nil: + errs = append(errs, field.Invalid(clusterNamePath, nil, "infrastructure cannot be nil")) + case m.infrastructure.Status.InfrastructureName == "": + errs = append(errs, field.Invalid(clusterNamePath, "", "infrastructure.Status.InfrastructureName cannot be empty")) + default: + capiMachineSet.Spec.Template.Spec.ClusterName = m.infrastructure.Status.InfrastructureName + capiMachineSet.Spec.ClusterName = m.infrastructure.Status.InfrastructureName + if capiMachineSet.Labels == nil { + capiMachineSet.Labels = map[string]string{} + } + capiMachineSet.Labels[clusterv1.ClusterNameLabel] = m.infrastructure.Status.InfrastructureName + }
🧹 Nitpick comments (2)
vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.go (2)
86-88: Dead code: Star-related fields are never populated.The
StarPath,StarURL, andStarNamefields are defined but never set through CLI flags (removed in the refactoring). They remain as dead code that serves no purpose.Consider removing these unused fields to complete the star functionality removal:
Image string - StarPath string - StarURL string - StarName string EnableExec bool
108-110: Dead code: Star field checks in condition are always true.Since
StarPathandStarURLare never populated (no CLI flags set them), the checksr.StarPath == ""andr.StarURL == ""are always true. This makes these condition parts meaningless.Simplify the condition to remove dead checks:
- if r.Image == "" && r.StarPath == "" && r.ExecPath == "" && r.StarURL == "" { + if r.Image == "" && r.ExecPath == "" { return nil, nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumhack/tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (46)
cmd/machine-api-migration/main.go(3 hunks)e2e/go.mod(1 hunks)go.mod(2 hunks)hack/tools/go.mod(2 hunks)pkg/controllers/common.go(3 hunks)pkg/controllers/machinesetsync/machineset_sync_controller.go(7 hunks)pkg/controllers/machinesync/machine_sync_controller.go(4 hunks)pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go(3 hunks)pkg/conversion/capi2mapi/metal3.go(1 hunks)pkg/conversion/mapi2capi/metal3.go(1 hunks)vendor/github.com/sergi/go-diff/AUTHORS(1 hunks)vendor/github.com/sergi/go-diff/CONTRIBUTORS(1 hunks)vendor/github.com/sergi/go-diff/LICENSE(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/diff.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/diffmatchpatch.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/match.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/mathutil.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/operation_string.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/patch.go(1 hunks)vendor/github.com/sergi/go-diff/diffmatchpatch/stringutil.go(1 hunks)vendor/modules.txt(7 hunks)vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/grep.go(1 hunks)vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.go(2 hunks)vendor/sigs.k8s.io/kustomize/cmd/config/internal/generateddocs/api/docs.go(1 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/build.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagenablehelm.go(1 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagsforfunctions.go(0 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/create/create.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbase.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbuildmetadata.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addcomponent.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addgenerator.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addmetadata.go(5 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addresource.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addtransformer.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/fix/convert.go(3 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removebuildmetadata.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeconfigmap.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removemetadata.go(1 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeresource.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removesecret.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removetransformer.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/kustfile/kustomizationfile.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/util/validate.go(2 hunks)vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/localize/localize.go(6 hunks)vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.go(1 hunks)
💤 Files with no reviewable changes (1)
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagsforfunctions.go
🚧 Files skipped from review as they are similar to previous changes (19)
- vendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/grep.go
- vendor/sigs.k8s.io/kustomize/cmd/config/internal/generateddocs/api/docs.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addcomponent.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/build.go
- pkg/controllers/common.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/util/validate.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeconfigmap.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removemetadata.go
- cmd/machine-api-migration/main.go
- vendor/github.com/sergi/go-diff/diffmatchpatch/stringutil.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/internal/kustfile/kustomizationfile.go
- vendor/github.com/sergi/go-diff/diffmatchpatch/diffmatchpatch.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/create/create.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addresource.go
- e2e/go.mod
- vendor/github.com/sergi/go-diff/LICENSE
- vendor/github.com/sergi/go-diff/diffmatchpatch/mathutil.go
- vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbase.go
- pkg/controllers/machinesync/machine_sync_controller.go
🔇 Additional comments (15)
vendor/github.com/sergi/go-diff/AUTHORS (1)
1-25: Standard vendor attribution file.This is a standard AUTHORS file from the upstream go-diff library containing copyright holders for legal/licensing purposes. As a vendored dependency file, it should remain unmodified from upstream.
vendor/github.com/sergi/go-diff/CONTRIBUTORS (1)
1-32: Standard vendor attribution file.This is a standard CONTRIBUTORS file from the upstream go-diff library listing individual contributors. As a vendored dependency file, it should remain unmodified from upstream.
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removetransformer.go (1)
8-8: LGTM! Vendor update adopts standard library.The migration from
kustfile.StringInSlicetoslices.Containsis correct and aligns with modern Go practices. This appears to be an upstream kustomize change pulled in via a vendor update.Also applies to: 76-76
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removesecret.go (1)
10-10: LGTM: Routine vendor dependency update.This vendor file has been updated to use the standard library
slices.Containsinstead of a custom helper function. The logic is preserved and the argument order is correctly adjusted for the new API.Also applies to: 93-93
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removeresource.go (1)
9-9: Now let me generate a script to check vendor/modules.txt and understand the workspace structure better:Let me now check vendor/modules.txt and the module declarations to verify this is a legitimate vendor update:
Based on my comprehensive verification, I can now provide the rewritten review comment:
Verify vendor directory is in sync with go.mod/go.sum.
The vendor file contains changes consistent with kustomize v5.4.0's fix for 'golang.org/x/exp/slices' dependencies (migrating to stdlib slices package in Go 1.21+). However, the initial grep found no entry for
sigs.k8s.io/kustomize/kustomize/v5in go.mod, which suggests a potential sync issue.Please confirm:
- Check
vendor/modules.txtto verify kustomize/v5 is listed with the correct version- Check
go.sumfor kustomize entries matching the vendor contents- Ensure
go work vendor(orGOWORK=off go mod vendor) has been run to regenerate vendor from current workspace dependenciesvendor/sigs.k8s.io/kustomize/cmd/config/internal/commands/run-fns.go (2)
251-253: Validation simplified correctly, but inconsistent with usage.The validation correctly requires either
--imageor--exec-path, aligning with the removal of star functionality. However, line 108 still checks forStarPathandStarURL, creating an inconsistency between validation and usage logic.Once the dead star field checks are removed from line 108 (as suggested in the previous comment), this validation will be fully consistent with the implementation.
23-23: LGTM: Documentation fix.The comment now correctly reflects the function name
GetRunFnRunnerinstead of the incorrectGetCatRunner.vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/build/flagenablehelm.go (1)
34-38: Remove the review comment—it is based on incorrect assumptions.This file was modified as part of a legitimate vendor operation (commit shows "tidy, verify, and vendor"), not a direct patch. The --helm-debug flag was officially added in upstream kustomize v5.5.0, and the commit's +5 lines to
flagenablehelm.gorepresent the dependency upgrade being applied viago mod vendor—a standard Go workflow. The connection to Metal3/CAPI is valid: the Cluster CAPI Operator uses kustomize to manage manifests. No manual verification is needed; the code change is upstream and correct.Likely an incorrect or invalid review comment.
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addbuildmetadata.go (1)
64-68: Contains refactor keeps behavior intactNice cleanup—once the Go toolchain question is settled, this is a straight behavior-preserving swap.
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/fix/convert.go (1)
227-228: Good move replacing the ad-hoc index helperSwitching to
slices.Indexdrops bespoke helpers and relies on stdlib generics—once the Go version bump lands, this will be easier to maintain.vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/remove/removebuildmetadata.go (1)
64-67: Filter logic unchangedThe switch to
slices.Containspreserves the guard and reads a bit clearer—looks good once we’re on the newer Go toolchain.vendor/github.com/sergi/go-diff/diffmatchpatch/operation_string.go (1)
1-17: Stringer output matches upstreamThe generated
Operation.String()aligns with the canonical go-diff bits and will aid logging/debugging. 👍vendor/github.com/sergi/go-diff/diffmatchpatch/match.go (1)
17-160: Bitap match port looks correctThis is the upstream Bitap matcher verbatim, so the fuzzy search behavior should now line up with the reference implementation.
vendor/github.com/sergi/go-diff/diffmatchpatch/patch.go (1)
22-556: Patch subsystem matches upstreamThanks for pulling in the full patch surface—this mirrors the upstream go-diff implementation and should behave identically.
vendor/sigs.k8s.io/kustomize/kustomize/v5/commands/edit/add/addgenerator.go (1)
9-9: No action required—Go toolchain supports stdlib slicesThe codebase compiles with Go 1.24.0, which fully supports the
slicespackage added in this change. The stdlibslicesrequirement (≥1.21) is satisfied. The import is compatible and safe to merge.
| mapaProviderConfig := baremetalv1.BareMetalMachineProviderSpec{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: "BareMetalMachineProviderSpec", | ||
| APIVersion: "baremetal.cluster.k8s.io/v1alpha1", | ||
| }, | ||
| // ObjectMeta - Only present because it's needed to form part of the runtime.RawExtension, not actually used by the provider. |
There was a problem hiding this comment.
🧩 Analysis chain
Use provider’s SchemeGroupVersion for APIVersion
Avoid hardcoding; set APIVersion from the BareMetal provider scheme (openshift).
Apply:
mapaProviderConfig := baremetalv1.BareMetalMachineProviderSpec{
TypeMeta: metav1.TypeMeta{
Kind: "BareMetalMachineProviderSpec",
- APIVersion: "baremetal.cluster.k8s.io/v1alpha1",
+ APIVersion: baremetalv1.SchemeGroupVersion.String(),
},Please confirm the group/version matches the version imported from github.com/openshift/cluster-api-provider-baremetal.
I'll verify the review comment by examining the codebase to confirm whether baremetalv1.SchemeGroupVersion exists and matches the hardcoded APIVersion.
🏁 Script executed:
#!/bin/bash
# 1. Check imports and the specific lines in metal3.go
echo "=== Checking imports and context in metal3.go ==="
head -50 pkg/conversion/capi2mapi/metal3.go | grep -E "^import|baremetalv1"
echo ""
echo "=== Lines 85-100 of metal3.go ==="
sed -n '85,100p' pkg/conversion/capi2mapi/metal3.goLength of output: 900
🏁 Script executed:
#!/bin/bash
# 2. Search for SchemeGroupVersion in the entire codebase
echo "=== Searching for SchemeGroupVersion usage ==="
rg "SchemeGroupVersion" -n -B2 -A2Length of output: 262526
🏁 Script executed:
#!/bin/bash
# 3. Check if baremetalv1 is imported and from where
echo "=== Searching for baremetalv1 package imports ==="
rg "baremetalv1" -n -C2 --type go | head -40Length of output: 2928
Use baremetalv1.SchemeGroupVersion for APIVersion
The recommendation is valid. The hardcoded value "baremetal.cluster.k8s.io/v1alpha1" matches baremetalv1.SchemeGroupVersion defined in the provider's register.go. Replace the hardcoded string with baremetalv1.SchemeGroupVersion.String() to eliminate duplication and tie the version to the provider's scheme.
mapaProviderConfig := baremetalv1.BareMetalMachineProviderSpec{
TypeMeta: metav1.TypeMeta{
Kind: "BareMetalMachineProviderSpec",
- APIVersion: "baremetal.cluster.k8s.io/v1alpha1",
+ APIVersion: baremetalv1.SchemeGroupVersion.String(),
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mapaProviderConfig := baremetalv1.BareMetalMachineProviderSpec{ | |
| TypeMeta: metav1.TypeMeta{ | |
| Kind: "BareMetalMachineProviderSpec", | |
| APIVersion: "baremetal.cluster.k8s.io/v1alpha1", | |
| }, | |
| // ObjectMeta - Only present because it's needed to form part of the runtime.RawExtension, not actually used by the provider. | |
| mapaProviderConfig := baremetalv1.BareMetalMachineProviderSpec{ | |
| TypeMeta: metav1.TypeMeta{ | |
| Kind: "BareMetalMachineProviderSpec", | |
| APIVersion: baremetalv1.SchemeGroupVersion.String(), | |
| }, | |
| // ObjectMeta - Only present because it's needed to form part of the runtime.RawExtension, not actually used by the provider. |
🤖 Prompt for AI Agents
In pkg/conversion/capi2mapi/metal3.go around lines 91 to 96, the APIVersion is
hardcoded as "baremetal.cluster.k8s.io/v1alpha1"; replace that hardcoded string
with baremetalv1.SchemeGroupVersion.String() so the APIVersion is derived from
the provider's scheme constant (ensure baremetalv1 is imported and used).
| capaMachine, ok := capaMachineObj.(*metal3v1.Metal3Machine) | ||
| if !ok { | ||
| panic(fmt.Errorf("%w: %T", errUnexpectedObjectTypeForMachine, capaMachineObj)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Don’t panic on unexpected infra type
Return an error instead of panicking in controller code.
Apply:
- if !ok {
- panic(fmt.Errorf("%w: %T", errUnexpectedObjectTypeForMachine, capaMachineObj))
- }
+ if !ok {
+ return nil, nil, warnings, fmt.Errorf("%w: %T", errUnexpectedObjectTypeForMachine, capaMachineObj)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| capaMachine, ok := capaMachineObj.(*metal3v1.Metal3Machine) | |
| if !ok { | |
| panic(fmt.Errorf("%w: %T", errUnexpectedObjectTypeForMachine, capaMachineObj)) | |
| } | |
| capaMachine, ok := capaMachineObj.(*metal3v1.Metal3Machine) | |
| if !ok { | |
| return nil, nil, warnings, fmt.Errorf("%w: %T", errUnexpectedObjectTypeForMachine, capaMachineObj) | |
| } |
🤖 Prompt for AI Agents
In pkg/conversion/mapi2capi/metal3.go around lines 160 to 164, replace the panic
when capaMachineObj is not a *metal3v1.Metal3Machine with an error return:
remove the panic(fmt.Errorf(...)) and return a wrapped error (e.g.
fmt.Errorf("%w: %T", errUnexpectedObjectTypeForMachine, capaMachineObj)) from
the function; if the function currently does not return an error, update its
signature to return (resultType, error) or (error) as appropriate and propagate
the error to callers so they can handle it instead of allowing a panic.
| // Copyright (c) 2012-2016 The go-diff authors. All rights reserved. | ||
| // https://github.com/sergi/go-diff | ||
| // See the included LICENSE file for license details. | ||
| // | ||
| // go-diff is a Go implementation of Google's Diff, Match, and Patch library | ||
| // Original library is Copyright (c) 2006 Google Inc. | ||
| // http://code.google.com/p/google-diff-match-patch/ | ||
|
|
||
| package diffmatchpatch | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "errors" | ||
| "fmt" | ||
| "html" | ||
| "math" | ||
| "net/url" | ||
| "regexp" | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
| "unicode/utf8" | ||
| ) | ||
|
|
||
| // Operation defines the operation of a diff item. | ||
| type Operation int8 | ||
|
|
||
| //go:generate stringer -type=Operation -trimprefix=Diff | ||
|
|
||
| const ( | ||
| // DiffDelete item represents a delete diff. | ||
| DiffDelete Operation = -1 | ||
| // DiffInsert item represents an insert diff. | ||
| DiffInsert Operation = 1 | ||
| // DiffEqual item represents an equal diff. | ||
| DiffEqual Operation = 0 | ||
| //IndexSeparator is used to seperate the array indexes in an index string | ||
| IndexSeparator = "," | ||
| ) | ||
|
|
||
| // Diff represents one diff operation | ||
| type Diff struct { | ||
| Type Operation | ||
| Text string | ||
| } | ||
|
|
||
| // splice removes amount elements from slice at index index, replacing them with elements. | ||
| func splice(slice []Diff, index int, amount int, elements ...Diff) []Diff { | ||
| if len(elements) == amount { | ||
| // Easy case: overwrite the relevant items. | ||
| copy(slice[index:], elements) | ||
| return slice | ||
| } | ||
| if len(elements) < amount { | ||
| // Fewer new items than old. | ||
| // Copy in the new items. | ||
| copy(slice[index:], elements) | ||
| // Shift the remaining items left. | ||
| copy(slice[index+len(elements):], slice[index+amount:]) | ||
| // Calculate the new end of the slice. | ||
| end := len(slice) - amount + len(elements) | ||
| // Zero stranded elements at end so that they can be garbage collected. | ||
| tail := slice[end:] | ||
| for i := range tail { | ||
| tail[i] = Diff{} | ||
| } | ||
| return slice[:end] | ||
| } | ||
| // More new items than old. | ||
| // Make room in slice for new elements. | ||
| // There's probably an even more efficient way to do this, | ||
| // but this is simple and clear. | ||
| need := len(slice) - amount + len(elements) | ||
| for len(slice) < need { | ||
| slice = append(slice, Diff{}) | ||
| } | ||
| // Shift slice elements right to make room for new elements. | ||
| copy(slice[index+len(elements):], slice[index+amount:]) | ||
| // Copy in new elements. | ||
| copy(slice[index:], elements) | ||
| return slice | ||
| } | ||
|
|
||
| // DiffMain finds the differences between two texts. | ||
| // If an invalid UTF-8 sequence is encountered, it will be replaced by the Unicode replacement character. | ||
| func (dmp *DiffMatchPatch) DiffMain(text1, text2 string, checklines bool) []Diff { | ||
| return dmp.DiffMainRunes([]rune(text1), []rune(text2), checklines) | ||
| } | ||
|
|
||
| // DiffMainRunes finds the differences between two rune sequences. | ||
| // If an invalid UTF-8 sequence is encountered, it will be replaced by the Unicode replacement character. | ||
| func (dmp *DiffMatchPatch) DiffMainRunes(text1, text2 []rune, checklines bool) []Diff { | ||
| var deadline time.Time | ||
| if dmp.DiffTimeout > 0 { | ||
| deadline = time.Now().Add(dmp.DiffTimeout) | ||
| } | ||
| return dmp.diffMainRunes(text1, text2, checklines, deadline) | ||
| } | ||
|
|
||
| func (dmp *DiffMatchPatch) diffMainRunes(text1, text2 []rune, checklines bool, deadline time.Time) []Diff { | ||
| if runesEqual(text1, text2) { | ||
| var diffs []Diff | ||
| if len(text1) > 0 { | ||
| diffs = append(diffs, Diff{DiffEqual, string(text1)}) | ||
| } | ||
| return diffs | ||
| } | ||
| // Trim off common prefix (speedup). | ||
| commonlength := commonPrefixLength(text1, text2) | ||
| commonprefix := text1[:commonlength] | ||
| text1 = text1[commonlength:] | ||
| text2 = text2[commonlength:] | ||
|
|
||
| // Trim off common suffix (speedup). | ||
| commonlength = commonSuffixLength(text1, text2) | ||
| commonsuffix := text1[len(text1)-commonlength:] | ||
| text1 = text1[:len(text1)-commonlength] | ||
| text2 = text2[:len(text2)-commonlength] | ||
|
|
||
| // Compute the diff on the middle block. | ||
| diffs := dmp.diffCompute(text1, text2, checklines, deadline) | ||
|
|
||
| // Restore the prefix and suffix. | ||
| if len(commonprefix) != 0 { | ||
| diffs = append([]Diff{{DiffEqual, string(commonprefix)}}, diffs...) | ||
| } | ||
| if len(commonsuffix) != 0 { | ||
| diffs = append(diffs, Diff{DiffEqual, string(commonsuffix)}) | ||
| } | ||
|
|
||
| return dmp.DiffCleanupMerge(diffs) | ||
| } | ||
|
|
||
| // diffCompute finds the differences between two rune slices. Assumes that the texts do not have any common prefix or suffix. | ||
| func (dmp *DiffMatchPatch) diffCompute(text1, text2 []rune, checklines bool, deadline time.Time) []Diff { | ||
| diffs := []Diff{} | ||
| if len(text1) == 0 { | ||
| // Just add some text (speedup). | ||
| return append(diffs, Diff{DiffInsert, string(text2)}) | ||
| } else if len(text2) == 0 { | ||
| // Just delete some text (speedup). | ||
| return append(diffs, Diff{DiffDelete, string(text1)}) | ||
| } | ||
|
|
||
| var longtext, shorttext []rune | ||
| if len(text1) > len(text2) { | ||
| longtext = text1 | ||
| shorttext = text2 | ||
| } else { | ||
| longtext = text2 | ||
| shorttext = text1 | ||
| } | ||
|
|
||
| if i := runesIndex(longtext, shorttext); i != -1 { | ||
| op := DiffInsert | ||
| // Swap insertions for deletions if diff is reversed. | ||
| if len(text1) > len(text2) { | ||
| op = DiffDelete | ||
| } | ||
| // Shorter text is inside the longer text (speedup). | ||
| return []Diff{ | ||
| Diff{op, string(longtext[:i])}, | ||
| Diff{DiffEqual, string(shorttext)}, | ||
| Diff{op, string(longtext[i+len(shorttext):])}, | ||
| } | ||
| } else if len(shorttext) == 1 { | ||
| // Single character string. | ||
| // After the previous speedup, the character can't be an equality. | ||
| return []Diff{ | ||
| {DiffDelete, string(text1)}, | ||
| {DiffInsert, string(text2)}, | ||
| } | ||
| // Check to see if the problem can be split in two. | ||
| } else if hm := dmp.diffHalfMatch(text1, text2); hm != nil { | ||
| // A half-match was found, sort out the return data. | ||
| text1A := hm[0] | ||
| text1B := hm[1] | ||
| text2A := hm[2] | ||
| text2B := hm[3] | ||
| midCommon := hm[4] | ||
| // Send both pairs off for separate processing. | ||
| diffsA := dmp.diffMainRunes(text1A, text2A, checklines, deadline) | ||
| diffsB := dmp.diffMainRunes(text1B, text2B, checklines, deadline) | ||
| // Merge the results. | ||
| diffs := diffsA | ||
| diffs = append(diffs, Diff{DiffEqual, string(midCommon)}) | ||
| diffs = append(diffs, diffsB...) | ||
| return diffs | ||
| } else if checklines && len(text1) > 100 && len(text2) > 100 { | ||
| return dmp.diffLineMode(text1, text2, deadline) | ||
| } | ||
| return dmp.diffBisect(text1, text2, deadline) | ||
| } | ||
|
|
||
| // diffLineMode does a quick line-level diff on both []runes, then rediff the parts for greater accuracy. This speedup can produce non-minimal diffs. | ||
| func (dmp *DiffMatchPatch) diffLineMode(text1, text2 []rune, deadline time.Time) []Diff { | ||
| // Scan the text on a line-by-line basis first. | ||
| text1, text2, linearray := dmp.DiffLinesToRunes(string(text1), string(text2)) | ||
|
|
||
| diffs := dmp.diffMainRunes(text1, text2, false, deadline) | ||
|
|
||
| // Convert the diff back to original text. | ||
| diffs = dmp.DiffCharsToLines(diffs, linearray) | ||
| // Eliminate freak matches (e.g. blank lines) | ||
| diffs = dmp.DiffCleanupSemantic(diffs) | ||
|
|
||
| // Rediff any replacement blocks, this time character-by-character. | ||
| // Add a dummy entry at the end. | ||
| diffs = append(diffs, Diff{DiffEqual, ""}) | ||
|
|
||
| pointer := 0 | ||
| countDelete := 0 | ||
| countInsert := 0 | ||
|
|
||
| // NOTE: Rune slices are slower than using strings in this case. | ||
| textDelete := "" | ||
| textInsert := "" | ||
|
|
||
| for pointer < len(diffs) { | ||
| switch diffs[pointer].Type { | ||
| case DiffInsert: | ||
| countInsert++ | ||
| textInsert += diffs[pointer].Text | ||
| case DiffDelete: | ||
| countDelete++ | ||
| textDelete += diffs[pointer].Text | ||
| case DiffEqual: | ||
| // Upon reaching an equality, check for prior redundancies. | ||
| if countDelete >= 1 && countInsert >= 1 { | ||
| // Delete the offending records and add the merged ones. | ||
| diffs = splice(diffs, pointer-countDelete-countInsert, | ||
| countDelete+countInsert) | ||
|
|
||
| pointer = pointer - countDelete - countInsert | ||
| a := dmp.diffMainRunes([]rune(textDelete), []rune(textInsert), false, deadline) | ||
| for j := len(a) - 1; j >= 0; j-- { | ||
| diffs = splice(diffs, pointer, 0, a[j]) | ||
| } | ||
| pointer = pointer + len(a) | ||
| } | ||
|
|
||
| countInsert = 0 | ||
| countDelete = 0 | ||
| textDelete = "" | ||
| textInsert = "" | ||
| } | ||
| pointer++ | ||
| } | ||
|
|
||
| return diffs[:len(diffs)-1] // Remove the dummy entry at the end. | ||
| } | ||
|
|
||
| // DiffBisect finds the 'middle snake' of a diff, split the problem in two and return the recursively constructed diff. | ||
| // If an invalid UTF-8 sequence is encountered, it will be replaced by the Unicode replacement character. | ||
| // See Myers 1986 paper: An O(ND) Difference Algorithm and Its Variations. | ||
| func (dmp *DiffMatchPatch) DiffBisect(text1, text2 string, deadline time.Time) []Diff { | ||
| // Unused in this code, but retained for interface compatibility. | ||
| return dmp.diffBisect([]rune(text1), []rune(text2), deadline) | ||
| } | ||
|
|
||
| // diffBisect finds the 'middle snake' of a diff, splits the problem in two and returns the recursively constructed diff. | ||
| // See Myers's 1986 paper: An O(ND) Difference Algorithm and Its Variations. | ||
| func (dmp *DiffMatchPatch) diffBisect(runes1, runes2 []rune, deadline time.Time) []Diff { | ||
| // Cache the text lengths to prevent multiple calls. | ||
| runes1Len, runes2Len := len(runes1), len(runes2) | ||
|
|
||
| maxD := (runes1Len + runes2Len + 1) / 2 | ||
| vOffset := maxD | ||
| vLength := 2 * maxD | ||
|
|
||
| v1 := make([]int, vLength) | ||
| v2 := make([]int, vLength) | ||
| for i := range v1 { | ||
| v1[i] = -1 | ||
| v2[i] = -1 | ||
| } | ||
| v1[vOffset+1] = 0 | ||
| v2[vOffset+1] = 0 | ||
|
|
||
| delta := runes1Len - runes2Len | ||
| // If the total number of characters is odd, then the front path will collide with the reverse path. | ||
| front := (delta%2 != 0) | ||
| // Offsets for start and end of k loop. Prevents mapping of space beyond the grid. | ||
| k1start := 0 | ||
| k1end := 0 | ||
| k2start := 0 | ||
| k2end := 0 | ||
| for d := 0; d < maxD; d++ { | ||
| // Bail out if deadline is reached. | ||
| if !deadline.IsZero() && d%16 == 0 && time.Now().After(deadline) { | ||
| break | ||
| } | ||
|
|
||
| // Walk the front path one step. | ||
| for k1 := -d + k1start; k1 <= d-k1end; k1 += 2 { | ||
| k1Offset := vOffset + k1 | ||
| var x1 int | ||
|
|
||
| if k1 == -d || (k1 != d && v1[k1Offset-1] < v1[k1Offset+1]) { | ||
| x1 = v1[k1Offset+1] | ||
| } else { | ||
| x1 = v1[k1Offset-1] + 1 | ||
| } | ||
|
|
||
| y1 := x1 - k1 | ||
| for x1 < runes1Len && y1 < runes2Len { | ||
| if runes1[x1] != runes2[y1] { | ||
| break | ||
| } | ||
| x1++ | ||
| y1++ | ||
| } | ||
| v1[k1Offset] = x1 | ||
| if x1 > runes1Len { | ||
| // Ran off the right of the graph. | ||
| k1end += 2 | ||
| } else if y1 > runes2Len { | ||
| // Ran off the bottom of the graph. | ||
| k1start += 2 | ||
| } else if front { | ||
| k2Offset := vOffset + delta - k1 | ||
| if k2Offset >= 0 && k2Offset < vLength && v2[k2Offset] != -1 { | ||
| // Mirror x2 onto top-left coordinate system. | ||
| x2 := runes1Len - v2[k2Offset] | ||
| if x1 >= x2 { | ||
| // Overlap detected. | ||
| return dmp.diffBisectSplit(runes1, runes2, x1, y1, deadline) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // Walk the reverse path one step. | ||
| for k2 := -d + k2start; k2 <= d-k2end; k2 += 2 { | ||
| k2Offset := vOffset + k2 | ||
| var x2 int | ||
| if k2 == -d || (k2 != d && v2[k2Offset-1] < v2[k2Offset+1]) { | ||
| x2 = v2[k2Offset+1] | ||
| } else { | ||
| x2 = v2[k2Offset-1] + 1 | ||
| } | ||
| var y2 = x2 - k2 | ||
| for x2 < runes1Len && y2 < runes2Len { | ||
| if runes1[runes1Len-x2-1] != runes2[runes2Len-y2-1] { | ||
| break | ||
| } | ||
| x2++ | ||
| y2++ | ||
| } | ||
| v2[k2Offset] = x2 | ||
| if x2 > runes1Len { | ||
| // Ran off the left of the graph. | ||
| k2end += 2 | ||
| } else if y2 > runes2Len { | ||
| // Ran off the top of the graph. | ||
| k2start += 2 | ||
| } else if !front { | ||
| k1Offset := vOffset + delta - k2 | ||
| if k1Offset >= 0 && k1Offset < vLength && v1[k1Offset] != -1 { | ||
| x1 := v1[k1Offset] | ||
| y1 := vOffset + x1 - k1Offset | ||
| // Mirror x2 onto top-left coordinate system. | ||
| x2 = runes1Len - x2 | ||
| if x1 >= x2 { | ||
| // Overlap detected. | ||
| return dmp.diffBisectSplit(runes1, runes2, x1, y1, deadline) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // Diff took too long and hit the deadline or number of diffs equals number of characters, no commonality at all. | ||
| return []Diff{ | ||
| {DiffDelete, string(runes1)}, | ||
| {DiffInsert, string(runes2)}, | ||
| } | ||
| } | ||
|
|
||
| func (dmp *DiffMatchPatch) diffBisectSplit(runes1, runes2 []rune, x, y int, | ||
| deadline time.Time) []Diff { | ||
| runes1a := runes1[:x] | ||
| runes2a := runes2[:y] | ||
| runes1b := runes1[x:] | ||
| runes2b := runes2[y:] | ||
|
|
||
| // Compute both diffs serially. | ||
| diffs := dmp.diffMainRunes(runes1a, runes2a, false, deadline) | ||
| diffsb := dmp.diffMainRunes(runes1b, runes2b, false, deadline) | ||
|
|
||
| return append(diffs, diffsb...) | ||
| } | ||
|
|
||
| // DiffLinesToChars splits two texts into a list of strings, and educes the texts to a string of hashes where each Unicode character represents one line. | ||
| // It's slightly faster to call DiffLinesToRunes first, followed by DiffMainRunes. | ||
| func (dmp *DiffMatchPatch) DiffLinesToChars(text1, text2 string) (string, string, []string) { | ||
| chars1, chars2, lineArray := dmp.diffLinesToStrings(text1, text2) | ||
| return chars1, chars2, lineArray | ||
| } | ||
|
|
||
| // DiffLinesToRunes splits two texts into a list of runes. | ||
| func (dmp *DiffMatchPatch) DiffLinesToRunes(text1, text2 string) ([]rune, []rune, []string) { | ||
| chars1, chars2, lineArray := dmp.diffLinesToStrings(text1, text2) | ||
| return []rune(chars1), []rune(chars2), lineArray | ||
| } | ||
|
|
||
| // DiffCharsToLines rehydrates the text in a diff from a string of line hashes to real lines of text. | ||
| func (dmp *DiffMatchPatch) DiffCharsToLines(diffs []Diff, lineArray []string) []Diff { | ||
| hydrated := make([]Diff, 0, len(diffs)) | ||
| for _, aDiff := range diffs { | ||
| chars := strings.Split(aDiff.Text, IndexSeparator) | ||
| text := make([]string, len(chars)) | ||
|
|
||
| for i, r := range chars { | ||
| i1, err := strconv.Atoi(r) | ||
| if err == nil { | ||
| text[i] = lineArray[i1] | ||
| } | ||
| } | ||
|
|
||
| aDiff.Text = strings.Join(text, "") | ||
| hydrated = append(hydrated, aDiff) | ||
| } | ||
| return hydrated | ||
| } | ||
|
|
||
| // DiffCommonPrefix determines the common prefix length of two strings. | ||
| func (dmp *DiffMatchPatch) DiffCommonPrefix(text1, text2 string) int { | ||
| // Unused in this code, but retained for interface compatibility. | ||
| return commonPrefixLength([]rune(text1), []rune(text2)) | ||
| } | ||
|
|
||
| // DiffCommonSuffix determines the common suffix length of two strings. | ||
| func (dmp *DiffMatchPatch) DiffCommonSuffix(text1, text2 string) int { | ||
| // Unused in this code, but retained for interface compatibility. | ||
| return commonSuffixLength([]rune(text1), []rune(text2)) | ||
| } | ||
|
|
||
| // commonPrefixLength returns the length of the common prefix of two rune slices. | ||
| func commonPrefixLength(text1, text2 []rune) int { | ||
| // Linear search. See comment in commonSuffixLength. | ||
| n := 0 | ||
| for ; n < len(text1) && n < len(text2); n++ { | ||
| if text1[n] != text2[n] { | ||
| return n | ||
| } | ||
| } | ||
| return n | ||
| } | ||
|
|
||
| // commonSuffixLength returns the length of the common suffix of two rune slices. | ||
| func commonSuffixLength(text1, text2 []rune) int { | ||
| // Use linear search rather than the binary search discussed at https://neil.fraser.name/news/2007/10/09/. | ||
| // See discussion at https://github.com/sergi/go-diff/issues/54. | ||
| i1 := len(text1) | ||
| i2 := len(text2) | ||
| for n := 0; ; n++ { | ||
| i1-- | ||
| i2-- | ||
| if i1 < 0 || i2 < 0 || text1[i1] != text2[i2] { | ||
| return n | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // DiffCommonOverlap determines if the suffix of one string is the prefix of another. | ||
| func (dmp *DiffMatchPatch) DiffCommonOverlap(text1 string, text2 string) int { | ||
| // Cache the text lengths to prevent multiple calls. | ||
| text1Length := len(text1) | ||
| text2Length := len(text2) | ||
| // Eliminate the null case. | ||
| if text1Length == 0 || text2Length == 0 { | ||
| return 0 | ||
| } | ||
| // Truncate the longer string. | ||
| if text1Length > text2Length { | ||
| text1 = text1[text1Length-text2Length:] | ||
| } else if text1Length < text2Length { | ||
| text2 = text2[0:text1Length] | ||
| } | ||
| textLength := int(math.Min(float64(text1Length), float64(text2Length))) | ||
| // Quick check for the worst case. | ||
| if text1 == text2 { | ||
| return textLength | ||
| } | ||
|
|
||
| // Start by looking for a single character match and increase length until no match is found. Performance analysis: http://neil.fraser.name/news/2010/11/04/ | ||
| best := 0 | ||
| length := 1 | ||
| for { | ||
| pattern := text1[textLength-length:] | ||
| found := strings.Index(text2, pattern) | ||
| if found == -1 { | ||
| break | ||
| } | ||
| length += found | ||
| if found == 0 || text1[textLength-length:] == text2[0:length] { | ||
| best = length | ||
| length++ | ||
| } | ||
| } | ||
|
|
||
| return best | ||
| } | ||
|
|
||
| // DiffHalfMatch checks whether the two texts share a substring which is at least half the length of the longer text. This speedup can produce non-minimal diffs. | ||
| func (dmp *DiffMatchPatch) DiffHalfMatch(text1, text2 string) []string { | ||
| // Unused in this code, but retained for interface compatibility. | ||
| runeSlices := dmp.diffHalfMatch([]rune(text1), []rune(text2)) | ||
| if runeSlices == nil { | ||
| return nil | ||
| } | ||
|
|
||
| result := make([]string, len(runeSlices)) | ||
| for i, r := range runeSlices { | ||
| result[i] = string(r) | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| func (dmp *DiffMatchPatch) diffHalfMatch(text1, text2 []rune) [][]rune { | ||
| if dmp.DiffTimeout <= 0 { | ||
| // Don't risk returning a non-optimal diff if we have unlimited time. | ||
| return nil | ||
| } | ||
|
|
||
| var longtext, shorttext []rune | ||
| if len(text1) > len(text2) { | ||
| longtext = text1 | ||
| shorttext = text2 | ||
| } else { | ||
| longtext = text2 | ||
| shorttext = text1 | ||
| } | ||
|
|
||
| if len(longtext) < 4 || len(shorttext)*2 < len(longtext) { | ||
| return nil // Pointless. | ||
| } | ||
|
|
||
| // First check if the second quarter is the seed for a half-match. | ||
| hm1 := dmp.diffHalfMatchI(longtext, shorttext, int(float64(len(longtext)+3)/4)) | ||
|
|
||
| // Check again based on the third quarter. | ||
| hm2 := dmp.diffHalfMatchI(longtext, shorttext, int(float64(len(longtext)+1)/2)) | ||
|
|
||
| hm := [][]rune{} | ||
| if hm1 == nil && hm2 == nil { | ||
| return nil | ||
| } else if hm2 == nil { | ||
| hm = hm1 | ||
| } else if hm1 == nil { | ||
| hm = hm2 | ||
| } else { | ||
| // Both matched. Select the longest. | ||
| if len(hm1[4]) > len(hm2[4]) { | ||
| hm = hm1 | ||
| } else { | ||
| hm = hm2 | ||
| } | ||
| } | ||
|
|
||
| // A half-match was found, sort out the return data. | ||
| if len(text1) > len(text2) { | ||
| return hm | ||
| } | ||
|
|
||
| return [][]rune{hm[2], hm[3], hm[0], hm[1], hm[4]} | ||
| } | ||
|
|
||
| // diffHalfMatchI checks if a substring of shorttext exist within longtext such that the substring is at least half the length of longtext? | ||
| // Returns a slice containing the prefix of longtext, the suffix of longtext, the prefix of shorttext, the suffix of shorttext and the common middle, or null if there was no match. | ||
| func (dmp *DiffMatchPatch) diffHalfMatchI(l, s []rune, i int) [][]rune { | ||
| var bestCommonA []rune | ||
| var bestCommonB []rune | ||
| var bestCommonLen int | ||
| var bestLongtextA []rune | ||
| var bestLongtextB []rune | ||
| var bestShorttextA []rune | ||
| var bestShorttextB []rune | ||
|
|
||
| // Start with a 1/4 length substring at position i as a seed. | ||
| seed := l[i : i+len(l)/4] | ||
|
|
||
| for j := runesIndexOf(s, seed, 0); j != -1; j = runesIndexOf(s, seed, j+1) { | ||
| prefixLength := commonPrefixLength(l[i:], s[j:]) | ||
| suffixLength := commonSuffixLength(l[:i], s[:j]) | ||
|
|
||
| if bestCommonLen < suffixLength+prefixLength { | ||
| bestCommonA = s[j-suffixLength : j] | ||
| bestCommonB = s[j : j+prefixLength] | ||
| bestCommonLen = len(bestCommonA) + len(bestCommonB) | ||
| bestLongtextA = l[:i-suffixLength] | ||
| bestLongtextB = l[i+prefixLength:] | ||
| bestShorttextA = s[:j-suffixLength] | ||
| bestShorttextB = s[j+prefixLength:] | ||
| } | ||
| } | ||
|
|
||
| if bestCommonLen*2 < len(l) { | ||
| return nil | ||
| } | ||
|
|
||
| return [][]rune{ | ||
| bestLongtextA, | ||
| bestLongtextB, | ||
| bestShorttextA, | ||
| bestShorttextB, | ||
| append(bestCommonA, bestCommonB...), | ||
| } | ||
| } | ||
|
|
||
| // DiffCleanupSemantic reduces the number of edits by eliminating semantically trivial equalities. | ||
| func (dmp *DiffMatchPatch) DiffCleanupSemantic(diffs []Diff) []Diff { | ||
| changes := false | ||
| // Stack of indices where equalities are found. | ||
| equalities := make([]int, 0, len(diffs)) | ||
|
|
||
| var lastequality string | ||
| // Always equal to diffs[equalities[equalitiesLength - 1]][1] | ||
| var pointer int // Index of current position. | ||
| // Number of characters that changed prior to the equality. | ||
| var lengthInsertions1, lengthDeletions1 int | ||
| // Number of characters that changed after the equality. | ||
| var lengthInsertions2, lengthDeletions2 int | ||
|
|
||
| for pointer < len(diffs) { | ||
| if diffs[pointer].Type == DiffEqual { | ||
| // Equality found. | ||
| equalities = append(equalities, pointer) | ||
| lengthInsertions1 = lengthInsertions2 | ||
| lengthDeletions1 = lengthDeletions2 | ||
| lengthInsertions2 = 0 | ||
| lengthDeletions2 = 0 | ||
| lastequality = diffs[pointer].Text | ||
| } else { | ||
| // An insertion or deletion. | ||
|
|
||
| if diffs[pointer].Type == DiffInsert { | ||
| lengthInsertions2 += utf8.RuneCountInString(diffs[pointer].Text) | ||
| } else { | ||
| lengthDeletions2 += utf8.RuneCountInString(diffs[pointer].Text) | ||
| } | ||
| // Eliminate an equality that is smaller or equal to the edits on both sides of it. | ||
| difference1 := int(math.Max(float64(lengthInsertions1), float64(lengthDeletions1))) | ||
| difference2 := int(math.Max(float64(lengthInsertions2), float64(lengthDeletions2))) | ||
| if utf8.RuneCountInString(lastequality) > 0 && | ||
| (utf8.RuneCountInString(lastequality) <= difference1) && | ||
| (utf8.RuneCountInString(lastequality) <= difference2) { | ||
| // Duplicate record. | ||
| insPoint := equalities[len(equalities)-1] | ||
| diffs = splice(diffs, insPoint, 0, Diff{DiffDelete, lastequality}) | ||
|
|
||
| // Change second copy to insert. | ||
| diffs[insPoint+1].Type = DiffInsert | ||
| // Throw away the equality we just deleted. | ||
| equalities = equalities[:len(equalities)-1] | ||
|
|
||
| if len(equalities) > 0 { | ||
| equalities = equalities[:len(equalities)-1] | ||
| } | ||
| pointer = -1 | ||
| if len(equalities) > 0 { | ||
| pointer = equalities[len(equalities)-1] | ||
| } | ||
|
|
||
| lengthInsertions1 = 0 // Reset the counters. | ||
| lengthDeletions1 = 0 | ||
| lengthInsertions2 = 0 | ||
| lengthDeletions2 = 0 | ||
| lastequality = "" | ||
| changes = true | ||
| } | ||
| } | ||
| pointer++ | ||
| } | ||
|
|
||
| // Normalize the diff. | ||
| if changes { | ||
| diffs = dmp.DiffCleanupMerge(diffs) | ||
| } | ||
| diffs = dmp.DiffCleanupSemanticLossless(diffs) | ||
| // Find any overlaps between deletions and insertions. | ||
| // e.g: <del>abcxxx</del><ins>xxxdef</ins> | ||
| // -> <del>abc</del>xxx<ins>def</ins> | ||
| // e.g: <del>xxxabc</del><ins>defxxx</ins> | ||
| // -> <ins>def</ins>xxx<del>abc</del> | ||
| // Only extract an overlap if it is as big as the edit ahead or behind it. | ||
| pointer = 1 | ||
| for pointer < len(diffs) { | ||
| if diffs[pointer-1].Type == DiffDelete && | ||
| diffs[pointer].Type == DiffInsert { | ||
| deletion := diffs[pointer-1].Text | ||
| insertion := diffs[pointer].Text | ||
| overlapLength1 := dmp.DiffCommonOverlap(deletion, insertion) | ||
| overlapLength2 := dmp.DiffCommonOverlap(insertion, deletion) | ||
| if overlapLength1 >= overlapLength2 { | ||
| if float64(overlapLength1) >= float64(utf8.RuneCountInString(deletion))/2 || | ||
| float64(overlapLength1) >= float64(utf8.RuneCountInString(insertion))/2 { | ||
|
|
||
| // Overlap found. Insert an equality and trim the surrounding edits. | ||
| diffs = splice(diffs, pointer, 0, Diff{DiffEqual, insertion[:overlapLength1]}) | ||
| diffs[pointer-1].Text = | ||
| deletion[0 : len(deletion)-overlapLength1] | ||
| diffs[pointer+1].Text = insertion[overlapLength1:] | ||
| pointer++ | ||
| } | ||
| } else { | ||
| if float64(overlapLength2) >= float64(utf8.RuneCountInString(deletion))/2 || | ||
| float64(overlapLength2) >= float64(utf8.RuneCountInString(insertion))/2 { | ||
| // Reverse overlap found. Insert an equality and swap and trim the surrounding edits. | ||
| overlap := Diff{DiffEqual, deletion[:overlapLength2]} | ||
| diffs = splice(diffs, pointer, 0, overlap) | ||
| diffs[pointer-1].Type = DiffInsert | ||
| diffs[pointer-1].Text = insertion[0 : len(insertion)-overlapLength2] | ||
| diffs[pointer+1].Type = DiffDelete | ||
| diffs[pointer+1].Text = deletion[overlapLength2:] | ||
| pointer++ | ||
| } | ||
| } | ||
| pointer++ | ||
| } | ||
| pointer++ | ||
| } | ||
|
|
||
| return diffs | ||
| } | ||
|
|
||
| // Define some regex patterns for matching boundaries. | ||
| var ( | ||
| nonAlphaNumericRegex = regexp.MustCompile(`[^a-zA-Z0-9]`) | ||
| whitespaceRegex = regexp.MustCompile(`\s`) | ||
| linebreakRegex = regexp.MustCompile(`[\r\n]`) | ||
| blanklineEndRegex = regexp.MustCompile(`\n\r?\n$`) | ||
| blanklineStartRegex = regexp.MustCompile(`^\r?\n\r?\n`) | ||
| ) | ||
|
|
||
| // diffCleanupSemanticScore computes a score representing whether the internal boundary falls on logical boundaries. | ||
| // Scores range from 6 (best) to 0 (worst). Closure, but does not reference any external variables. | ||
| func diffCleanupSemanticScore(one, two string) int { | ||
| if len(one) == 0 || len(two) == 0 { | ||
| // Edges are the best. | ||
| return 6 | ||
| } | ||
|
|
||
| // Each port of this function behaves slightly differently due to subtle differences in each language's definition of things like 'whitespace'. Since this function's purpose is largely cosmetic, the choice has been made to use each language's native features rather than force total conformity. | ||
| rune1, _ := utf8.DecodeLastRuneInString(one) | ||
| rune2, _ := utf8.DecodeRuneInString(two) | ||
| char1 := string(rune1) | ||
| char2 := string(rune2) | ||
|
|
||
| nonAlphaNumeric1 := nonAlphaNumericRegex.MatchString(char1) | ||
| nonAlphaNumeric2 := nonAlphaNumericRegex.MatchString(char2) | ||
| whitespace1 := nonAlphaNumeric1 && whitespaceRegex.MatchString(char1) | ||
| whitespace2 := nonAlphaNumeric2 && whitespaceRegex.MatchString(char2) | ||
| lineBreak1 := whitespace1 && linebreakRegex.MatchString(char1) | ||
| lineBreak2 := whitespace2 && linebreakRegex.MatchString(char2) | ||
| blankLine1 := lineBreak1 && blanklineEndRegex.MatchString(one) | ||
| blankLine2 := lineBreak2 && blanklineEndRegex.MatchString(two) | ||
|
|
||
| if blankLine1 || blankLine2 { | ||
| // Five points for blank lines. | ||
| return 5 | ||
| } else if lineBreak1 || lineBreak2 { | ||
| // Four points for line breaks. | ||
| return 4 | ||
| } else if nonAlphaNumeric1 && !whitespace1 && whitespace2 { | ||
| // Three points for end of sentences. | ||
| return 3 | ||
| } else if whitespace1 || whitespace2 { | ||
| // Two points for whitespace. | ||
| return 2 | ||
| } else if nonAlphaNumeric1 || nonAlphaNumeric2 { | ||
| // One point for non-alphanumeric. | ||
| return 1 | ||
| } | ||
| return 0 | ||
| } | ||
|
|
||
| // DiffCleanupSemanticLossless looks for single edits surrounded on both sides by equalities which can be shifted sideways to align the edit to a word boundary. | ||
| // E.g: The c<ins>at c</ins>ame. -> The <ins>cat </ins>came. | ||
| func (dmp *DiffMatchPatch) DiffCleanupSemanticLossless(diffs []Diff) []Diff { | ||
| pointer := 1 | ||
|
|
||
| // Intentionally ignore the first and last element (don't need checking). | ||
| for pointer < len(diffs)-1 { | ||
| if diffs[pointer-1].Type == DiffEqual && | ||
| diffs[pointer+1].Type == DiffEqual { | ||
|
|
||
| // This is a single edit surrounded by equalities. | ||
| equality1 := diffs[pointer-1].Text | ||
| edit := diffs[pointer].Text | ||
| equality2 := diffs[pointer+1].Text | ||
|
|
||
| // First, shift the edit as far left as possible. | ||
| commonOffset := dmp.DiffCommonSuffix(equality1, edit) | ||
| if commonOffset > 0 { | ||
| commonString := edit[len(edit)-commonOffset:] | ||
| equality1 = equality1[0 : len(equality1)-commonOffset] | ||
| edit = commonString + edit[:len(edit)-commonOffset] | ||
| equality2 = commonString + equality2 | ||
| } | ||
|
|
||
| // Second, step character by character right, looking for the best fit. | ||
| bestEquality1 := equality1 | ||
| bestEdit := edit | ||
| bestEquality2 := equality2 | ||
| bestScore := diffCleanupSemanticScore(equality1, edit) + | ||
| diffCleanupSemanticScore(edit, equality2) | ||
|
|
||
| for len(edit) != 0 && len(equality2) != 0 { | ||
| _, sz := utf8.DecodeRuneInString(edit) | ||
| if len(equality2) < sz || edit[:sz] != equality2[:sz] { | ||
| break | ||
| } | ||
| equality1 += edit[:sz] | ||
| edit = edit[sz:] + equality2[:sz] | ||
| equality2 = equality2[sz:] | ||
| score := diffCleanupSemanticScore(equality1, edit) + | ||
| diffCleanupSemanticScore(edit, equality2) | ||
| // The >= encourages trailing rather than leading whitespace on edits. | ||
| if score >= bestScore { | ||
| bestScore = score | ||
| bestEquality1 = equality1 | ||
| bestEdit = edit | ||
| bestEquality2 = equality2 | ||
| } | ||
| } | ||
|
|
||
| if diffs[pointer-1].Text != bestEquality1 { | ||
| // We have an improvement, save it back to the diff. | ||
| if len(bestEquality1) != 0 { | ||
| diffs[pointer-1].Text = bestEquality1 | ||
| } else { | ||
| diffs = splice(diffs, pointer-1, 1) | ||
| pointer-- | ||
| } | ||
|
|
||
| diffs[pointer].Text = bestEdit | ||
| if len(bestEquality2) != 0 { | ||
| diffs[pointer+1].Text = bestEquality2 | ||
| } else { | ||
| diffs = append(diffs[:pointer+1], diffs[pointer+2:]...) | ||
| pointer-- | ||
| } | ||
| } | ||
| } | ||
| pointer++ | ||
| } | ||
|
|
||
| return diffs | ||
| } | ||
|
|
||
| // DiffCleanupEfficiency reduces the number of edits by eliminating operationally trivial equalities. | ||
| func (dmp *DiffMatchPatch) DiffCleanupEfficiency(diffs []Diff) []Diff { | ||
| changes := false | ||
| // Stack of indices where equalities are found. | ||
| type equality struct { | ||
| data int | ||
| next *equality | ||
| } | ||
| var equalities *equality | ||
| // Always equal to equalities[equalitiesLength-1][1] | ||
| lastequality := "" | ||
| pointer := 0 // Index of current position. | ||
| // Is there an insertion operation before the last equality. | ||
| preIns := false | ||
| // Is there a deletion operation before the last equality. | ||
| preDel := false | ||
| // Is there an insertion operation after the last equality. | ||
| postIns := false | ||
| // Is there a deletion operation after the last equality. | ||
| postDel := false | ||
| for pointer < len(diffs) { | ||
| if diffs[pointer].Type == DiffEqual { // Equality found. | ||
| if len(diffs[pointer].Text) < dmp.DiffEditCost && | ||
| (postIns || postDel) { | ||
| // Candidate found. | ||
| equalities = &equality{ | ||
| data: pointer, | ||
| next: equalities, | ||
| } | ||
| preIns = postIns | ||
| preDel = postDel | ||
| lastequality = diffs[pointer].Text | ||
| } else { | ||
| // Not a candidate, and can never become one. | ||
| equalities = nil | ||
| lastequality = "" | ||
| } | ||
| postIns = false | ||
| postDel = false | ||
| } else { // An insertion or deletion. | ||
| if diffs[pointer].Type == DiffDelete { | ||
| postDel = true | ||
| } else { | ||
| postIns = true | ||
| } | ||
|
|
||
| // Five types to be split: | ||
| // <ins>A</ins><del>B</del>XY<ins>C</ins><del>D</del> | ||
| // <ins>A</ins>X<ins>C</ins><del>D</del> | ||
| // <ins>A</ins><del>B</del>X<ins>C</ins> | ||
| // <ins>A</del>X<ins>C</ins><del>D</del> | ||
| // <ins>A</ins><del>B</del>X<del>C</del> | ||
| var sumPres int | ||
| if preIns { | ||
| sumPres++ | ||
| } | ||
| if preDel { | ||
| sumPres++ | ||
| } | ||
| if postIns { | ||
| sumPres++ | ||
| } | ||
| if postDel { | ||
| sumPres++ | ||
| } | ||
| if len(lastequality) > 0 && | ||
| ((preIns && preDel && postIns && postDel) || | ||
| ((len(lastequality) < dmp.DiffEditCost/2) && sumPres == 3)) { | ||
|
|
||
| insPoint := equalities.data | ||
|
|
||
| // Duplicate record. | ||
| diffs = splice(diffs, insPoint, 0, Diff{DiffDelete, lastequality}) | ||
|
|
||
| // Change second copy to insert. | ||
| diffs[insPoint+1].Type = DiffInsert | ||
| // Throw away the equality we just deleted. | ||
| equalities = equalities.next | ||
| lastequality = "" | ||
|
|
||
| if preIns && preDel { | ||
| // No changes made which could affect previous entry, keep going. | ||
| postIns = true | ||
| postDel = true | ||
| equalities = nil | ||
| } else { | ||
| if equalities != nil { | ||
| equalities = equalities.next | ||
| } | ||
| if equalities != nil { | ||
| pointer = equalities.data | ||
| } else { | ||
| pointer = -1 | ||
| } | ||
| postIns = false | ||
| postDel = false | ||
| } | ||
| changes = true | ||
| } | ||
| } | ||
| pointer++ | ||
| } | ||
|
|
||
| if changes { | ||
| diffs = dmp.DiffCleanupMerge(diffs) | ||
| } | ||
|
|
||
| return diffs | ||
| } | ||
|
|
||
| // DiffCleanupMerge reorders and merges like edit sections. Merge equalities. | ||
| // Any edit section can move as long as it doesn't cross an equality. | ||
| func (dmp *DiffMatchPatch) DiffCleanupMerge(diffs []Diff) []Diff { | ||
| // Add a dummy entry at the end. | ||
| diffs = append(diffs, Diff{DiffEqual, ""}) | ||
| pointer := 0 | ||
| countDelete := 0 | ||
| countInsert := 0 | ||
| commonlength := 0 | ||
| textDelete := []rune(nil) | ||
| textInsert := []rune(nil) | ||
|
|
||
| for pointer < len(diffs) { | ||
| switch diffs[pointer].Type { | ||
| case DiffInsert: | ||
| countInsert++ | ||
| textInsert = append(textInsert, []rune(diffs[pointer].Text)...) | ||
| pointer++ | ||
| break | ||
| case DiffDelete: | ||
| countDelete++ | ||
| textDelete = append(textDelete, []rune(diffs[pointer].Text)...) | ||
| pointer++ | ||
| break | ||
| case DiffEqual: | ||
| // Upon reaching an equality, check for prior redundancies. | ||
| if countDelete+countInsert > 1 { | ||
| if countDelete != 0 && countInsert != 0 { | ||
| // Factor out any common prefixies. | ||
| commonlength = commonPrefixLength(textInsert, textDelete) | ||
| if commonlength != 0 { | ||
| x := pointer - countDelete - countInsert | ||
| if x > 0 && diffs[x-1].Type == DiffEqual { | ||
| diffs[x-1].Text += string(textInsert[:commonlength]) | ||
| } else { | ||
| diffs = append([]Diff{{DiffEqual, string(textInsert[:commonlength])}}, diffs...) | ||
| pointer++ | ||
| } | ||
| textInsert = textInsert[commonlength:] | ||
| textDelete = textDelete[commonlength:] | ||
| } | ||
| // Factor out any common suffixies. | ||
| commonlength = commonSuffixLength(textInsert, textDelete) | ||
| if commonlength != 0 { | ||
| insertIndex := len(textInsert) - commonlength | ||
| deleteIndex := len(textDelete) - commonlength | ||
| diffs[pointer].Text = string(textInsert[insertIndex:]) + diffs[pointer].Text | ||
| textInsert = textInsert[:insertIndex] | ||
| textDelete = textDelete[:deleteIndex] | ||
| } | ||
| } | ||
| // Delete the offending records and add the merged ones. | ||
| if countDelete == 0 { | ||
| diffs = splice(diffs, pointer-countInsert, | ||
| countDelete+countInsert, | ||
| Diff{DiffInsert, string(textInsert)}) | ||
| } else if countInsert == 0 { | ||
| diffs = splice(diffs, pointer-countDelete, | ||
| countDelete+countInsert, | ||
| Diff{DiffDelete, string(textDelete)}) | ||
| } else { | ||
| diffs = splice(diffs, pointer-countDelete-countInsert, | ||
| countDelete+countInsert, | ||
| Diff{DiffDelete, string(textDelete)}, | ||
| Diff{DiffInsert, string(textInsert)}) | ||
| } | ||
|
|
||
| pointer = pointer - countDelete - countInsert + 1 | ||
| if countDelete != 0 { | ||
| pointer++ | ||
| } | ||
| if countInsert != 0 { | ||
| pointer++ | ||
| } | ||
| } else if pointer != 0 && diffs[pointer-1].Type == DiffEqual { | ||
| // Merge this equality with the previous one. | ||
| diffs[pointer-1].Text += diffs[pointer].Text | ||
| diffs = append(diffs[:pointer], diffs[pointer+1:]...) | ||
| } else { | ||
| pointer++ | ||
| } | ||
| countInsert = 0 | ||
| countDelete = 0 | ||
| textDelete = nil | ||
| textInsert = nil | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if len(diffs[len(diffs)-1].Text) == 0 { | ||
| diffs = diffs[0 : len(diffs)-1] // Remove the dummy entry at the end. | ||
| } | ||
|
|
||
| // Second pass: look for single edits surrounded on both sides by equalities which can be shifted sideways to eliminate an equality. E.g: A<ins>BA</ins>C -> <ins>AB</ins>AC | ||
| changes := false | ||
| pointer = 1 | ||
| // Intentionally ignore the first and last element (don't need checking). | ||
| for pointer < (len(diffs) - 1) { | ||
| if diffs[pointer-1].Type == DiffEqual && | ||
| diffs[pointer+1].Type == DiffEqual { | ||
| // This is a single edit surrounded by equalities. | ||
| if strings.HasSuffix(diffs[pointer].Text, diffs[pointer-1].Text) { | ||
| // Shift the edit over the previous equality. | ||
| diffs[pointer].Text = diffs[pointer-1].Text + | ||
| diffs[pointer].Text[:len(diffs[pointer].Text)-len(diffs[pointer-1].Text)] | ||
| diffs[pointer+1].Text = diffs[pointer-1].Text + diffs[pointer+1].Text | ||
| diffs = splice(diffs, pointer-1, 1) | ||
| changes = true | ||
| } else if strings.HasPrefix(diffs[pointer].Text, diffs[pointer+1].Text) { | ||
| // Shift the edit over the next equality. | ||
| diffs[pointer-1].Text += diffs[pointer+1].Text | ||
| diffs[pointer].Text = | ||
| diffs[pointer].Text[len(diffs[pointer+1].Text):] + diffs[pointer+1].Text | ||
| diffs = splice(diffs, pointer+1, 1) | ||
| changes = true | ||
| } | ||
| } | ||
| pointer++ | ||
| } | ||
|
|
||
| // If shifts were made, the diff needs reordering and another shift sweep. | ||
| if changes { | ||
| diffs = dmp.DiffCleanupMerge(diffs) | ||
| } | ||
|
|
||
| return diffs | ||
| } | ||
|
|
||
| // DiffXIndex returns the equivalent location in s2. | ||
| func (dmp *DiffMatchPatch) DiffXIndex(diffs []Diff, loc int) int { | ||
| chars1 := 0 | ||
| chars2 := 0 | ||
| lastChars1 := 0 | ||
| lastChars2 := 0 | ||
| lastDiff := Diff{} | ||
| for i := 0; i < len(diffs); i++ { | ||
| aDiff := diffs[i] | ||
| if aDiff.Type != DiffInsert { | ||
| // Equality or deletion. | ||
| chars1 += len(aDiff.Text) | ||
| } | ||
| if aDiff.Type != DiffDelete { | ||
| // Equality or insertion. | ||
| chars2 += len(aDiff.Text) | ||
| } | ||
| if chars1 > loc { | ||
| // Overshot the location. | ||
| lastDiff = aDiff | ||
| break | ||
| } | ||
| lastChars1 = chars1 | ||
| lastChars2 = chars2 | ||
| } | ||
| if lastDiff.Type == DiffDelete { | ||
| // The location was deleted. | ||
| return lastChars2 | ||
| } | ||
| // Add the remaining character length. | ||
| return lastChars2 + (loc - lastChars1) | ||
| } | ||
|
|
||
| // DiffPrettyHtml converts a []Diff into a pretty HTML report. | ||
| // It is intended as an example from which to write one's own display functions. | ||
| func (dmp *DiffMatchPatch) DiffPrettyHtml(diffs []Diff) string { | ||
| var buff bytes.Buffer | ||
| for _, diff := range diffs { | ||
| text := strings.Replace(html.EscapeString(diff.Text), "\n", "¶<br>", -1) | ||
| switch diff.Type { | ||
| case DiffInsert: | ||
| _, _ = buff.WriteString("<ins style=\"background:#e6ffe6;\">") | ||
| _, _ = buff.WriteString(text) | ||
| _, _ = buff.WriteString("</ins>") | ||
| case DiffDelete: | ||
| _, _ = buff.WriteString("<del style=\"background:#ffe6e6;\">") | ||
| _, _ = buff.WriteString(text) | ||
| _, _ = buff.WriteString("</del>") | ||
| case DiffEqual: | ||
| _, _ = buff.WriteString("<span>") | ||
| _, _ = buff.WriteString(text) | ||
| _, _ = buff.WriteString("</span>") | ||
| } | ||
| } | ||
| return buff.String() | ||
| } | ||
|
|
||
| // DiffPrettyText converts a []Diff into a colored text report. | ||
| func (dmp *DiffMatchPatch) DiffPrettyText(diffs []Diff) string { | ||
| var buff bytes.Buffer | ||
| for _, diff := range diffs { | ||
| text := diff.Text | ||
|
|
||
| switch diff.Type { | ||
| case DiffInsert: | ||
| _, _ = buff.WriteString("\x1b[32m") | ||
| _, _ = buff.WriteString(text) | ||
| _, _ = buff.WriteString("\x1b[0m") | ||
| case DiffDelete: | ||
| _, _ = buff.WriteString("\x1b[31m") | ||
| _, _ = buff.WriteString(text) | ||
| _, _ = buff.WriteString("\x1b[0m") | ||
| case DiffEqual: | ||
| _, _ = buff.WriteString(text) | ||
| } | ||
| } | ||
|
|
||
| return buff.String() | ||
| } | ||
|
|
||
| // DiffText1 computes and returns the source text (all equalities and deletions). | ||
| func (dmp *DiffMatchPatch) DiffText1(diffs []Diff) string { | ||
| //StringBuilder text = new StringBuilder() | ||
| var text bytes.Buffer | ||
|
|
||
| for _, aDiff := range diffs { | ||
| if aDiff.Type != DiffInsert { | ||
| _, _ = text.WriteString(aDiff.Text) | ||
| } | ||
| } | ||
| return text.String() | ||
| } | ||
|
|
||
| // DiffText2 computes and returns the destination text (all equalities and insertions). | ||
| func (dmp *DiffMatchPatch) DiffText2(diffs []Diff) string { | ||
| var text bytes.Buffer | ||
|
|
||
| for _, aDiff := range diffs { | ||
| if aDiff.Type != DiffDelete { | ||
| _, _ = text.WriteString(aDiff.Text) | ||
| } | ||
| } | ||
| return text.String() | ||
| } | ||
|
|
||
| // DiffLevenshtein computes the Levenshtein distance that is the number of inserted, deleted or substituted characters. | ||
| func (dmp *DiffMatchPatch) DiffLevenshtein(diffs []Diff) int { | ||
| levenshtein := 0 | ||
| insertions := 0 | ||
| deletions := 0 | ||
|
|
||
| for _, aDiff := range diffs { | ||
| switch aDiff.Type { | ||
| case DiffInsert: | ||
| insertions += utf8.RuneCountInString(aDiff.Text) | ||
| case DiffDelete: | ||
| deletions += utf8.RuneCountInString(aDiff.Text) | ||
| case DiffEqual: | ||
| // A deletion and an insertion is one substitution. | ||
| levenshtein += max(insertions, deletions) | ||
| insertions = 0 | ||
| deletions = 0 | ||
| } | ||
| } | ||
|
|
||
| levenshtein += max(insertions, deletions) | ||
| return levenshtein | ||
| } | ||
|
|
||
| // DiffToDelta crushes the diff into an encoded string which describes the operations required to transform text1 into text2. | ||
| // E.g. =3\t-2\t+ing -> Keep 3 chars, delete 2 chars, insert 'ing'. Operations are tab-separated. Inserted text is escaped using %xx notation. | ||
| func (dmp *DiffMatchPatch) DiffToDelta(diffs []Diff) string { | ||
| var text bytes.Buffer | ||
| for _, aDiff := range diffs { | ||
| switch aDiff.Type { | ||
| case DiffInsert: | ||
| _, _ = text.WriteString("+") | ||
| _, _ = text.WriteString(strings.Replace(url.QueryEscape(aDiff.Text), "+", " ", -1)) | ||
| _, _ = text.WriteString("\t") | ||
| break | ||
| case DiffDelete: | ||
| _, _ = text.WriteString("-") | ||
| _, _ = text.WriteString(strconv.Itoa(utf8.RuneCountInString(aDiff.Text))) | ||
| _, _ = text.WriteString("\t") | ||
| break | ||
| case DiffEqual: | ||
| _, _ = text.WriteString("=") | ||
| _, _ = text.WriteString(strconv.Itoa(utf8.RuneCountInString(aDiff.Text))) | ||
| _, _ = text.WriteString("\t") | ||
| break | ||
| } | ||
| } | ||
| delta := text.String() | ||
| if len(delta) != 0 { | ||
| // Strip off trailing tab character. | ||
| delta = delta[0 : utf8.RuneCountInString(delta)-1] | ||
| delta = unescaper.Replace(delta) | ||
| } | ||
| return delta | ||
| } | ||
|
|
||
| // DiffFromDelta given the original text1, and an encoded string which describes the operations required to transform text1 into text2, comAdde the full diff. | ||
| func (dmp *DiffMatchPatch) DiffFromDelta(text1 string, delta string) (diffs []Diff, err error) { | ||
| i := 0 | ||
| runes := []rune(text1) | ||
|
|
||
| for _, token := range strings.Split(delta, "\t") { | ||
| if len(token) == 0 { | ||
| // Blank tokens are ok (from a trailing \t). | ||
| continue | ||
| } | ||
|
|
||
| // Each token begins with a one character parameter which specifies the operation of this token (delete, insert, equality). | ||
| param := token[1:] | ||
|
|
||
| switch op := token[0]; op { | ||
| case '+': | ||
| // Decode would Diff all "+" to " " | ||
| param = strings.Replace(param, "+", "%2b", -1) | ||
| param, err = url.QueryUnescape(param) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !utf8.ValidString(param) { | ||
| return nil, fmt.Errorf("invalid UTF-8 token: %q", param) | ||
| } | ||
|
|
||
| diffs = append(diffs, Diff{DiffInsert, param}) | ||
| case '=', '-': | ||
| n, err := strconv.ParseInt(param, 10, 0) | ||
| if err != nil { | ||
| return nil, err | ||
| } else if n < 0 { | ||
| return nil, errors.New("Negative number in DiffFromDelta: " + param) | ||
| } | ||
|
|
||
| i += int(n) | ||
| // Break out if we are out of bounds, go1.6 can't handle this very well | ||
| if i > len(runes) { | ||
| break | ||
| } | ||
| // Remember that string slicing is by byte - we want by rune here. | ||
| text := string(runes[i-int(n) : i]) | ||
|
|
||
| if op == '=' { | ||
| diffs = append(diffs, Diff{DiffEqual, text}) | ||
| } else { | ||
| diffs = append(diffs, Diff{DiffDelete, text}) | ||
| } | ||
| default: | ||
| // Anything else is an error. | ||
| return nil, errors.New("Invalid diff operation in DiffFromDelta: " + string(token[0])) | ||
| } | ||
| } | ||
|
|
||
| if i != len(runes) { | ||
| return nil, fmt.Errorf("Delta length (%v) is different from source text length (%v)", i, len(text1)) | ||
| } | ||
|
|
||
| return diffs, nil | ||
| } | ||
|
|
||
| // diffLinesToStrings splits two texts into a list of strings. Each string represents one line. | ||
| func (dmp *DiffMatchPatch) diffLinesToStrings(text1, text2 string) (string, string, []string) { | ||
| // '\x00' is a valid character, but various debuggers don't like it. So we'll insert a junk entry to avoid generating a null character. | ||
| lineArray := []string{""} // e.g. lineArray[4] == 'Hello\n' | ||
|
|
||
| lineHash := make(map[string]int) | ||
| //Each string has the index of lineArray which it points to | ||
| strIndexArray1 := dmp.diffLinesToStringsMunge(text1, &lineArray, lineHash) | ||
| strIndexArray2 := dmp.diffLinesToStringsMunge(text2, &lineArray, lineHash) | ||
|
|
||
| return intArrayToString(strIndexArray1), intArrayToString(strIndexArray2), lineArray | ||
| } | ||
|
|
||
| // diffLinesToStringsMunge splits a text into an array of strings, and reduces the texts to a []string. | ||
| func (dmp *DiffMatchPatch) diffLinesToStringsMunge(text string, lineArray *[]string, lineHash map[string]int) []uint32 { | ||
| // Walk the text, pulling out a substring for each line. text.split('\n') would would temporarily double our memory footprint. Modifying text would create many large strings to garbage collect. | ||
| lineStart := 0 | ||
| lineEnd := -1 | ||
| strs := []uint32{} | ||
|
|
||
| for lineEnd < len(text)-1 { | ||
| lineEnd = indexOf(text, "\n", lineStart) | ||
|
|
||
| if lineEnd == -1 { | ||
| lineEnd = len(text) - 1 | ||
| } | ||
|
|
||
| line := text[lineStart : lineEnd+1] | ||
| lineStart = lineEnd + 1 | ||
| lineValue, ok := lineHash[line] | ||
|
|
||
| if ok { | ||
| strs = append(strs, uint32(lineValue)) | ||
| } else { | ||
| *lineArray = append(*lineArray, line) | ||
| lineHash[line] = len(*lineArray) - 1 | ||
| strs = append(strs, uint32(len(*lineArray)-1)) | ||
| } | ||
| } | ||
|
|
||
| return strs | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify upstream version and known issues for vendored dependency.
This entire file is vendored third-party code from github.com/sergi/go-diff. A previous review identified a potential bug at line 756 where blanklineEndRegex is used instead of blanklineStartRegex for the boundary check on the right segment.
Since this is vendored code, modifications should come from upstream rather than being patched locally. Please verify:
- Whether this is the latest version of the library
- Whether the boundary regex issue has been fixed upstream
- If not, consider reporting the issue to the upstream maintainers
Run the following script to check the upstream status:
🏁 Script executed:
#!/bin/bash
# Check the latest version and tags of the upstream library
echo "=== Latest releases/tags from upstream ==="
gh api repos/sergi/go-diff/releases --jq '.[0:3] | .[] | {tag_name, published_at, name}'
echo -e "\n=== Check for open/closed issues related to blankline regex ==="
gh api graphql -f query='
{
repository(owner: "sergi", name: "go-diff") {
issues(first: 10, states: [OPEN, CLOSED], filterBy: {labels: ["bug"]}) {
nodes {
number
title
state
createdAt
url
}
}
}
}'
echo -e "\n=== Recent commits mentioning blankline or semantic ==="
gh api repos/sergi/go-diff/commits --jq '.[0:10] | .[] | select(.commit.message | test("blankline|semantic"; "i")) | {sha: .sha[0:7], date: .commit.author.date, message: .commit.message}'Length of output: 1870
Fix semantic cleanup regex bug at line 756.
The current code incorrectly uses blanklineEndRegex for both blankLine1 and blankLine2 checks. Since blankLine2 evaluates the beginning of the two parameter (checking the start), it should use blanklineStartRegex instead.
Line 756 should be:
blankLine2 := lineBreak2 && blanklineStartRegex.MatchString(two)This bug exists in the current upstream repository and has not been reported or fixed. Since you've vendored this code, applying the fix locally is appropriate.
🤖 Prompt for AI Agents
In vendor/github.com/sergi/go-diff/diffmatchpatch/diff.go around lines ~740-760,
the semantic cleanup scoring uses blanklineEndRegex for both sides causing
blankLine2 to be computed against the wrong regex; change the computation for
blankLine2 to use blanklineStartRegex when evaluating the start of the second
string so blankLine2 := lineBreak2 && blanklineStartRegex.MatchString(two).
Ensure you only replace that single blankLine2 assignment and run tests to
confirm behavior.
| // Copyright 2019 The Kubernetes Authors. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| // The copyutil package contains libraries for copying directories of configuration. | ||
| package copyutil | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
||
| "github.com/sergi/go-diff/diffmatchpatch" | ||
| "sigs.k8s.io/kustomize/kyaml/errors" | ||
| "sigs.k8s.io/kustomize/kyaml/filesys" | ||
| "sigs.k8s.io/kustomize/kyaml/sets" | ||
| ) | ||
|
|
||
| // CopyDir copies a src directory to a dst directory. CopyDir skips copying the .git directory from the src. | ||
| func CopyDir(fSys filesys.FileSystem, src string, dst string) error { | ||
| return errors.Wrap(fSys.Walk(src, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // don't copy the .git dir | ||
| if path != src { | ||
| rel := strings.TrimPrefix(path, src) | ||
| if IsDotGitFolder(rel) { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // path is an absolute path, rather than a path relative to src. | ||
| // e.g. if src is /path/to/package, then path might be /path/to/package/and/sub/dir | ||
| // we need the path relative to src `and/sub/dir` when we are copying the files to dest. | ||
| copyTo := strings.TrimPrefix(path, src) | ||
|
|
||
| // make directories that don't exist | ||
| if info.IsDir() { | ||
| return errors.Wrap(fSys.MkdirAll(filepath.Join(dst, copyTo))) | ||
| } | ||
|
|
||
| // copy file by reading and writing it | ||
| b, err := fSys.ReadFile(filepath.Join(src, copyTo)) | ||
| if err != nil { | ||
| return errors.Wrap(err) | ||
| } | ||
| err = fSys.WriteFile(filepath.Join(dst, copyTo), b) | ||
| if err != nil { | ||
| return errors.Wrap(err) | ||
| } | ||
|
|
||
| return nil | ||
| })) | ||
| } | ||
|
|
||
| // Diff returns a list of files that differ between the source and destination. | ||
| // | ||
| // Diff is guaranteed to return a non-empty set if any files differ, but | ||
| // this set is not guaranteed to contain all differing files. | ||
| func Diff(sourceDir, destDir string) (sets.String, error) { | ||
| // get set of filenames in the package source | ||
| upstreamFiles := sets.String{} | ||
| err := filepath.Walk(sourceDir, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // skip git repo if it exists | ||
| if IsDotGitFolder(path) { | ||
| return nil | ||
| } | ||
|
|
||
| upstreamFiles.Insert(strings.TrimPrefix(strings.TrimPrefix(path, sourceDir), string(filepath.Separator))) | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return sets.String{}, err | ||
| } | ||
|
|
||
| // get set of filenames in the cloned package | ||
| localFiles := sets.String{} | ||
| err = filepath.Walk(destDir, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // skip git repo if it exists | ||
| if IsDotGitFolder(path) { | ||
| return nil | ||
| } | ||
|
|
||
| localFiles.Insert(strings.TrimPrefix(strings.TrimPrefix(path, destDir), string(filepath.Separator))) | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return sets.String{}, err | ||
| } | ||
|
|
||
| // verify the source and cloned packages have the same set of filenames | ||
| diff := upstreamFiles.SymmetricDifference(localFiles) | ||
|
|
||
| // verify file contents match | ||
| for _, f := range upstreamFiles.Intersection(localFiles).List() { | ||
| fi, err := os.Stat(filepath.Join(destDir, f)) | ||
| if err != nil { | ||
| return diff, err | ||
| } | ||
| if fi.Mode().IsDir() { | ||
| // already checked that this directory exists in the local files | ||
| continue | ||
| } | ||
|
|
||
| // compare upstreamFiles | ||
| b1, err := os.ReadFile(filepath.Join(destDir, f)) | ||
| if err != nil { | ||
| return diff, err | ||
| } | ||
| b2, err := os.ReadFile(filepath.Join(sourceDir, f)) | ||
| if err != nil { | ||
| return diff, err | ||
| } | ||
| if !bytes.Equal(b1, b2) { | ||
| fmt.Println(PrettyFileDiff(string(b1), string(b2))) | ||
| diff.Insert(f) | ||
| } | ||
| } | ||
| // return the differing files | ||
| return diff, nil | ||
| } | ||
|
|
||
| // IsDotGitFolder checks if the provided path is either the .git folder or | ||
| // a file underneath the .git folder. | ||
| func IsDotGitFolder(path string) bool { | ||
| cleanPath := filepath.ToSlash(filepath.Clean(path)) | ||
| for _, c := range strings.Split(cleanPath, "/") { | ||
| if c == ".git" { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // PrettyFileDiff takes the content of two files and returns the pretty diff | ||
| func PrettyFileDiff(s1, s2 string) string { | ||
| dmp := diffmatchpatch.New() | ||
| wSrc, wDst, warray := dmp.DiffLinesToRunes(s1, s2) | ||
| diffs := dmp.DiffMainRunes(wSrc, wDst, false) | ||
| diffs = dmp.DiffCharsToLines(diffs, warray) | ||
| return dmp.DiffPrettyText(diffs) | ||
| } | ||
|
|
||
| // SyncFile copies file from src file path to a dst file path by replacement | ||
| // deletes dst file if src file doesn't exist | ||
| func SyncFile(src, dst string) error { | ||
| srcFileInfo, err := os.Stat(src) | ||
| if err != nil { | ||
| // delete dst if source doesn't exist | ||
| if err = deleteFile(dst); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| input, err := os.ReadFile(src) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var filePerm os.FileMode | ||
|
|
||
| // get the destination file perm if file exists | ||
| dstFileInfo, err := os.Stat(dst) | ||
| if err != nil { | ||
| // get source file perm if destination file doesn't exist | ||
| filePerm = srcFileInfo.Mode().Perm() | ||
| } else { | ||
| filePerm = dstFileInfo.Mode().Perm() | ||
| } | ||
|
|
||
| err = os.WriteFile(dst, input, filePerm) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // deleteFile deletes file from path, returns no error if file doesn't exist | ||
| func deleteFile(path string) error { | ||
| if _, err := os.Stat(path); err != nil { | ||
| // return nil if file doesn't exist | ||
| return nil | ||
| } | ||
| return os.Remove(path) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify necessity and version of vendored copyutil package.
This entire file is newly vendored third-party code with identified security issues. Before merging:
- Confirm this package is required for the Metal3 migration functionality
- Verify you're vendoring the latest version of
sigs.k8s.io/kustomize/kyaml - Check if upstream has addressed the path traversal vulnerability
- Consider whether these utilities could be replaced with standard library functions or safer alternatives
🌐 Web query:
What is the latest version of sigs.k8s.io/kustomize/kyaml and does it fix the CopyDir path traversal vulnerability?
💡 Result:
Latest kyaml release: v0.20.1 (tag kyaml/v0.20.1, 23 Jul 2025). [1]
About the CopyDir path‑traversal issue: the path‑traversal vulnerabilities (GHSA-j77r-2fxf-5jrw / GHSA-7pwf-jg34-hxwp / CVE-2022-24877 / CVE-2022-24878) were fixed by introducing a new Kustomize filesystem that confines file access; the fix was shipped in kustomize-controller v0.24.0 (included in flux2 v0.29.0). Users are advised to upgrade to patched versions (and to a current kyaml). [2][3]
References
- kustomize releases (kyaml/v0.20.1). [1]
- CVE / advisory describing the path traversal and the fix (kustomize-controller v0.24.0 / flux2 v0.29.0). [2][3]
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the kyaml version in go.mod and vendor metadata
echo "=== Checking go.mod for kyaml dependency ==="
if [ -f go.mod ]; then
grep -i "sigs.k8s.io/kustomize/kyaml" go.mod || echo "kyaml not found in go.mod"
else
echo "go.mod not found at repository root"
fi
echo ""
echo "=== Checking vendor/modules.txt ==="
if [ -f vendor/modules.txt ]; then
grep "sigs.k8s.io/kustomize/kyaml" vendor/modules.txt | head -5
else
echo "vendor/modules.txt not found"
fi
echo ""
echo "=== Checking for version info in vendor directory ==="
fd -t f "go.mod" vendor/sigs.k8s.io/kustomize/kyaml 2>/dev/null | head -3 | xargs -I {} sh -c 'echo "File: {}"; head -20 "{}"'
Length of output: 341
---
🌐 Web query:
```web
kustomize kyaml v0.19.0 security vulnerabilities path traversal CVE
💡 Result:
Short answer: Yes — kyaml v0.19.0 falls into the affected range for known Kustomize path‑traversal issues (CVE‑2022‑24877 and CVE‑2022‑24878). These were fixed in the kustomize controller / kustomize libraries by v0.29.0; upgrade to ≥ v0.29.0. [1][2][3]
Sources:
- GitLab advisory / affected versions (starts at 0.19.0, fixed in 0.29.0). [1]
- NVD CVE entries for CVE‑2022‑24877 / CVE‑2022‑24878 describing path‑traversal impacts and fixes. [2]
- OSV / vulnerability record listing v0.19.0 in affected versions and fixed in v0.29.0. [3]
Would you like exact upgrade commands or checks for your project dependency graph?
Upgrade vendored kyaml from v0.19.0 to v0.29.0 to fix path traversal vulnerabilities.
The vendored kyaml v0.19.0 is affected by CVE-2022-24877 and CVE-2022-24878 path traversal vulnerabilities, which are fixed in v0.29.0. The CopyDir function in this file is part of the vulnerable code. Update the kyaml dependency to the patched version before merging.
🤖 Prompt for AI Agents
vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.go lines 1-197: this
vendored kyaml copyutil contains the vulnerable CopyDir implementation from
kyaml v0.19.0; upgrade the kyaml dependency to v0.29.0 and re-vendor to pull in
the patched copyutil. Update go.mod to require sigs.k8s.io/kustomize/kyaml
v0.29.0 (or the minimal module that contains the fix), run `go get
sigs.k8s.io/kustomize/[email protected]` (or update replace directives if using
vendor), then run `go mod vendor` (or regenerate the vendor directory) so
vendor/sigs.k8s.io/kustomize/kyaml/... is replaced with the fixed code; run `go
test ./...` and a quick scan to ensure the new copyutil.go is present and no
other call sites need changes before committing.
| srcFileInfo, err := os.Stat(src) | ||
| if err != nil { | ||
| // delete dst if source doesn't exist | ||
| if err = deleteFile(dst); err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Incorrect error handling may delete destination on transient source errors.
The function deletes dst whenever os.Stat(src) fails, regardless of the error type. This could cause unintended data loss if src is temporarily inaccessible due to permissions, network issues, or other transient errors.
The code should check specifically for "not found" errors:
srcFileInfo, err := os.Stat(src)
if err != nil {
- // delete dst if source doesn't exist
- if err = deleteFile(dst); err != nil {
- return err
+ // delete dst only if source doesn't exist
+ if os.IsNotExist(err) {
+ if err = deleteFile(dst); err != nil {
+ return err
+ }
+ return nil
}
- return nil
+ return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| srcFileInfo, err := os.Stat(src) | |
| if err != nil { | |
| // delete dst if source doesn't exist | |
| if err = deleteFile(dst); err != nil { | |
| return err | |
| } | |
| return nil | |
| srcFileInfo, err := os.Stat(src) | |
| if err != nil { | |
| // delete dst only if source doesn't exist | |
| if os.IsNotExist(err) { | |
| if err = deleteFile(dst); err != nil { | |
| return err | |
| } | |
| return nil | |
| } | |
| return err | |
| } |
🤖 Prompt for AI Agents
In vendor/sigs.k8s.io/kustomize/kyaml/copyutil/copyutil.go around lines 157 to
163, the code unconditionally deletes dst whenever os.Stat(src) returns an
error; change this to only delete dst when the error indicates the source is
missing (use os.IsNotExist(err) or errors.Is(err, fs.ErrNotExist)), and for any
other stat errors return the error without modifying dst so transient
permission/network errors won't cause data loss.
|
@honza: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| fldPath := field.NewPath("spec") | ||
|
|
||
| mapaProviderConfig := baremetalv1.BareMetalMachineProviderSpec{ |
There was a problem hiding this comment.
This variable name should probably be updated
| warnings []string | ||
| ) | ||
|
|
||
| mapaSpec, warn, err := m.toProviderSpec() |
| return nil, nil, nil, field.ErrorList{field.Invalid(field.NewPath("spec", "providerSpec", "value"), m.machine.Spec.ProviderSpec.Value, err.Error())} | ||
| } | ||
|
|
||
| capaMachine, warn, machineErrs := m.toMetal3Machine(baremetalProviderConfig) |
|
|
||
| warnings = append(warnings, warn...) | ||
|
|
||
| capaMachine, ok := capaMachineObj.(*metal3v1.Metal3Machine) |
3 participants
Assisted-by: claude-4.5-sonnet
Summary by CodeRabbit