Nutanix: Machine API Provider to Cluster API Provider #395
Conversation
WalkthroughAdds Nutanix support: new MAPI→CAPI Nutanix conversion logic and tests, integrates Nutanix routing into machine/machineset sync controllers, vendors Nutanix CAPX and Prism Go client code (types, credentials, deepcopy, licenses), and updates module/vendor metadata. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(250,250,252)
participant Client as Caller
participant Controller as MachineSyncController
participant Router as PlatformRouter
participant NutanixConv as NutanixConverter
participant CAPI as CAPI artifacts
end
Client->>Controller: Submit MAPI Machine / MachineSet
Controller->>Router: convertMAPIToCAPIMachine/Set(...)
alt platform == Nutanix
Router->>NutanixConv: FromNutanixMachineAndInfra / FromNutanixMachineSetAndInfra
NutanixConv->>NutanixConv: Unmarshal RawExtension provider spec/status
NutanixConv->>NutanixConv: Convert images, subnets, disks, GPUs, boot, categories
NutanixConv->>NutanixConv: Normalize slices, merge labels/annotations
NutanixConv-->>Router: Return CAPI Machine/Set + Nutanix templates (+ warnings/errors)
else
Router->>CAPI: delegate to existing converters
end
Router-->>Controller: Converted objects or errors
Controller-->>Client: Return results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/machinesync/machine_sync_capi2mapi_infrastructure.go (1)
78-85: Avoid converting a nil provider status (possible silent status wipe).When platform isn’t matched, newProviderStatus stays nil. Converting nil may error or write an empty RawExtension. Guard it.
Apply this diff:
func setChangedMAPIMachineProviderStatusFields(platform configv1.PlatformType, existingMAPIMachine, convertedMAPIMachine *mapiv1beta1.Machine) error { var newProviderStatus interface{} … } + // No changes for this platform; leave ProviderStatus untouched. + if newProviderStatus == nil { + return nil + } + rawExtension, err := capi2mapi.RawExtensionFromInterface(newProviderStatus) if err != nil { return fmt.Errorf("unable to convert ProviderStatus to RawExtension: %w", err) }
🧹 Nitpick comments (10)
pkg/conversion/mapi2capi/nutanix_test.go (3)
37-39: Avoid shared mutable state in the builder.nutanixProviderSpecBuilder holds a pointer to the spec while methods have value receivers; copies share the same underlying spec and can leak mutations across tests. Prefer a value spec to make builders immutable-by-copy.
Apply this refactor:
-type nutanixProviderSpecBuilder struct { - providerSpec *mapiv1.NutanixMachineProviderConfig -} +type nutanixProviderSpecBuilder struct { + providerSpec mapiv1.NutanixMachineProviderConfig +} @@ func NutanixProviderSpec() nutanixProviderSpecBuilder { - return nutanixProviderSpecBuilder{ - providerSpec: &mapiv1.NutanixMachineProviderConfig{ + return nutanixProviderSpecBuilder{ + providerSpec: mapiv1.NutanixMachineProviderConfig{ TypeMeta: metav1.TypeMeta{ APIVersion: "machine.openshift.io/v1", Kind: "NutanixMachineProviderConfig", }, VCPUSockets: 2, VCPUsPerSocket: 1, MemorySize: resource.MustParse("4Gi"), SystemDiskSize: resource.MustParse("120Gi"), Image: mapiv1.NutanixResourceIdentifier{ Type: mapiv1.NutanixIdentifierName, Name: ptr.To("test-image"), }, Subnets: []mapiv1.NutanixResourceIdentifier{ { Type: mapiv1.NutanixIdentifierUUID, UUID: ptr.To("subnet-uuid"), }, }, Cluster: mapiv1.NutanixResourceIdentifier{ Type: mapiv1.NutanixIdentifierUUID, UUID: ptr.To("cluster-uuid"), }, Project: mapiv1.NutanixResourceIdentifier{ Type: mapiv1.NutanixIdentifierUUID, UUID: ptr.To("project-uuid"), }, BootType: mapiv1.NutanixLegacyBoot, - }, + }, } } @@ func (n nutanixProviderSpecBuilder) Build() *mapiv1.NutanixMachineProviderConfig { - return n.providerSpec.DeepCopy() + return n.providerSpec.DeepCopy() }No call sites change; chaining semantics stay intact but isolate state per builder value.
Also applies to: 42-74, 82-84
86-96: Optional: encode provider spec via k8s serializer to JSON.RawExtension conventionally carries JSON. Using the scheme serializer avoids YAML/JSON ambiguity and ensures type meta is preserved as API expects.
Example:
// assuming scheme is initialized with machine.openshift.io/v1 encoder := serializer.NewCodecFactory(scheme).LegacyCodec(mapiv1.SchemeGroupVersion) raw := runtime.RawExtension{} raw.Raw, _ = runtime.Encode(encoder, n.providerSpec.DeepCopy()) return &raw
570-587: Minor: prefer typed constants over raw strings.Use mapiv1.NutanixDiskDeviceTypeDisk and mapiv1.NutanixDiskAdapterTypeSCSI for consistency with the rest of the suite.
pkg/controllers/machinesync/machine_sync_capi2mapi_infrastructure.go (1)
58-75: Nutanix conditions merge looks consistent with AWS. Add a guard for nil ProviderStatus inputs.If either ProviderStatus RawExtension is nil, ensure mapi2capi.NutanixProviderStatusFromRawExtension handles it; otherwise, bail early to avoid errors. Add a short-circuit if no conditions to merge.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixclustertemplate_types.go (1)
28-31: Add explicit resource path/shortName/scope (+storageversion) for consistency and discoverability.
Cluster template currently has only categories. Recommend aligning with machine template.If keeping local patches in vendor is undesirable, consider bumping to an upstream version that includes this.
//+kubebuilder:object:root=true -//+kubebuilder:resource:categories=cluster-api +//+kubebuilder:resource:path=nutanixclustertemplates,shortName=nctmpl,scope=Namespaced,categories=cluster-api +//+kubebuilder:storageversionvendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go (1)
227-231: Optional pointer field serialized as null; add omitempty.
StorageContainer *NutanixResourceIdentifieris marked +optional but will marshal as"storageContainer": null. Prefer omitting nulls.- StorageContainer *NutanixResourceIdentifier `json:"storageContainer"` + StorageContainer *NutanixResourceIdentifier `json:"storageContainer,omitempty"`vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/types.go (2)
73-79: Mark optional string fields with omitempty to avoid emitting empty strings.
These are tagged +optional but will serialize as empty strings, which can complicate diffs/patches and downstream validation.type NutanixCredentialReference struct { @@ - Namespace string `json:"namespace"` + Namespace string `json:"namespace,omitempty"` } @@ type NutanixTrustBundleReference struct { @@ - Data string `json:"data"` + Data string `json:"data,omitempty"` @@ - Name string `json:"name"` + Name string `json:"name,omitempty"` @@ - Namespace string `json:"namespace"` + Namespace string `json:"namespace,omitempty"` }Also applies to: 95-103
34-46: Ensure credentials are never logged or included in error messages.
Types embed usernames/passwords and secret refs. Validate that callers sanitize logs and redact values.Would you like a quick grep script to scan for potential secret logging in this repo?
Also applies to: 108-131
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanix_types.go (2)
113-131: Consider adding basic validation helpers for NutanixResourceIdentifier.A Validate() method could assert that when Type==uuid, UUID is non-nil/non-empty (and Name is nil), and vice versa. Improves early error surfacing.
I can draft a minimal Validate() and unit tests if useful.
89-103: Small UX improvement: DisplayString fallback.Returning something like
fmt.Sprintf("%s=<unset>", nri.Type)may aid debugging vs “unknown”.
📜 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 (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
go.mod(3 hunks)pkg/controllers/machinesetsync/machineset_sync_controller.go(1 hunks)pkg/controllers/machinesync/machine_sync_capi2mapi_infrastructure.go(1 hunks)pkg/controllers/machinesync/machine_sync_controller.go(1 hunks)pkg/conversion/mapi2capi/nutanix.go(1 hunks)pkg/conversion/mapi2capi/nutanix_test.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/LICENSE(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/doc.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/groupversion_info.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanix_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixclustertemplate_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixfailuredomain_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/zz_generated.deepcopy.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/LICENSE(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/parsecreds.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/types.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/zz_generated.deepcopy.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/types/types.go(1 hunks)vendor/modules.txt(1 hunks)
🔇 Additional comments (35)
vendor/modules.txt (1)
956-963: Vendor adds Nutanix modules requiring go 1.24.Matches go.mod. Nothing blocking; just ensure CI uses 1.24 as noted above.
Confirm your build images aren’t pinning older Go versions.
vendor/github.com/nutanix-cloud-native/prism-go-client/LICENSE (1)
1-202: License included.Apache-2.0 text looks intact. No action.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/LICENSE (1)
1-202: License included.Apache-2.0 text looks intact. No action.
pkg/conversion/mapi2capi/nutanix_test.go (1)
799-807: Duplicated error messages.This test expects the same infra error twice. If duplication isn’t intentional, either dedup errors in the converter before returning or adjust the test to assert a single occurrence.
Would you prefer we normalize/unique errors at the conversion boundary? I can send a small patch to fold duplicates.
vendor/github.com/nutanix-cloud-native/prism-go-client/environment/types/types.go (2)
15-15: No call sites found in the codebase.The prism-go-client types.ErrNotFound error is not used anywhere outside the vendor directory. Since this is a third-party vendored library with no visible call sites in the repository, the review comment cannot be verified locally. This could indicate the library is newly added, not yet utilized, or only consumed through indirect APIs. Manual verification of whether external callers of this library properly use errors.Is() is recommended if the library is exposed as a public API.
34-41: Acknowledge the review comment is technically correct; verify no current marshaling occurs.The review correctly identifies that
*url.URLwith ajson:"address,omitempty"tag will serialize struct fields rather than a string representation. Since Go'snet/url.URLdoes not implementjson.Marshaler, encoding/json falls back to struct field serialization.However, searches for marshaling patterns targeting
ManagementEndpointin non-vendor code (pkg/, cmd/) returned no results, indicating no current code path triggers this issue. The type resides in vendored code (vendor/github.com/nutanix-cloud-native/prism-go-client), which is an indirect dependency and not typically modified.If future code needs to serialize
ManagementEndpointto JSON withaddressas a plain string, a custom wrapper type implementingjson.Marshaler/json.Unmarshalerwould be required outside the vendor directory—as the review suggests.go.mod (2)
306-307: No breaking changes or concerns identified for kustomize v0.19.0.kyaml v0.19.0 does not list any breaking changes or migration steps — the release mainly unifies internal package versions and updates dependencies. The git history shows these are new dependencies being added (not upgrades from prior versions), so no migration path is needed. The only relevant breaking change in kustomize's kyaml line was v0.18.0's removal of Starlark support for KRM functions, which does not apply to v0.19.0. No documented issues exist for v0.19.0's impact on existing generators or patching mechanisms.
3-3: CI/tooling is already configured for Go 1.24.Verification confirms:
Dockerfile.rhelexplicitly usesrhel-9-golang-1.24-openshift-4.21, andgo.modcorrectly declaresgo 1.24.0. Workspace and toolchain configurations confirm Go 1.24.x across submodules (go.work,hack/tools/go.mod). No build/test drift detected.pkg/conversion/mapi2capi/nutanix.go (9)
52-60: LGTM! Good defensive programming.The
ensureEmptySliceNotNilhelper prevents reconciliation loops caused by nil vs empty slice differences. This is a valuable pattern for maintaining stable comparisons.
62-79: LGTM! Constructor functions follow established patterns.The
FromNutanixMachineAndInfraandFromNutanixMachineSetAndInfraconstructors correctly follow the pattern used by other platform providers (AWS, OpenStack, PowerVS) in this codebase.
81-102: LGTM! Proper template naming with spec hash.The use of
util.GenerateInfraMachineTemplateNameWithSpecHashensures immutability by including a hash of the spec in the template name, which aligns with CAPI best practices.
144-155: LGTM! Proper infrastructure validation.The validation ensures that
infrastructure.Status.InfrastructureNameis present before proceeding with cluster name assignment, which prevents potential nil pointer issues downstream.
176-200: LGTM! Proper spec and status extraction.Both
nutanixProviderSpecFromRawExtensionandNutanixProviderStatusFromRawExtensionhandle nil inputs gracefully and provide clear error messages. Exporting the status function is appropriate as it's likely used by the status sync infrastructure controller.
247-256: LGTM! Consistent slice normalization.The normalization of nil vs empty slices using
ensureEmptySliceNotNilis applied consistently and includes special handling for the optionalAdditionalCategoriesfield. This defensive programming prevents unnecessary reconciliation loops.
599-623: LGTM! Proper handling of unsupported boot types.The function correctly handles the
SecureBootboot type by issuing a warning and falling back toLegacy, as SecureBoot is not supported in CAPX. The default case also provides a helpful warning for unset boot types.
324-338: No issue found; nil return is intentional and correct.The
convertCategoriesToCAPXfunction'snilreturn for empty categories is intentional. The code at lines 254-256 explicitly validates this pattern withif spec.AdditionalCategories != nil, treatingAdditionalCategoriesas an optional field wherenilhas semantic meaning (field not set), unlikeDataDisks,GPUs, andSubnetswhich are unconditionally normalized to empty slices. This design prevents unnecessary spec mutations that cause reconciliation loops.
640-647: Type conversion is correct—DeviceID is int64 in CAPX, not string.The web search result was inaccurate. DeviceID in the CAPX NutanixGPU struct is defined as
*int64, not a string. The conversion from MAPI's*int32to CAPX's*int64is a safe widening conversion with no overflow risk. The tests confirm this conversion works correctly with values likeint32(12345).pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
710-711: LGTM! Proper Nutanix platform integration.The Nutanix case correctly follows the established pattern used by other platforms (AWS, OpenStack, PowerVS) and routes through the appropriate converter
FromNutanixMachineSetAndInfra.pkg/controllers/machinesync/machine_sync_controller.go (1)
589-590: LGTM! Proper Nutanix platform integration.The Nutanix case correctly follows the established pattern used by other platforms (AWS, OpenStack, PowerVS) and routes through the appropriate converter
FromNutanixMachineAndInfra.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/doc.go (1)
1-18: LGTM! Standard package documentation.This is standard Go package documentation with proper licensing and package declaration.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_conversion.go (1)
1-23: LGTM! Standard conversion hub markers.The
Hub()methods designateNutanixClusterandNutanixClusterListas conversion hubs, following the standard controller-runtime pattern for API version conversion.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_conversion.go (1)
19-23: LGTM: Hub markers are correct for conversion hub wiring.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_conversion.go (1)
19-23: LGTM: Template Hub markers align with CAPI expectations.vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/parsecreds.go (1)
17-35: Let me search for the parsecreds.go file directly to examine its full context:Based on my investigation, I cannot directly access the types.go file to verify all credential types defined in the prism-go-client codebase due to the file being in a vendored directory. However, the earlier ripgrep output shows:
BasicAuthCredentialTypeconstant is definedCredentialstruct withTypeandDatafieldsNutanixCredentialsstruct with aCredentialsslice- The loop iterates through
creds.CredentialsThe review comment's concern about skipping unsupported credential types and continuing the loop is a logical optimization IF multiple credential types can exist in a single
Credentialsslice AND only one valid type needs to be found per call. However, without seeing:
- All defined
CredentialTypevalues- The function signature and its documented intent
- Callers' expectations
- Whether mixed-type credential lists are a valid use case
I cannot definitively confirm whether this change would be correct or introduce a bug.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/groupversion_info.go (1)
27-44: LGTM: GroupVersion/SchemeBuilder wiring is correct and idiomatic.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_types.go (1)
36-46: LGTM: NutanixMachineTemplate mirrors CAPI template conventions.
Scheme registration and kubebuilder markers look good.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go (2)
147-151: Enum values for BootType: verify they match upstream constants and user-facing docs.
Marker uses lower-caselegacy;uefiwhile the type isNutanixBootType. Ensure case/values align with what controllers expect to avoid CRD validation passes but controller rejects.
135-141: The review comment is based on an incorrect premise and should be disregarded.The
omitzerooption was added to encoding/json in Go 1.24, and the project specifiesgo 1.24.0in its go.mod. Thejson:"cluster,omitzero"tag is valid and intentional, not a bug.Regarding the
Subnetsfield withjson:"subnet"tag: singular/plural naming mismatches between Go fields and JSON tags are common in API design for wire-format compatibility and are often intentional. Without evidence that this is an error (rather than deliberate API design), and given this is vendored upstream code, no changes should be made here.Likely an incorrect or invalid review comment.
vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/zz_generated.deepcopy.go (2)
40-47: DeepCopy for Credential.Data (json.RawMessage) is correct.make+copy avoids aliasing; no issues spotted.
16-22: Vendored autogenerated file verified — code changes approved.File is confirmed as unmodified autogenerated output from controller-gen (v0.5.0 of prism-go-client per vendor/modules.txt). All deepcopy methods follow standard patterns: flat structs use simple pointer copies, json.RawMessage and slices are properly allocated, and pointer fields include nil checks. No hand-edits detected.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go (3)
19-27: No action required. Repository uses Go 1.24.0, which exceeds the Go 1.21+ requirement forcmp.Orusage. The code will compile without errors.
168-181: Unable to verify impact—function not called within this repository.The search found no call sites for
GetPrismCentralCredentialRef()anywhere in the codebase. This function exists only in vendor code (Nutanix provider).The original concern about inconsistent error handling remains valid: the function returns an error when
CredentialRefis nil (line 174) but returns(nil, nil)whenKind != SecretKind(line 177). Similar Kind checks in the non-vendor codebase (e.g.,machine_sync_controller.go,aws.go) consistently return errors on kind mismatches rather than silent nils.However, since there are no call sites in this repository, the actual impact cannot be determined locally. This suggests the function is either part of the external API or not yet integrated into this codebase.
153-156: Verify whether this vendored code requires immediate correction or upstream escalation.The contradiction is confirmed:
+kubebuilder:validation:Requiredconflicts withjson:",omitempty"because the field is marked required but will be omitted from JSON serialization when the boolean isfalse. The web search confirms this is a real schema/serialization issue.However, this file is in the vendor directory (third-party code), so if you're not directly maintaining this dependency, consider:
- Whether this is an upstream issue to report to nutanix-cloud-native
- Whether your PR changes require you to address this, or if this is pre-existing technical debt
- For reference, the adjacent
Subnetsfield follows the correct pattern (has+Requiredbut noomitempty)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go (1)
84-85: Do not modify vendored code; fix upstream instead.The constant "ProjectAssignationFailed" has only one reference in this repository (its definition at line 84). While renaming to "ProjectAssignmentFailed" would improve readability and fix the awkward spelling, this file is vendored code (
vendor/github.com/nutanix-cloud-native/...) and should not be modified locally. Instead, submit the fix to the upstream repository (nutanix-cloud-native/cluster-api-provider-nutanix) so it propagates through dependency updates.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/zz_generated.deepcopy.go (1)
19-28: Autogenerated vendor file is correctly synced.CAPX v1.7.2 is properly tracked in vendor/modules.txt. The deep copy patterns are standard controller-gen output with no issues.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go
Show resolved
Hide resolved
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go
Show resolved
Hide resolved
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go
Show resolved
Hide resolved
...ub.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go
Show resolved
Hide resolved
.../nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixfailuredomain_types.go
Show resolved
Hide resolved
abhay-nutanix
force-pushed
the
mapi2capi
branch
from
5b92d36 to
b434771
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
go.mod (1)
14-14: Version compatibility concern already raised.A previous review identified that CAPX v1.7.2 may not support CAPI v1.10.4. Please refer to the existing comment for details.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixfailuredomain_types.go (1)
48-53: Fix Conditions type to align with CAPI helpers.Change []capiv1.Condition to capiv1.Conditions; current Get/Set won’t compile as-is.
type NutanixFailureDomainStatus struct { // conditions represent the latest states of the failure domain. // +optional - Conditions []capiv1.Condition `json:"conditions,omitempty"` + Conditions capiv1.Conditions `json:"conditions,omitempty"` }Also applies to: 70-78
🧹 Nitpick comments (4)
pkg/conversion/mapi2capi/nutanix_test.go (1)
571-586: Prefer typed constants over raw strings for valid enum values.Using string literals for DeviceType/AdapterType/DiskMode is brittle. Switch to mapiv1 constants to avoid drift.
- DeviceType: "Disk", - AdapterType: "SCSI", + DeviceType: mapiv1.NutanixDiskDeviceTypeDisk, + AdapterType: mapiv1.NutanixDiskAdapterTypeSCSI, ... - DiskMode: "Standard", + DiskMode: mapiv1.NutanixDiskModeStandard,vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go (1)
227-231: Optional pointer should use omitempty.StorageContainer is optional but lacks omitempty; without it, JSON may include nulls.
- StorageContainer *NutanixResourceIdentifier `json:"storageContainer"` + StorageContainer *NutanixResourceIdentifier `json:"storageContainer,omitempty"`pkg/conversion/mapi2capi/nutanix.go (2)
145-156: Extract duplicated infrastructure validation to a helper function.The infrastructure validation logic (checking nil, empty InfrastructureName, and setting ClusterName/Labels) is duplicated between
ToMachineSetAndMachineTemplateandtoMachineAndInfrastructureMachine. Consider extracting this to a shared helper function to improve maintainability.Example helper:
func validateAndSetInfrastructureName(infra *configv1.Infrastructure, obj metav1.Object, clusterNameSetter func(string)) field.ErrorList { if infra == nil || infra.Status.InfrastructureName == "" { var infraName string if infra != nil { infraName = infra.Status.InfrastructureName } return field.ErrorList{field.Invalid( field.NewPath("infrastructure", "status", "infrastructureName"), infraName, "infrastructure cannot be nil and infrastructure.Status.InfrastructureName cannot be empty", )} } clusterNameSetter(infra.Status.InfrastructureName) labels := obj.GetLabels() if labels == nil { labels = make(map[string]string) } labels[clusterv1.ClusterNameLabel] = infra.Status.InfrastructureName obj.SetLabels(labels) return nil }Also applies to: 381-391
248-257: Consider removing redundant slice normalization.The comment on line 250 acknowledges that conversion functions already ensure empty slices, yet this code defensively normalizes them again. While this defensive approach is safe, it adds minor overhead. Since the conversion functions (
convertDataDisksToCAPX,convertNutanixGPUToCAPX, etc.) already callensureEmptySliceNotNilinternally, you could remove this redundant normalization block.
📜 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 (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
go.mod(3 hunks)pkg/controllers/machinesetsync/machineset_sync_controller.go(1 hunks)pkg/controllers/machinesync/machine_sync_capi2mapi_infrastructure.go(1 hunks)pkg/controllers/machinesync/machine_sync_controller.go(1 hunks)pkg/conversion/mapi2capi/nutanix.go(1 hunks)pkg/conversion/mapi2capi/nutanix_test.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/LICENSE(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/doc.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/groupversion_info.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanix_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixclustertemplate_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixfailuredomain_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/zz_generated.deepcopy.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/LICENSE(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/parsecreds.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/types.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/zz_generated.deepcopy.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/types/types.go(1 hunks)vendor/modules.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/controllers/machinesetsync/machineset_sync_controller.go
- pkg/controllers/machinesync/machine_sync_controller.go
- vendor/github.com/nutanix-cloud-native/prism-go-client/environment/types/types.go
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/groupversion_info.go
- vendor/github.com/nutanix-cloud-native/prism-go-client/LICENSE
- vendor/modules.txt
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_conversion.go
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/LICENSE
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_types.go
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/zz_generated.deepcopy.go
🔇 Additional comments (18)
vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/zz_generated.deepcopy.go (1)
1-166: LGTM! Autogenerated deepcopy code follows standard patterns.The generated DeepCopy methods correctly handle nil checks, deep copy slices element-wise, properly copy json.RawMessage data, and safely allocate and copy pointer fields.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanix_types.go (1)
1-155: LGTM! Type definitions and helper methods are well-implemented.The NutanixResourceIdentifier and related types include proper nil-safety checks, clear enum definitions, and useful helper methods (String, DisplayString, IsUUID, IsName, EqualTo) for resource identification and comparison.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/doc.go (1)
1-18: LGTM! Standard package declaration file.This doc.go file follows Go conventions for package documentation.
pkg/controllers/machinesync/machine_sync_capi2mapi_infrastructure.go (1)
58-76: LGTM! Nutanix platform handling follows established AWS pattern.The Nutanix case correctly extracts provider statuses, merges conditions via SetMAPIProviderCondition, and prepares the result for RawExtension conversion, consistent with the existing AWS implementation.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_conversion.go (1)
1-23: LGTM! Standard conversion hub markers.The Hub() methods follow Kubernetes conversion patterns to designate NutanixMachine and NutanixMachineList as canonical versions for API conversions.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_conversion.go (1)
1-23: LGTM! Standard conversion hub markers.The Hub() methods follow Kubernetes conversion patterns to designate NutanixMachineTemplate and NutanixMachineTemplateList as canonical versions for API conversions.
vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/parsecreds.go (2)
16-16: Note the limitation: only single API endpoint is currently supported.The TODO comment indicates this is a known limitation. Ensure consumers of this vendored function are aware that multi-endpoint scenarios are not yet handled.
10-37: LGTM! Credential parsing logic is sound for the stated scope.The function properly unmarshals credentials, validates BasicAuth type, checks for required PrismCentral fields, and returns appropriate errors. The single-endpoint limitation (Line 16 TODO) is documented.
pkg/conversion/mapi2capi/nutanix_test.go (1)
36-74: Builder pattern looks solid and readable.Good use of a fluent spec builder and DeepCopy for isolation across table entries.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixclustertemplate_types.go (1)
23-55: Type scaffolding and scheme registration LGTM.Matches common CAPI patterns; no issues.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go (2)
135-136: Non-standard JSON option "omitzero".json:"cluster,omitzero" is not a standard encoding/json option; it will not omit zero values. If omission is desired, prefer a pointer plus omitempty or keep value and use
omitempty.If omission is intended, consider:
- Cluster NutanixResourceIdentifier `json:"cluster,omitzero"` + Cluster *NutanixResourceIdentifier `json:"cluster,omitempty"`Please verify against upstream CAPX to avoid diverging from the provider’s API.
136-141: This issue is in vendored code—verify if modification is intentional.The JSON tag mismatch (Subnets field with
json:"subnet") exists in vendor code from upstream v1.7.2. Before fixing locally:
- Confirm if this PR intentionally modifies vendored code or if it's an accidental inclusion
- Check if the project has a documented strategy for local patches to vendored dependencies
- Consider reporting/fixing this upstream instead, then pulling in the vendor update
If local modification is intentional, the fix remains:
- Subnets []NutanixResourceIdentifier `json:"subnet,omitempty"` + Subnets []NutanixResourceIdentifier `json:"subnets,omitempty"`Any local changes to vendor code risk being overwritten on the next vendor sync.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go (1)
1-94: Vendored dependency: issues should be addressed upstream.This file is vendored from the upstream Nutanix Cluster API provider. While previous review comments identified valid issues (typo on line 27, incorrect comment on lines 81-85, and type inconsistency on line 92), these should be fixed in the upstream repository rather than here. Consider opening issues or pull requests at https://github.com/nutanix-cloud-native/cluster-api-provider-nutanix if these concerns impact your integration.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go (1)
1-212: Vendored dependency: documentation improvements should be addressed upstream.This file is vendored from the upstream Nutanix Cluster API provider. The previous review comment regarding clarifying documentation for
GetPrismCentralTrustBundle()(lines 183-193) is valid but should be addressed in the upstream repository at https://github.com/nutanix-cloud-native/cluster-api-provider-nutanix.pkg/conversion/mapi2capi/nutanix.go (4)
52-60: LGTM: Clean slice normalization helper.The
ensureEmptySliceNotNilhelper correctly prevents nil vs. empty slice inconsistencies that would cause unnecessary reconciliation loops. The generic implementation and clear documentation make this a well-designed utility.
118-122: Type assertion now handles errors correctly.The previous concern about using
panicfor type assertion failures has been addressed. The current code properly appends the error and returns early with an aggregated error, allowing graceful error handling.
588-598: Well-designed optional identifier handling.The
convertOptionalNutanixResourceIdentifierToCAPXfunction provides clear separation between required and optional resource identifiers, treating empty Type as "unspecified" without error. This is a good abstraction that improves code clarity.
600-624: LGTM: Robust boot type conversion with appropriate warnings.The boot type conversion correctly handles unsupported values (SecureBoot) and unspecified values by falling back to Legacy with clear warnings. This defensive approach ensures compatibility while informing users of the conversions.
abhay-nutanix
force-pushed
the
mapi2capi
branch
from
3ba902b to
3f4259e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go (3)
27-29: Typo: “releated” → “related”.-// Conditions and Reasons releated to failure domain +// Conditions and Reasons related to failure domain
81-85: Comment references wrong constant.Update to describe ProjectAssignedCondition, not VMAddressesAssignedCondition.
-// VMAddressesAssignedCondition shows the status of the process of assigning the VMs to a project +// ProjectAssignedCondition shows the status of assigning the VM(s) to a project
88-94: Inconsistent type for TrustBundleSecretOwnerSetCondition.Align with other “...Condition” constants typed as capiv1.ConditionType.
- CredentialRefSecretOwnerSetFailed = "CredentialRefSecretOwnerSetFailed" - TrustBundleSecretOwnerSetCondition = "TrustBundleSecretOwnerSet" - TrustBundleSecretOwnerSetFailed = "TrustBundleSecretOwnerSetFailed" + CredentialRefSecretOwnerSetFailed = "CredentialRefSecretOwnerSetFailed" + TrustBundleSecretOwnerSetCondition capiv1.ConditionType = "TrustBundleSecretOwnerSet" + TrustBundleSecretOwnerSetFailed = "TrustBundleSecretOwnerSetFailed"vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixfailuredomain_types.go (1)
48-53: Use capiv1.Conditions for status Conditions.CAPI helpers expect capiv1.Conditions; []capiv1.Condition breaks Get/Set patterns.
type NutanixFailureDomainStatus struct { // conditions represent the latest states of the failure domain. // +optional - Conditions []capiv1.Condition `json:"conditions,omitempty"` + Conditions capiv1.Conditions `json:"conditions,omitempty"` }GetConditions/SetConditions remain correct with this change.
Also applies to: 70-78
🧹 Nitpick comments (2)
vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/types.go (1)
78-79: Prefer omitting optional zero-values in JSON.Several optional fields are serialized even when empty. Add
omitemptyto cut noise and avoid persisting empty secrets.- Namespace string `json:"namespace"` + Namespace string `json:"namespace,omitempty"`- Data string `json:"data"` + Data string `json:"data,omitempty"`- Name string `json:"name"` + Name string `json:"name,omitempty"`- Namespace string `json:"namespace"` + Namespace string `json:"namespace,omitempty"`- Insecure bool `json:"insecure"` + Insecure bool `json:"insecure,omitempty"`Also applies to: 96-103, 120-123
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go (1)
183-193: Past review comment about trust bundle documentation remains unaddressed.The previous review suggested adding a clarifying comment to document that this function intentionally returns
nilfor inline trust bundles (Kind == "String") and only supports ConfigMap references. The current comment doesn't explain this selective filtering behavior.However, since this is vendored code from upstream, the appropriate action would be to:
- Document this behavior in your integration code that calls this function, OR
- Contribute the documentation improvement upstream to the Nutanix CAPI provider
📜 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 (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
go.mod(3 hunks)pkg/controllers/machinesetsync/machineset_sync_controller.go(1 hunks)pkg/controllers/machinesync/machine_sync_capi2mapi_infrastructure.go(1 hunks)pkg/controllers/machinesync/machine_sync_controller.go(1 hunks)pkg/conversion/mapi2capi/nutanix.go(1 hunks)pkg/conversion/mapi2capi/nutanix_test.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/LICENSE(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/doc.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/groupversion_info.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanix_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixclustertemplate_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixfailuredomain_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/zz_generated.deepcopy.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/LICENSE(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/parsecreds.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/types.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/zz_generated.deepcopy.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/types/types.go(1 hunks)vendor/modules.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/controllers/machinesetsync/machineset_sync_controller.go
- pkg/controllers/machinesync/machine_sync_controller.go
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_conversion.go
- pkg/controllers/machinesync/machine_sync_capi2mapi_infrastructure.go
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_conversion.go
- vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/parsecreds.go
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/doc.go
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_conversion.go
- vendor/github.com/nutanix-cloud-native/prism-go-client/LICENSE
- vendor/modules.txt
- go.mod
🔇 Additional comments (8)
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/LICENSE (1)
1-202: License file appropriately included for vendored dependency.The Apache License 2.0 text is complete and correctly formatted. This license file properly accompanies the vendored
cluster-api-provider-nutanixdependency and ensures compliance with the upstream project's licensing requirements.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/groupversion_info.go (1)
27-44: LGTM — scheme registration looks standard.vendor/github.com/nutanix-cloud-native/prism-go-client/environment/types/types.go (1)
14-16: LGTM — environment and endpoint types look good.Also applies to: 28-42
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixclustertemplate_types.go (1)
28-50: LGTM — CRD scaffolding is conventional and complete.Also applies to: 52-55
pkg/conversion/mapi2capi/nutanix_test.go (2)
158-169: Solid coverage and table design.Nice breadth on identifiers, GPUs, disks, boot types, and infra errors.
160-163: No latent cross-test coupling — verified.
NutanixProviderSpecClean() returns a value-type builder (nutanixProviderSpecBuilder), Machine/MachineSet WithProviderSpecBuilder stores a copy of the interface value, and the vendor Nutanix BuildRawExtension marshals a fresh runtime.RawExtension on each Build; no mutation leaks detected.pkg/conversion/mapi2capi/nutanix.go (2)
74-161: LGTM! Comprehensive error handling with proper nil guards.The conversion function demonstrates excellent defensive programming with multiple nil guards and proper error accumulation. The past review concern about using panic for type assertion failures has been properly addressed - the code now returns errors instead (lines 98-102).
590-615: Verify storage container identifier type restriction.The conversion explicitly rejects
Nametype identifiers for storage containers (line 600), only acceptingUUIDtype. This differs from the generalconvertNutanixResourceIdentifierToCAPXwhich accepts both types.Please confirm this is intentional - does the Nutanix storage API only support UUID-based lookups for storage containers? If so, consider adding a clarifying comment:
// Note: Storage container identifiers only support UUID type, not Name type case mapiv1.NutanixIdentifierName: errors = append(errors, field.Invalid(field.NewPath("type"), id.Type, "storage containers only support UUID identifier type"))
...or/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanix_types.go
Show resolved
Hide resolved
...or/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanix_types.go
Show resolved
Hide resolved
abhay-nutanix
force-pushed
the
mapi2capi
branch
from
2278a08 to
d67071e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
go.mod (1)
14-14: Acknowledge existing compatibility concern.A previous review has already flagged that CAPX v1.7.2 may not support CAPI v1.10.4. Please ensure this compatibility issue is resolved before merging.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go (1)
183-193: Add clarifying comment explaining trust bundle filtering behavior.The function returns
nilwhenKind == NutanixTrustBundleKindString(line 188), intentionally rejecting inline trust bundles and only accepting ConfigMap-based references. The current comment doesn't explain this filtering behavior, making the design intent unclear.This issue was previously flagged and remains unaddressed in the vendored code.
Enhance the function comment to document the filtering behavior:
-// GetPrismCentralTrustBundle returns the trust bundle reference for the Nutanix Prism Central. +// GetPrismCentralTrustBundle returns the trust bundle reference for the Nutanix Prism Central. +// Returns nil for inline trust bundles (Kind == "String") as only ConfigMap-based references are supported. func (ncl *NutanixCluster) GetPrismCentralTrustBundle() *credentialTypes.NutanixTrustBundleReference {Note: Since this is vendored code, the improvement should be contributed upstream to
cluster-api-provider-nutanix.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixfailuredomain_types.go (1)
48-78: Type mismatch: Conditions field should usecapiv1.Conditionstype alias.The
Conditionsfield (line 52) is declared as[]capiv1.Condition, but the accessor methodsGetConditions()(line 71) andSetConditions()(line 76) usecapiv1.Conditions. This creates a type inconsistency. CAPI utilities and helpers expect thecapiv1.Conditionstype alias, not the raw slice type.This issue was previously flagged and remains unresolved in the vendored code.
Apply this diff to fix the type mismatch:
type NutanixFailureDomainStatus struct { // conditions represent the latest states of the failure domain. // +optional - Conditions []capiv1.Condition `json:"conditions,omitempty"` + Conditions capiv1.Conditions `json:"conditions,omitempty"` }Note: Since this is vendored code, the fix should be contributed upstream to
cluster-api-provider-nutanix.
🧹 Nitpick comments (1)
vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/types.go (1)
34-40: Consider adding kubebuilder marker for consistency.
BasicAuthCredentiallacks the+kubebuilder:object:generate=truemarker while related types likeBasicAuth,PrismCentralBasicAuth, andPrismElementBasicAuthhave it. AlthoughBasicAuthCredentialis embedded injson.RawMessage, adding the marker ensures consistency and future-proofs the type if it's used directly in API objects.
📜 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 (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
go.mod(3 hunks)pkg/controllers/machinesetsync/machineset_sync_controller.go(1 hunks)pkg/controllers/machinesync/machine_sync_capi2mapi_infrastructure.go(1 hunks)pkg/controllers/machinesync/machine_sync_controller.go(1 hunks)pkg/conversion/mapi2capi/nutanix.go(1 hunks)pkg/conversion/mapi2capi/nutanix_test.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/LICENSE(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/doc.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/groupversion_info.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanix_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixclustertemplate_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixfailuredomain_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_conversion.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_types.go(1 hunks)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/zz_generated.deepcopy.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/LICENSE(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/parsecreds.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/types.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/zz_generated.deepcopy.go(1 hunks)vendor/github.com/nutanix-cloud-native/prism-go-client/environment/types/types.go(1 hunks)vendor/modules.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- vendor/github.com/nutanix-cloud-native/prism-go-client/environment/types/types.go
🚧 Files skipped from review as they are similar to previous changes (7)
- vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/parsecreds.go
- vendor/modules.txt
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/LICENSE
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixclustertemplate_types.go
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_conversion.go
- vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanix_types.go
🔇 Additional comments (20)
vendor/github.com/nutanix-cloud-native/prism-go-client/LICENSE (1)
1-202: License file properly included for vendored dependency.The Apache License 2.0 text is correctly formatted and represents the standard license for the prism-go-client module. Including vendor licenses is a compliance best practice.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/doc.go (1)
1-18: LGTM!Standard package documentation with proper Apache 2.0 license header.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_conversion.go (1)
19-23: LGTM!Standard conversion hub marker methods following Kubernetes controller-runtime conventions.
vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/zz_generated.deepcopy.go (1)
1-166: LGTM!Autogenerated deepcopy code with correct nil checks, slice copying, and pointer handling.
pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
710-711: LGTM!Nutanix platform routing follows the established pattern used by AWS, OpenStack, and PowerVS conversions.
pkg/controllers/machinesync/machine_sync_controller.go (1)
589-590: LGTM!Nutanix machine conversion follows the same pattern as other supported platforms.
pkg/controllers/machinesync/machine_sync_capi2mapi_infrastructure.go (1)
58-75: LGTM!Nutanix provider status handling correctly mirrors the AWS implementation, properly merging conditions between existing and converted status.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_conversion.go (1)
19-23: LGTM!Standard conversion hub marker methods for NutanixMachineTemplate resources.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_types.go (1)
1-69: Vendored code looks correct.This vendored NutanixMachineTemplate type definition from the Nutanix CAPX provider is properly structured with appropriate kubebuilder markers, JSON tags, and scheme registration.
pkg/conversion/mapi2capi/nutanix_test.go (3)
36-156: Excellent test builder pattern.The fluent builder pattern with sensible defaults makes the tests highly readable and maintainable. The separation between
NutanixProviderSpec()andNutanixProviderSpecClean()constructors is a good practice for test flexibility.
158-899: Comprehensive test coverage.The table-driven tests provide excellent coverage of both success and failure scenarios, including edge cases, multiple errors, and validation of warnings. The descriptive test entry names make failures easy to diagnose.
901-960: Utility function tests are thorough.The tests for
ensureEmptySliceNotNilandNutanixProviderStatusFromRawExtensioncover all relevant cases including nil inputs, empty data, and error conditions.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go (1)
1-322: Vendored NutanixMachine types are properly integrated.This vendored CRD definition includes comprehensive field specifications, proper kubebuilder validation markers, condition helpers, and scheme registration. The types align well with the conversion logic in
pkg/conversion/mapi2capi/nutanix.go.pkg/conversion/mapi2capi/nutanix.go (3)
144-148: Panic issue from previous review has been fixed.The type assertion now properly returns an error instead of panicking, allowing graceful error handling and recovery. Good fix!
82-112: Solid error handling in conversion pipeline.The conversion properly aggregates errors and warnings throughout the process, ensuring all validation issues are collected and reported together. The defensive nil checks prevent panics while allowing continued validation to surface multiple issues in one pass.
779-787: Good defensive programming practice.The
ensureEmptySliceNotNilutility prevents nil vs empty slice inconsistencies that can cause unnecessary spec mutations and reconciliation loops. The comment clearly explains the rationale.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/groupversion_info.go (1)
1-44: Vendored API group scaffolding is correct.This standard kubebuilder-generated group/version registration code properly sets up the
infrastructure.cluster.x-k8s.io/v1beta1API group for the Nutanix CAPX types.vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/types.go (1)
1-131: Vendored credential types look well-structured.The credential and endpoint types are properly annotated with kubebuilder validation markers and include appropriate field constraints for Kubernetes API usage.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go (1)
1-212: Vendored NutanixCluster types follow CAPI conventions.The CRD structure is well-organized with appropriate kubebuilder markers, validation constraints, and standard CAPI accessor patterns. The use of
cmp.Orfor namespace defaulting (line 197) is a good pattern.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/zz_generated.deepcopy.go (1)
1-788: Autogenerated deepcopy implementations are correct for current type definitions.This autogenerated file correctly implements deepcopy methods for all Nutanix v1beta1 types based on their current definitions. Note that line 376 uses
[]apiv1beta1.Conditionconsistent with theNutanixFailureDomainStatus.Conditionsfield declaration. If the type mismatch issue innutanixfailuredomain_types.gois fixed upstream, this file will need regeneration.
|
/test ? |
|
@jcpowermac: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
|
/lgtm |
|
Scheduling tests matching the |
|
@abhay-nutanix: The following tests failed, say
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. |
3 participants
Summary by CodeRabbit
New Features
Dependencies
Tests
Chores