NO-JIRA: Add initial support for CAPI Nutanix provider #383
Conversation
WalkthroughAdds Nutanix infrastructure provider support: vendor Nutanix CAPI and Prism client code, operator wiring (scheme registration, reconcilers, webhooks), InfraCluster controller logic to create NutanixCluster resources, CAPI installer adjustments, e2e Nutanix tests, manifests, and dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Infra as OCP Infra (Infra.Status)
participant Operator as CAPI Operator
participant Controller as InfraCluster Controller
participant NutanixCR as NutanixCluster (K8s)
participant NutanixSvc as Nutanix (Prism)
Infra->>Operator: Infra reconciliation event
Operator->>Controller: ensureInfraCluster (detect NutanixPlatformType)
Controller->>Controller: Parse APIServerInternalURL -> host,port
Controller->>Controller: buildNutanixClusterSpec(host,port)
Controller->>NutanixCR: GET NutanixCluster
alt not found
Controller->>NutanixCR: CREATE NutanixCluster (spec incl. ControlPlaneEndpoint, optional PrismCentral, failure domains)
end
NutanixCR->>NutanixSvc: Provision/Configure per spec
NutanixSvc-->>NutanixCR: Status updates
NutanixCR-->>Controller: Ready condition set
Controller-->>Operator: Reconciliation complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[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 |
abhay-nutanix
force-pushed
the
nutanixInfraChange
branch
from
5c2cd84 to
96ef969
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (24)
go.mod(1 hunks)manifests/0000_30_cluster-api_12_clusteroperator.yaml(1 hunks)pkg/controllers/infracluster/infracluster_controller.go(1 hunks)pkg/controllers/infracluster/nutanix.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 (15)
vendor/github.com/nutanix-cloud-native/prism-go-client/environment/types/types.go (1)
1-64: LGTM! Clean API design for environment configuration.The package defines a well-structured API for environment configuration and retrieval:
- Clear separation of concerns with
ApiCredentialsandManagementEndpoint- Appropriate use of struct embedding
- Clean interface definitions for
EnvironmentandProvider- Standard error handling with
ErrNotFoundpkg/controllers/infracluster/infracluster_controller.go (1)
231-237: LGTM! Nutanix platform handling follows established patterns.The addition correctly integrates Nutanix platform support into the infrastructure cluster controller, following the same pattern used for AWS, GCP, Azure, and other platforms.
manifests/0000_30_cluster-api_12_clusteroperator.yaml (1)
142-153: LGTM! Nutanix relatedObjects entries follow established conventions.The additions correctly register Nutanix infrastructure resources (nutanixclusters, nutanixmachines, nutanixmachinetemplates) with the ClusterOperator, following the same pattern used for other infrastructure providers.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_conversion.go (1)
19-23: LGTM! Standard conversion hub markers.The empty
Hub()methods correctly markNutanixClusterandNutanixClusterListas conversion hubs, following the standard Kubernetes API conversion pattern used by controller-runtime.vendor/modules.txt (1)
956-962: Clarify Go version in go.mod
The prism-go-client v0.5.4 release exists upstream; please confirm that specifyinggo 1.24.0in the module is intentional.go.mod (1)
14-15: Dependencies confirmed — no action required. prism-go-client v0.5.4 and cluster-api-provider-nutanix v1.7.2 are available on proxy.golang.org.Likely an incorrect or invalid review comment.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_conversion.go (1)
19-23: LGTM! Standard CAPI conversion hub pattern.The Hub marker methods correctly designate NutanixMachineTemplate and NutanixMachineTemplateList as conversion hubs, following the standard Cluster API pattern for multi-version support.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_conversion.go (1)
19-23: LGTM! Standard CAPI conversion hub pattern.The Hub marker methods correctly designate NutanixMachine and NutanixMachineList as conversion hubs, following the standard Cluster API pattern.
vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/parsecreds.go (1)
16-16: Note the single API endpoint limitation.The TODO comment indicates that only a single API endpoint is currently supported. This limitation means ParseCredentials returns after processing the first valid credential, ignoring any additional credentials in the array.
Confirm whether multi-endpoint support is planned or if this single-endpoint constraint aligns with the current design requirements.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/groupversion_info.go (1)
27-44: LGTM! Standard API group registration.The group version setup correctly defines the v1beta1 API group for Nutanix infrastructure resources with proper scheme registration following standard Kubernetes API machinery patterns.
pkg/controllers/infracluster/nutanix.go (1)
49-61: LGTM! URL parsing and validation logic is correct.The API server URL parsing, port extraction, and PlatformStatus validation are properly implemented with appropriate error handling.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixclustertemplate_types.go (1)
23-55: LGTM! Standard CAPI template types.The NutanixClusterTemplate types follow the standard Cluster API template pattern with proper kubebuilder markers and scheme registration.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_types.go (1)
26-69: LGTM! Standard CAPI template types.The NutanixMachineTemplate types follow the standard Cluster API template pattern with proper kubebuilder markers, scheme registration, and appropriate resource scope annotations.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go (1)
21-94: LGTM! Well-defined condition types and reasons.The condition constants follow Cluster API conventions and are well-organized by functional area (failure domains, cluster setup, VM provisioning, credentials). The documentation comments clearly explain each condition's purpose.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/zz_generated.deepcopy.go (1)
1-788: LGTM! Generated deepcopy implementations for Nutanix v1beta1 API types.This is auto-generated code (controller-gen) from the vendored Nutanix CAPX dependency. The deepcopy implementations follow standard Kubernetes patterns and correctly handle primitives, pointers, slices, maps, and nested structs. The file aligns with the PR objective of adding Nutanix platform support.
...ub.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go
Show resolved
Hide resolved
abhay-nutanix
force-pushed
the
nutanixInfraChange
branch
from
96ef969 to
be75e24
Compare
There was a problem hiding this comment.
Actionable comments posted: 34
🧹 Nitpick comments (23)
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/diag.go (1)
37-37: Variable shadowing (optional observation).The
framesvariable shadows the frame count from line 34. While this doesn't cause a bug (the count is only used once), it could be clearer to use different variable names. However, since this is vendored code from the Azure SDK, this is just an observation and doesn't require action unless the vendor code is being patched.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/errorinfo.go (1)
18-22: Consider adding a nil guard for defensive programming.The function wraps the error without checking if it's nil. While the API contract likely expects non-nil errors, adding a nil guard would make the function more robust and prevent potential panics in the Error() method.
Apply this diff to add defensive nil handling:
func NonRetriableError(err error) error { + if err == nil { + return nil + } return &nonRetriableError{err} }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/uuid.go (1)
45-76: LGTM!The Parse() function correctly validates the UUID format and parses hex values. The lenient approach (not validating variant/version bits) is acceptable for a parser.
Optional: Consider more specific error messages.
The error messages could be more specific to help developers identify issues faster:
if s[8] != '-' || s[13] != '-' || s[18] != '-' || s[23] != '-' { - return uuid, errors.New("invalid UUID format") + return uuid, errors.New("invalid UUID format: hyphens must be at positions 8, 13, 18, 23") }However, this is a minor enhancement and may not align with the vendored package's error handling philosophy.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskrestorepoint_client.go (1)
175-211: Optionally set Content-Type explicitly for JSON POST.
runtime.MarshalAsJSON typically sets Content-Type, but being explicit can guard against future runtime changes.Apply:
req.Raw().Header["Accept"] = []string{"application/json"} + if _, ok := req.Raw().Header["Content-Type"]; !ok { + req.Raw().Header["Content-Type"] = []string{"application/json"} + } if err := runtime.MarshalAsJSON(req, grantAccessData); err != nil { return nil, err }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/temporal/resource.go (3)
136-138: Broadcast while holding the lock (idiomatic cond usage).Not strictly required, but signaling before Unlock avoids subtle timing windows and follows sync.Cond best practices.
Apply this diff:
- er.cond.L.Unlock() - er.cond.Broadcast() + er.cond.Broadcast() + er.cond.L.Unlock()
58-65: Clarify that ShouldRefresh is called under lock; keep it fast/non-blocking.Get invokes ShouldRefresh while holding the internal mutex (Line 95). If ShouldRefresh blocks or re-enters Resource, it can degrade throughput or deadlock.
Add to the ResourceOptions comment:
type ResourceOptions[TResource, TState any] struct { - // ShouldRefresh indicates whether [Resource.Get] should acquire an updated resource despite + // ShouldRefresh indicates whether [Resource.Get] should acquire an updated resource despite // the currently held resource not having expired. [Resource.Get] ignores all errors from // refresh attempts triggered by ShouldRefresh returning true, and doesn't call ShouldRefresh // when the resource has expired (it unconditionally updates expired resources). When // ShouldRefresh is nil, [Resource.Get] refreshes the resource if it will expire within 5 // minutes. + // + // Note: ShouldRefresh is invoked while holding the internal mutex. It must be fast, + // non-blocking, and must not call back into Resource methods to avoid deadlocks. ShouldRefresh ShouldRefresh[TResource, TState] }Also applies to: 95-101
151-154: Document lock expectation for expiringSoon.This helper reads er.expiration; it’s currently called from Get under lock. A brief note helps prevent misuse if extended later.
Apply this diff:
-func (er *Resource[TResource, TState]) expiringSoon(TResource, TState) bool { - // call time.Now() instead of using Get's value so ShouldRefresh doesn't need a time.Time parameter +// expiringSoon must be called with er.cond held by the caller. +func (er *Resource[TResource, TState]) expiringSoon(TResource, TState) bool { + // call time.Now() instead of using Get's value so ShouldRefresh doesn't need a time.Time parameter return er.expiration.Add(-5 * time.Minute).Before(time.Now()) }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_api_version.go (1)
28-33: Use iota for enum-style constants.Slightly clearer and idiomatic; no behavior change.
-const ( - // APIVersionLocationQueryParam indicates a query parameter - APIVersionLocationQueryParam = 0 - // APIVersionLocationHeader indicates a header - APIVersionLocationHeader = 1 -) +const ( + // APIVersionLocationQueryParam indicates a query parameter + APIVersionLocationQueryParam APIVersionLocation = iota + // APIVersionLocationHeader indicates a header + APIVersionLocationHeader +)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/shared.go (1)
124-129: Precompile module version regex once (micro-optimization)Avoid compiling the regex on every call.
Apply this diff:
-func ValidateModVer(moduleVersion string) error { - modVerRegx := regexp.MustCompile(`^v\d+\.\d+\.\d+(?:-[a-zA-Z0-9_.-]+)?$`) - if !modVerRegx.MatchString(moduleVersion) { +func ValidateModVer(moduleVersion string) error { + if !modVerRegx.MatchString(moduleVersion) { return fmt.Errorf("malformed moduleVersion param value %s", moduleVersion) } return nil }Add at package scope:
var modVerRegx = regexp.MustCompile(`^v\d+\.\d+\.\d+(?:-[a-zA-Z0-9_.-]+)?$`)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_register_rp.go (3)
98-100: Avoid surfacing JSON unmarshal error; let default error handling handle 409 bodyReturning the unmarshal error here hides the original service error. Prefer returning the response with nil error so upstream error unmarshalling can proceed.
- if err = runtime.UnmarshalAsJSON(resp, &reqErr); err != nil { - return resp, err - } + if err = runtime.UnmarshalAsJSON(resp, &reqErr); err != nil { + // let caller's error handling surface the service error from resp + return resp, nil + }
155-162: Add small jitter to polling delay to avoid thundering herdSleep uses a fixed delay. Consider adding jitter to spread concurrent retries.
288-291: Consider accepting additional success codes for RegisterSome services return 201/202 for provider registration. If applicable, accept these too.
Would you like me to update this to allow http.StatusOK, http.StatusAccepted, and http.StatusCreated, and verify against Azure RP docs?
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/log.go (1)
82-90: Global logger is unsynchronized; possible data racesSetEvents/SetListener vs Should/Write can race under concurrent use. If this API is used post-init, consider adding minimal sync or documenting init-only usage.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/request.go (1)
145-164: Multipart builder buffers entire payload; consider streaming for large formsSetMultipartFormData builds the full multipart body in memory. For large files/streams, consider an io.Pipe + multipart.Writer to stream without buffering.
I can provide a streaming variant using io.Pipe if desired.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_body_download.go (2)
47-53: Consider HEAD as idempotent for retriesHEAD is idempotent. Include it in the retriable methods set.
- if m := strings.ToUpper(req.Raw().Method); m == http.MethodDelete || m == http.MethodGet || m == http.MethodPut { + if m := strings.ToUpper(req.Raw().Method); m == http.MethodDelete || m == http.MethodGet || m == http.MethodPut || m == http.MethodHead {
31-33: Unbounded body download riskDownloading all response bodies into memory can spike RSS for large payloads. Consider a max-size guard or a per-op opt-out for large responses.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_bearer_token.go (1)
29-36: Typo in comment“retreived” → “retrieved”.
- // mainResource is the resource to be retreived using the tenant specified in the credential + // mainResource is the resource to be retrieved using the tenant specified in the credentiale2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/response_error.go (2)
97-106: Fix regex alternation; avoid matching '|' and broaden code capture
[c|C]is a character class including '|', not alternation. Use case-insensitive and capture non-'<', non-space.func extractErrorCodeXML(body []byte) string { // regular expression is much easier than dealing with the XML parser - rx := regexp.MustCompile(`<(?:\w+:)?[c|C]ode>\s*(\w+)\s*<\/(?:\w+:)?[c|C]ode>`) + rx := regexp.MustCompile(`(?i)<(?:\w+:)?code>\s*([^<\s]+)\s*</(?:\w+:)?code>`) res := rx.FindStringSubmatch(string(body)) if len(res) != 2 { return "" } // first submatch is the entire thing, second one is the captured error code return res[1] }
135-141: Include full URL (with query) in error messagePrinting only scheme/host/path drops query parameters. Use URL.String().
- fmt.Fprintf(msg, "%s %s://%s%s\n", e.RawResponse.Request.Method, e.RawResponse.Request.URL.Scheme, e.RawResponse.Request.URL.Host, e.RawResponse.Request.URL.Path) + fmt.Fprintf(msg, "%s %s\n", e.RawResponse.Request.Method, e.RawResponse.Request.URL.String())e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/loc/loc.go (1)
91-99: Validate updated Location header before overwriting PollURLA malformed Location can poison subsequent polls. Validate before assignment.
- if h := resp.Header.Get(shared.HeaderLocation); h != "" { - p.PollURL = h - } + if h := resp.Header.Get(shared.HeaderLocation); h != "" { + if poller.IsValidURL(h) { + p.PollURL = h + } else { + log.Write(log.EventLRO, fmt.Sprintf("invalid polling URL %s", h)) + } + }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.go (1)
82-85: Doc typo and clarityTypo: “subsets” → “subnets”. Also, the preceding check allows single-part built-ins; update the comment to reflect both forms.
-// ParseResourceType parses the ResourceType from a resource type string (e.g. Microsoft.Network/virtualNetworks/subsets) +// ParseResourceType parses the ResourceType from a resource type string (e.g. Microsoft.Network/virtualNetworks/subnets) // or a resource identifier string. // e.g. /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myRg/providers/Microsoft.Network/virtualNetworks/vnet/subnets/mySubnet)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/fake/fake.go (1)
24-26: Nil-safe check for ApplicableAvoid potential nil deref if a nil response is passed.
func Applicable(resp *http.Response) bool { - return resp.Header.Get(shared.HeaderFakePollerStatus) != "" + if resp == nil { + return false + } + return resp.Header.Get(shared.HeaderFakePollerStatus) != "" }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/request.go (1)
76-102: Large-body copy on NewRequestFromRequest – potential memory spikeWhen req.Body isn’t a ReadSeekCloser, the entire body is buffered into memory. For very large payloads this can be costly. If feasible, document this caveat at call sites or gate with a size limit/streaming fallback. Recognize the pipeline’s need for rewindable bodies; just flagging the perf trade-off.
📜 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 (107)
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/CHANGELOG.md(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/LICENSE.txt(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/README.md(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_identifier.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy/policy.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_identifier.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_type.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/pipeline.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_bearer_token.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_register_rp.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_trace_namespace.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/runtime.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/ci.yml(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/cloud.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/core.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/errors.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/etag.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/exported.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/pipeline.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/request.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/response_error.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/log/log.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/async/async.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/body/body.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/fake/fake.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/loc/loc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/op/op.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/poller.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/util.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/constants.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/shared.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/log.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/policy.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/errors.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pager.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pipeline.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_api_version.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_bearer_token.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_body_download.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_header.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_trace.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_include_response.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_key_credential.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_logging.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_request_id.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_retry.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_sas_credential.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_telemetry.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/poller.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/request.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/response.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_other.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_wasm.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_http_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/progress.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/constants.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/tracing.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/LICENSE.txt(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/diag.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/errorinfo.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/exported/exported.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/log.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/poller/util.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/temporal/resource.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/uuid.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/CHANGELOG.md(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/LICENSE.txt(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/README.md(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/assets.json(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/autorest.md(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/availabilitysets_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/build.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/capacityreservationgroups_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/capacityreservations_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/ci.yml(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/client_factory.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceoperatingsystems_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceroleinstances_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceroles_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudservices_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudservicesupdatedomain_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleries_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleryimages_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleryimageversions_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/dedicatedhostgroups_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/dedicatedhosts_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskaccesses_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskencryptionsets_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskrestorepoint_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/disks_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleries_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryapplications_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryapplicationversions_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryimages_client.go(1 hunks)
⛔ Files not processed due to max files limit (2)
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryimageversions_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/gallerysharingprofile_client.go
✅ Files skipped from review due to trivial changes (13)
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/LICENSE.txt
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/build.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/LICENSE.txt
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/README.md
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/CHANGELOG.md
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/README.md
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/LICENSE.txt
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/CHANGELOG.md
249-249: Link text should be descriptive
(MD059, descriptive-link-text)
582-582: Bare URL used
(MD034, no-bare-urls)
583-583: Bare URL used
(MD034, no-bare-urls)
586-586: Bare URL used
(MD034, no-bare-urls)
587-587: Bare URL used
(MD034, no-bare-urls)
588-588: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (68)
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/etag.go (1)
1-57: LGTM! Well-implemented ETag support per RFC 7232.This vendored code from the Azure SDK for Go correctly implements:
- Strong comparison (both ETags must be strong and equal)
- Weak comparison (strips
W/prefix and compares opaque tags)- Proper bounds checking in
IsWeak()before slicing operations- Standard HTTP conditional request matching via
MatchConditionsThe implementation is safe and follows RFC 7232 section 2.3.2 semantics.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/diag.go (1)
1-51: Verify the relevance of Azure SDK vendoring for Nutanix infrastructure support.This file is part of the Azure SDK for Go vendor modules, but the PR objectives state: "Add infraCluster changes for Nutanix to support NutanixCluster." There appears to be a mismatch between the PR's stated goal (Nutanix infrastructure) and the file being reviewed (Azure SDK).
Please clarify:
- Why are Azure SDK modules being added for Nutanix support?
- Is this file relevant to the Nutanix infrastructure changes, or was it included by mistake?
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/doc.go (1)
1-7: Audit Azure SDK vendoring and dependency usage
- No non-vendor code imports
github.com/Azure/azure-sdk-for-go, yet Azure modules appear in go.mod as indirect dependencies and in the vendor directory. Confirm if these are required for Nutanix support; otherwise, remove them and rungo mod tidy+go mod vendor.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/errorinfo.go (4)
1-7: LGTM!The build constraints and license header are standard and correctly formatted.
24-28: LGTM!The struct follows the standard Go error wrapping pattern with clear documentation.
30-34: LGTM!The Error() method correctly delegates to the embedded error. Potential nil-pointer concerns are addressed by the defensive nil check suggested in the NonRetriableError function.
36-46: LGTM!Both methods follow idiomatic Go error handling patterns:
- The marker method uses a pointer receiver to prevent copying
- The Unwrap() method enables proper error chain traversal with errors.Unwrap() and errors.As()
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/dedicatedhostgroups_client.go (2)
71-74: Confirm 202 handling for CreateOrUpdate responses.Most Azure Compute create/update APIs are long-running operations that return HTTP 202 + Azure-AsyncOperation. This helper only treats 200/201 as success and will surface a false failure if the service responds with 202. Please confirm the swagger for
Microsoft.Compute/hostGroupsguarantees synchronous 200/201 responses; otherwise we need the LRO (BeginCreateOrUpdate) pattern here.
139-142: Verify Delete does not rely on 202 Accepted.Compute delete APIs frequently return 202 (Accepted) for async completion. This code flags anything except 200/204 as an error, so a 202 would bubble up as a failure. Please check the service contract for
DedicatedHostGroups_Delete; if 202 is possible we should switch to the poller-based delete helper that understands the async pattern.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/uuid.go (3)
1-14: LGTM!Build constraints and imports are appropriate for this UUID implementation.
16-22: LGTM!The UUID type definition follows the standard RFC4122 16-byte representation.
40-43: LGTM!The String() method correctly formats the UUID in the canonical RFC4122 format (8-4-4-4-12 hexadecimal digits).
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskrestorepoint_client.go (8)
56-76: Get(): request/response handling looks correct.
- Validates required params, escapes paths, requires 200, unmarshals JSON.
80-110: Robust input validation and safe URL construction.
- All path params checked and PathEscape’d; API version pinned.
132-148: BeginGrantAccess/grantAccess LRO pattern LGTM.
- Accepts 200/202, uses resume token path, FinalStateViaLocation, traces applied.
Also applies to: 154-173
220-241: Pager deref of NextLink: confirm runtime semantics.
The Fetcher dereferences *page.NextLink when page != nil. This is safe if runtime.NewPager only invokes Fetcher when More() is true (which checks NextLink != nil). Please confirm to avoid a potential panic if semantics change.
244-271: ListByRestorePoint request: parameter checks and query string are correct.
282-308: BeginRevokeAccess/revokeAccess paths look correct.
- Proper status handling, tracing, and request building to endGetAccess.
Also applies to: 314-333, 336-367
35-45: moduleName/moduleVersion definitions verified
Declarations exist in constants.go (lines 12–13); no further action needed.
9-21: Vendoring hygiene: verify intentional vendoring
- 141 files vendored under e2e/vendor/github.com/Azure/azure-sdk-for-go
- LICENSE.txt present in sdk/azcore, sdk/internal, and armcompute/v5 directories
- Project Go version is 1.24.0 (meets ≥1.18 requirement)
Confirm that including the full Azure SDK here is intentional for hermetic E2E workflows and aligns with licensing compliance.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_wasm.go (1)
13-15: WASM stub aligns with wasm transport realityReturning
nilhere is the right move so wasm builds continue to compile without trying to install a native dialer that the wasm runtime can’t use anyway.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_request_id.go (1)
24-33: Request ID propagation looks solidWe only synthesize a UUID when the caller didn’t already set
x-ms-client-request-id, and we bail out cleanly if UUID generation ever fails. Nice tight policy implementation.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_include_response.go (1)
17-35: Response capture wiring is reliableWe capture the raw response before returning and hand off to the newer policy helper, so existing callers keep working while being nudged toward the updated API.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/cloud.go (1)
9-44: Cloud configuration surface matches expectationsPublic clouds ship with the right AAD hosts and empty service maps, and the strongly typed
ServiceName/ServiceConfigurationpairing keeps follow-on additions straightforward.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/poller/util.go (1)
64-155: LRO helpers cover the essential edge casesThe JSON helpers surface
ErrNoBody, tolerate missing fields, and return informative errors when types don’t match, which keeps downstream pollers predictable.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/doc.go (1)
1-10: LGTM!The documentation file follows standard Go conventions with appropriate build constraints and license headers.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/doc.go (1)
1-10: LGTM!The documentation file follows standard Go conventions with appropriate build constraints and license headers.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/autorest.md (1)
8-9: Hardcoded commit SHA for reproducibility.The specification URLs reference a specific commit SHA (
92de53a5f1e0e03c94b40475d2135d97148ed014). This is intentional for vendored dependencies to ensure reproducible builds, but be aware that updating to newer API specifications will require changing this commit reference.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_other.go (1)
13-15: LGTM!The platform-specific dialer wrapper correctly delegates to
net.Dialer.DialContextfor non-WASM builds.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/doc.go (1)
1-9: LGTM!The documentation file follows standard Go conventions with appropriate build constraints and license headers.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/ci.yml (1)
1-29: LGTM!The CI workflow configuration follows standard Azure SDK patterns with appropriate branch triggers and path filters.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/ci.yml (1)
1-28: LGTM!The CI workflow configuration follows standard Azure SDK patterns with appropriate branch triggers, path filters, and the
IncludeRelease: trueparameter for enabling release builds.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_http_client.go (1)
18-48: LGTM!The default HTTP client configuration follows best practices with:
- Appropriate timeouts for dial, TLS handshake, and idle connections
- TLS 1.2 minimum version for security
- HTTP/2 support with health check configuration
- Proxy support from environment variables
The TODO comment on Line 38 references a known Go issue (golang/go#59690) which is appropriately documented for future evaluation.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_trace_namespace.go (2)
18-30: LGTM: Policy implementation is correct.The tracing namespace policy correctly:
- Checks for an active tracer before proceeding
- Parses the resource type from the request URL
- Sets the namespace attribute on the current span
- Continues the policy chain regardless of success/failure
The silent error handling (continuing without attribute if parsing fails) is appropriate for a tracing policy.
1-30: Incorrect inconsistency: this PR adds Nutanix infraCluster support under pkg/controllers/infracluster (infracluster_controller.go, nutanix.go), so the Azure SDK vendoring is unrelated.Likely an incorrect or invalid review comment.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/errors.go (1)
1-17: LGTM: Type alias is correctly defined.The public
ResponseErroralias provides a clean API surface for the internal error type. The documentation clearly explains:
- When the error is returned (non-2xx responses)
- How to access it in error chains (
errors.As())- Marshaling behavior (preserves
Error(), omitsRawResponse)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/poller.go (1)
9-24: LGTM: Enum pattern is idiomatic.The
FinalStateViaenum-like type with string constants is the idiomatic Go approach for defining a fixed set of values. Each constant is clearly documented with its purpose for LRO final payload retrieval.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/doc.go (1)
1-264: LGTM: Comprehensive package documentation.The documentation thoroughly covers:
- Policy implementation patterns (stateless via
policyFunc, stateful via structs)- Pipeline construction and request flow
- Pageable operations with
runtime.Pager[T]- Long-running operations with
runtime.Poller[T]- Resume tokens for LRO persistence
- Built-in logging configuration
- Fake servers for testing
The code examples are clear and follow Go best practices.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_telemetry.go (3)
28-55: LGTM: Telemetry policy construction is correct.The constructor properly:
- Handles nil options with safe defaults
- Respects the
Disabledflag for opt-out- Normalizes Application ID (replaces spaces, truncates to 24 chars)
- Extracts the package name from fully qualified module paths
- Builds a well-formatted telemetry string:
[<app_id> ]azsdk-go-<mod>/<ver> <platform_info>
61-71: LGTM: User-Agent header handling is correct.The
Domethod correctly:
- Skips processing when telemetry is disabled (empty
telemetryValue)- Preserves any existing User-Agent header by appending to it
- Sets the final User-Agent header before continuing the pipeline
73-83: LGTM: Platform info initialization is safe.The
platformInfovariable initialization:
- Uses an IIFE pattern to compute the value once at startup
- Safely accesses environment variables (
os.Getenv("OS")) for Windows- Falls back to
runtime.GOOSfor other platforms- Formats as
(<go_version>; <os>)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/runtime.go (2)
11-24: LGTM: Cloud configurations are correct.The
initfunction properly configuresResourceManagerservice endpoints for three Azure clouds:
- Azure China: Uses
chinacloudapi.cndomain- Azure Government: Uses
usgovcloudapi.netdomain- Azure Public: Uses standard
azure.comandwindows.netdomainsThe Audience and Endpoint URLs follow Azure's documented conventions.
1-2: Verify build constraint alignment:e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/runtime.gouses//go:build go1.16, but 120 other Azure SDK files targetgo1.18. Align this togo1.18or document why a lower version is required.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_identifier.go (1)
13-23: LGTM: Public facade is correctly implemented.The file provides a clean public API surface by:
- Exposing
RootResourceIDconstant for the tenant root- Aliasing
ResourceIDtype from the internal package- Wrapping
ParseResourceIDfunction with appropriate documentationThis facade pattern keeps internal implementation details hidden while providing a stable public API.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/errors.go (1)
15-27: LGTM: Error construction helpers are correct.Both functions provide convenient wrappers for creating
ResponseErrorinstances:
NewResponseError: Automatically extracts error code fromx-ms-error-codeheader or response bodyNewResponseErrorWithErrorCode: Accepts an explicit error code for non-standard locationsThe documentation clearly explains when to use each variant.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/log.go (1)
14-37: LGTM: public log facade and event constants.Type aliasing and constants are consistent; wrappers are minimal and correct.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/log/log.go (1)
17-50: LGTM: internal logging aggregator is consistent.Constants and delegation align the public/internal surfaces without behavior changes.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_identifier.go (1)
22-239: LGTM: ResourceID parsing and stringification.Covers providers/subscriptions/resource groups/locations; caching and recursion are correct.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/pipeline.go (1)
58-66: LGTM: transport policy placement is correctTransport appended last and Do() wiring is straightforward.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleries_client.go (1)
54-74: LGTM: request/response flow follows ARM conventionsParameter validation, pipeline execution, status validation, and JSON unmarshal look correct.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_bearer_token.go (1)
99-102: LGTM: delegates to inner runtime bearer policyDo() simply delegates; auxiliary auth handled in OnRequest as expected.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_retry.go (2)
92-101: Retry policy looks goodDefaults, jittered backoff, TryTimeout handling, and ShouldRetry override are implemented correctly.
208-210: No changes needed: Drain is nil-safe The implementation in response.go guards againstresp == nilandresp.Body == nil.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_trace.go (1)
46-95: LGTM: HTTP tracing policySolid attribute set, sanitized URL, error handling via *url.Error unwrap, and span suppression logic.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleryimages_client.go (1)
56-76: Get flow LGTMRequest build, tracing, error mapping to ResponseError, and JSON unmarshal look correct.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/client.go (1)
55-57: TracingProvider.NewTracer is safe with zero-value
TracingProvideris a struct defaulting to a no-op tracer (newTracerFn=nil);NewTracerchecks this and never panics. No guard needed.Likely an incorrect or invalid review comment.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.go (1)
14-25: No missing references:builtInResourceNamespace,splitStringAndOmitEmpty, andParseResourceIDare defined in resource_identifier.go; imports are correct.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleries_client.go (1)
35-45: moduleName and moduleVersion are defined: found in constants.go (lines 12–13), no action needed.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/capacityreservationgroups_client.go (1)
72-75: Consider 202 Accepted/LRO handling (verify API semantics)Create/Update/Delete accept only 200/201/204. If these operations are long-running, the service may return 202 with polling headers. Please confirm the 202/LRO path isn't required for this resource/API version, or add LRO handling.
If needed, I can help wire up an LRO poller to handle 202 + Location/Async headers.
Also applies to: 142-147, 393-396
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleryimageversions_client.go (1)
47-79: LGTM – request/response and tracing follow SDK conventionsGet flow validates inputs, checks 200, and unmarshals correctly. Pager construction matches established pattern.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/core.go (1)
140-151: Remove unnecessary nil-guard for TracingProvider
TracingProvideris a value‐type struct that defaults to a no-op provider. Its zero‐value isn’t nil, and callingNewTraceron it safely returns a no-opTracerwithout panic.Likely an incorrect or invalid review comment.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/policy.go (1)
31-70: Public ClientOptions surface looks correctFields and defaults align with azcore conventions; context helpers and aliases are consistent.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pager.go (1)
21-41: Generic pager design LGTMHandler contract, first-page handling, and tracing integration are sound.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskencryptionsets_client.go (1)
93-96: Allow 201 Created for CreateOrUpdate (PUT)
Add http.StatusCreated to the allowed status codes to avoid classifying a 201 response as an error.- if !runtime.HasStatusCode(httpResp, http.StatusOK, http.StatusAccepted) { + if !runtime.HasStatusCode(httpResp, http.StatusOK, http.StatusCreated, http.StatusAccepted) { err = runtime.NewResponseError(httpResp) return nil, err }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceoperatingsystems_client.go (1)
35-45: LGTM on client construction, getters, and pagersPatterns, validation, tracing, and paging logic look correct and consistent with ARM SDK conventions.
Also applies to: 56-76, 187-208, 249-270
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/disks_client.go (1)
35-46: Disks client LROs, getters, and pagers look solidStatus handling, poller usage, and parameter validations are correct. No issues spotted.
Also applies to: 59-74, 129-153, 214-234, 353-378, 413-434, 544-569
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/poller.go (1)
72-131: Poller runtime implementation looks correctConstruction, resume, polling loop, and result caching are well-implemented and match azcore’s design.
Also applies to: 231-289, 295-317, 323-362, 368-377
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/capacityreservations_client.go (1)
59-74: CapacityReservations client surfaces are correctLRO handling, status codes, paging, and validations look good. No issues found.
Also applies to: 82-101, 147-162, 170-189, 230-250, 302-323, 370-385, 391-410
...or/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_identifier.go
Outdated
Show resolved
Hide resolved
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.go
Outdated
Show resolved
Hide resolved
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy/policy.go
Outdated
Show resolved
Hide resolved
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_type.go
Outdated
Show resolved
Hide resolved
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/exported.go
Outdated
Show resolved
Hide resolved
...om/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/dedicatedhosts_client.go
Outdated
Show resolved
Hide resolved
...om/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/dedicatedhosts_client.go
Outdated
Show resolved
Hide resolved
...om/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/dedicatedhosts_client.go
Outdated
Show resolved
Hide resolved
...hub.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleries_client.go
Outdated
Show resolved
Hide resolved
...ure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryapplications_client.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 21
♻️ Duplicate comments (33)
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/diag.go (2)
16-16: Typo already flagged in previous review.The typo "funtion" (should be "function") on line 16 was already identified in a previous review. Since this is vendored code from the Azure SDK, avoid modifying it unless absolutely necessary.
29-29: Typo already flagged in previous review.The typo "funtion" (should be "function") on line 29 was already identified in a previous review. Since this is vendored code from the Azure SDK, avoid modifying it unless absolutely necessary.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/response.go (1)
44-60: BOM removal bug still present (duplicate of previous review).The critical bug identified in the previous review remains unfixed. The
payloadvariable captured at lines 44 and 65 contains the original bytes including any BOM. WhenremoveBOMis called at lines 52 and 73, it modifies the cached bytes insideresp, but the localpayloadvariable still holds the original BOM-prefixed data. The subsequentjson.Unmarshalandxml.Unmarshaloperations use this stale local variable, causing unmarshaling to fail on BOM-prefixed responses.Since this is vendored Azure SDK code, consider:
- Updating to a newer version of
github.com/Azure/azure-sdk-for-go/sdk/azcorethat fixes this issue, or- Applying a local patch to fix the bug, or
- At minimum, documenting this known critical issue if the vendored version cannot be changed immediately.
To fix locally, call
removeBOMbefore reading the payload, or re-read after removal:func UnmarshalAsJSON(resp *http.Response, v any) error { + err := removeBOM(resp) + if err != nil { + return err + } payload, err := Payload(resp) if err != nil { return err } // TODO: verify early exit is correct if len(payload) == 0 { return nil } - err = removeBOM(resp) - if err != nil { - return err - } err = json.Unmarshal(payload, v) if err != nil { err = fmt.Errorf("unmarshalling type %T: %s", v, err) } return err } func UnmarshalAsXML(resp *http.Response, v any) error { + err := removeBOM(resp) + if err != nil { + return err + } payload, err := Payload(resp) if err != nil { return err } // TODO: verify early exit is correct if len(payload) == 0 { return nil } - err = removeBOM(resp) - if err != nil { - return err - } err = xml.Unmarshal(payload, v) if err != nil { err = fmt.Errorf("unmarshalling type %T: %s", v, err) } return err }Also applies to: 65-81
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/log.go (1)
39-44: Typo already flagged in previous review.The typo "writen" → "written" on line 40 was already reported. Note that this is a vendored file from the Azure SDK, so fixes should ideally be contributed upstream to Microsoft's repository.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/log.go (1)
22-26: Typo already flagged in previous review.The typo "writen" → "written" on line 23 was already reported. This is vendored Azure SDK code, so changes should be contributed upstream.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_key_credential.go (1)
61-63: Overwrite the key header instead of appending (retry-safe).Using Header.Add accumulates duplicate values on retries; breaks auth. Use Header.Set.
- req.Raw().Header.Add(k.header, val) + req.Raw().Header.Set(k.header, val)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/availabilitysets_client.go (1)
70-76: Accept 201 Created for PUT CreateOrUpdate.Create-or-update may return 201 for new resources. Treat 201 as success.
- if !runtime.HasStatusCode(httpResp, http.StatusOK) { + if !runtime.HasStatusCode(httpResp, http.StatusOK, http.StatusCreated) { err = runtime.NewResponseError(httpResp) return AvailabilitySetsClientCreateOrUpdateResponse{}, err }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_sas_credential.go (1)
52-53: Use Header.Set to prevent duplicate SAS headers on retries.Retries reuse the request; Add stacks values.
- req.Raw().Header.Add(k.header, exported.SASCredentialGet(k.cred)) + req.Raw().Header.Set(k.header, exported.SASCredentialGet(k.cred))e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_identifier.go (1)
69-71: Capitalize “ID” consistently in error message.- return nil, fmt.Errorf("invalid resource ID: resource id '%s' must start with '/'", id) + return nil, fmt.Errorf("invalid resource ID: resource ID '%s' must start with '/'", id)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_logging.go (1)
137-146: Don’t overwrite/return pipeline error due to response-body logging.Keep logging best‑effort and preserve the original err.
- } else if p.includeBody { - err = writeRespBody(response, b) + } else if p.includeBody { + _ = writeRespBody(response, b) // best-effort; ignore logging errorse2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_type.go (1)
35-38: Doc typo (“subsets” → “subnets”) and stray trailing parenthesisCorrect example text.
-// ParseResourceType parses the ResourceType from a resource type string (e.g. Microsoft.Network/virtualNetworks/subsets) +// ParseResourceType parses the ResourceType from a resource type string (e.g. Microsoft.Network/virtualNetworks/subnets) // or a resource identifier string. -// e.g. /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myRg/providers/Microsoft.Network/virtualNetworks/vnet/subnets/mySubnet) +// e.g. /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myRg/providers/Microsoft.Network/virtualNetworks/vnet/subnets/mySubnete2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy/policy.go (1)
73-86: Clone still aliases AuxiliaryTenants; deep-copy to avoid cross-mutationMutating the clone’s AuxiliaryTenants will mutate the source. Deep-copy it.
Apply this diff:
func (o *ClientOptions) Clone() *ClientOptions { if o == nil { return nil } copiedOptions := *o copiedOptions.Cloud.Services = copyMap(copiedOptions.Cloud.Services) copiedOptions.Logging.AllowedHeaders = copyArray(copiedOptions.Logging.AllowedHeaders) copiedOptions.Logging.AllowedQueryParams = copyArray(copiedOptions.Logging.AllowedQueryParams) copiedOptions.Retry.StatusCodes = copyArray(copiedOptions.Retry.StatusCodes) copiedOptions.PerRetryPolicies = copyArray(copiedOptions.PerRetryPolicies) copiedOptions.PerCallPolicies = copyArray(copiedOptions.PerCallPolicies) + // deep-copy AuxiliaryTenants to avoid aliasing with the original + copiedOptions.AuxiliaryTenants = copyArray(o.AuxiliaryTenants) return &copiedOptions }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pipeline.go (1)
59-70: Fix slice merge bug for AllowedHeaders/AllowedQueryParametersPreallocates with length then appends, creating empty entries and extra allocs. Preallocate with length 0 and capacity, then append.
- if len(plOpts.AllowedHeaders) > 0 { - headers := make([]string, len(plOpts.AllowedHeaders)+len(cp.Logging.AllowedHeaders)) - copy(headers, plOpts.AllowedHeaders) - headers = append(headers, cp.Logging.AllowedHeaders...) - cp.Logging.AllowedHeaders = headers - } + if len(plOpts.AllowedHeaders) > 0 { + headers := make([]string, 0, len(plOpts.AllowedHeaders)+len(cp.Logging.AllowedHeaders)) + headers = append(headers, plOpts.AllowedHeaders...) + headers = append(headers, cp.Logging.AllowedHeaders...) + cp.Logging.AllowedHeaders = headers + } - if len(plOpts.AllowedQueryParameters) > 0 { - qp := make([]string, len(plOpts.AllowedQueryParameters)+len(cp.Logging.AllowedQueryParams)) - copy(qp, plOpts.AllowedQueryParameters) - qp = append(qp, cp.Logging.AllowedQueryParams...) - cp.Logging.AllowedQueryParams = qp - } + if len(plOpts.AllowedQueryParameters) > 0 { + qp := make([]string, 0, len(plOpts.AllowedQueryParameters)+len(cp.Logging.AllowedQueryParams)) + qp = append(qp, plOpts.AllowedQueryParameters...) + qp = append(qp, cp.Logging.AllowedQueryParams...) + cp.Logging.AllowedQueryParams = qp + }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/body/body.go (2)
26-28: Fix typo in comment: “absense” → “absence”.- // we can't check for absense of headers due to some misbehaving services + // we can't check for absence of headers due to some misbehaving services
81-90: Fix typos in comments (“absense” → “absence”).- // absense of provisioning state is ok for a 201, means the operation is in progress + // absence of provisioning state is ok for a 201, means the operation is in progress ... - // for a 200, absense of provisioning state indicates success + // for a 200, absence of provisioning state indicates successe2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/request.go (2)
31-34: Fix typo in comment: “usesd” → “used”.-// Base64Encoding is usesd to specify which base-64 encoder/decoder to use when +// Base64Encoding is used to specify which base-64 encoder/decoder to use when
137-139: Fix typo in comment: “inclue” → “include”.- // inclue the XML header as some services require it + // include the XML header as some services require ite2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/response_error.go (1)
23-27: Guard against nil http.Response in NewResponseError to prevent panic.Add a fast nil check before accessing headers.
func NewResponseError(resp *http.Response) error { + if resp == nil { + return fmt.Errorf("nil http.Response") + } // prefer the error code in the response header if ec := resp.Header.Get(shared.HeaderXMSErrorCode); ec != "" { return NewResponseErrorWithErrorCode(resp, ec) }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_bearer_token.go (1)
202-205: Redact CAE claims from error to avoid PII leakageDon’t echo raw claims in error messages; it can expose sensitive data.
- err = errorinfo.NonRetriableError(errors.New("authentication challenge contains invalid claims: " + claims)) + err = errorinfo.NonRetriableError(errors.New("authentication challenge contains invalid claims"))e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleries_client.go (1)
431-435: Accept 202 for Update LRO initiationPATCH Update can return 202 to start an LRO. Only accepting 200 yields false errors.
- if !runtime.HasStatusCode(httpResp, http.StatusOK) { + if !runtime.HasStatusCode(httpResp, http.StatusOK, http.StatusAccepted) { err = runtime.NewResponseError(httpResp) return nil, err }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/async/async.go (1)
123-129: Nil deref risk in Result when resumed before first Poll()Guard p.resp to avoid panic for resumed pollers that haven’t polled yet.
func (p *Poller[T]) Result(ctx context.Context, out *T) error { - if p.resp.StatusCode == http.StatusNoContent { + if p.resp == nil { + return errors.New("poller has no response; call Poll until Done() before Result") + } + if p.resp.StatusCode == http.StatusNoContent { return nil } else if poller.Failed(p.CurState) { return exported.NewResponseError(p.resp) }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/exported.go (1)
90-99: Prevent panic when trimming quotes; remove redundant conversionSafe-guard slicing when removing quotes and avoid redundant string conversion.
func DecodeByteArray(s string, v *[]byte, format Base64Encoding) error { if len(s) == 0 { return nil } - payload := string(s) - if payload[0] == '"' { - // remove surrounding quotes - payload = payload[1 : len(payload)-1] - } + payload := s + // remove surrounding quotes when present + if len(payload) >= 2 && payload[0] == '"' && payload[len(payload)-1] == '"' { + payload = payload[1 : len(payload)-1] + }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.go (1)
112-114: Guard against empty Types to avoid paniclastType() panics when Types is empty. Return a safe default instead.
func (t ResourceType) lastType() string { - return t.Types[len(t.Types)-1] + if len(t.Types) == 0 { + return "" + } + return t.Types[len(t.Types)-1] }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/util.go (1)
143-157: Avoid closing resp.Body here; callers may need the body (close previous bodies instead)Closing in PollHelper will break pollers that later unmarshal from the last poll response (e.g., Location poller’s Result). Prefer closing the previously held response body in each poller before overwriting p.resp (see suggested diffs in op/loc/fake).
Can you confirm consumers never rely on reading the last poll response body? If not, we can revisit moving the close here.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/op/op.go (1)
117-131: Guard Result when no response is available to avoid panicResult dereferences p.resp; if called before any Poll, this will panic. Add a defensive nil check.
func (p *Poller[T]) Result(ctx context.Context, out *T) error { + if p.resp == nil { + return errors.New("poller has no response; call Poll until Done() before Result") + } var req *exported.Request var err errore2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/fake/fake.go (1)
120-128: Guard Result when no response is availableResult dereferences p.resp and will panic if called before Poll sets it.
func (p *Poller[T]) Result(ctx context.Context, out *T) error { - if p.resp.StatusCode == http.StatusNoContent { + if p.resp == nil { + return errors.New("no polling response available; call Poll until Done() before Result") + } + if p.resp.StatusCode == http.StatusNoContent { return nil } else if poller.Failed(p.FakeStatus) { return exported.NewResponseError(p.resp) } return pollers.ResultHelper(p.resp, poller.Failed(p.FakeStatus), "", out) }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryapplications_client.go (1)
396-396: Past review comment still applies: Include HTTP 202 for LRO update responsesThis issue was previously flagged. The
update()helper currently only acceptshttp.StatusOK(200), but long-running update operations may returnhttp.StatusAccepted(202). This is inconsistent withcreateOrUpdate()at line 94, which correctly accepts 200, 201, and 202.The fix recommended in the previous review remains valid:
- if !runtime.HasStatusCode(httpResp, http.StatusOK) { + if !runtime.HasStatusCode(httpResp, http.StatusOK, http.StatusAccepted) { err = runtime.NewResponseError(httpResp) return nil, err }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceroleinstances_client.go (1)
6-7: Avoid manual edits to auto-generated vendor code.As with the previous file, this is generated code in the vendor tree. Manual modifications should be avoided.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudservices_client.go (1)
6-7: Manual edits to generated vendor code.This file is also auto-generated and should not be manually modified.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/dedicatedhosts_client.go (4)
6-7: Manual edits to generated vendor code.This file is also auto-generated and should not be manually modified.
92-95: CreateOrUpdate LRO still missing 202 Accepted.This issue was flagged in a previous review but remains unresolved. The
createOrUpdatemethod only accepts 200 and 201, breaking LRO flows that return 202.Based on learnings from past review comments.
560-564: Restart LRO still missing 202 Accepted.This issue was flagged in a previous review but remains unresolved. The
restartmethod only accepts 200, breaking LRO flows that return 202.Based on learnings from past review comments.
642-645: Update LRO still missing 202 Accepted.This issue was flagged in a previous review but remains unresolved. The
updatemethod only accepts 200, breaking LRO flows that return 202.Based on learnings from past review comments.
🧹 Nitpick comments (15)
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/autorest.md (1)
3-3: markdownlint MD034: bare URL in vendored file — prefer excluding vendor from lint.Given this is upstream‑vendored content, don’t patch it locally. Exclude vendor paths from markdownlint instead (e.g., add e2e/vendor/** to .markdownlintignore). If you must quiet it, minimally wrap the URL with angle brackets.
Apply minimal inline fix (only if you choose to modify vendored content):
-> see https://aka.ms/autorest +> see <https://aka.ms/autorest>Or exclude vendor from markdownlint (in .markdownlintignore):
e2e/vendor/**e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/core.go (1)
48-50: ClarifyNullValuetype expectations.Document that T must be a pointer, map, or slice to avoid misuse.
// NullValue is used to send an explicit 'null' within a request. -// This is typically used in JSON-MERGE-PATCH operations to delete a value. +// This is typically used in JSON-MERGE-PATCH operations to delete a value. +// Note: T should be a pointer, map, or slice type. func NullValue[T any]() T {e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/CHANGELOG.md (1)
249-249: Optional: Address markdown linting issues.The markdown linter flagged a few minor issues:
- Line 249: Link text could be more descriptive
- Lines 582-588: Bare URLs should be formatted as proper links
These are optional documentation quality improvements for vendored changelog content.
Also applies to: 582-588
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/pipeline.go (1)
38-41: Preallocate slice capacity for the two appended per-retry policies.Avoids an extra allocation when appending 2 items.
- perRetry := make([]azpolicy.Policy, len(plOpts.PerRetry), len(plOpts.PerRetry)+1) + perRetry := make([]azpolicy.Policy, len(plOpts.PerRetry), len(plOpts.PerRetry)+2)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_sas_credential.go (1)
43-44: Fix typos in the interface name and verb in comment.-// Do implementes the Do method on the [policy.Polilcy] interface. +// Do implements the Do method on the [policy.Policy] interface.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_logging.go (1)
194-201: Log all header values (not just the first), while preserving redaction.Improves fidelity for multi-valued headers.
- // don't use Get() as it will canonicalize k which might cause a mismatch - value := header[k][0] - // redact all header values not in the allow-list - if _, ok := p.allowedHeaders[strings.ToLower(k)]; !ok { - value = redactedValue - } - fmt.Fprintf(b, " %s: %+v\n", k, value) + // redact all header values not in the allow-list + if _, ok := p.allowedHeaders[strings.ToLower(k)]; !ok { + fmt.Fprintf(b, " %s: %s\n", k, redactedValue) + continue + } + // don't use Get() as it will canonicalize k which might cause a mismatch + values := header[k] + if len(values) == 0 { + fmt.Fprintf(b, " %s: \n", k) + } else if len(values) == 1 { + fmt.Fprintf(b, " %s: %+v\n", k, values[0]) + } else { + fmt.Fprintf(b, " %s: %+v\n", k, strings.Join(values, ",")) + }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy/policy.go (2)
88-98: Pre-size copyMap for fewer allocationsUse capacity hint.
-func copyMap[K comparable, V any](src map[K]V) map[K]V { +func copyMap[K comparable, V any](src map[K]V) map[K]V { if src == nil { return nil } - copiedMap := make(map[K]V) + copiedMap := make(map[K]V, len(src)) for k, v := range src { copiedMap[k] = v } return copiedMap }
49-51: Fix doc typos and wordingMinor comment/grammar fixes.
-// The default valule is 5 minutes. +// The default value is 5 minutes.-// Clone return a deep copy of the current options. +// Clone returns a deep copy of the current options.-// copyMap return a new array with all the elements in the src array +// copyArray returns a new slice with all the elements in the src sliceAlso applies to: 73-74, 100-104
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_bearer_token.go (1)
45-49: Preserve auxiliary token order (map iteration is nondeterministic)Iterating a map randomizes header token order. Preserve the order from options.AuxiliaryTenants for stability.
type BearerTokenPolicy struct { - auxResources map[string]*temporal.Resource[azcore.AccessToken, acquiringResourceState] + auxResources map[string]*temporal.Resource[azcore.AccessToken, acquiringResourceState] + auxOrder []string btp *azruntime.BearerTokenPolicy cred azcore.TokenCredential scopes []string }p := &BearerTokenPolicy{cred: cred} p.auxResources = make(map[string]*temporal.Resource[azcore.AccessToken, acquiringResourceState], len(opts.AuxiliaryTenants)) for _, t := range opts.AuxiliaryTenants { p.auxResources[t] = temporal.NewResource(acquireAuxToken) } + p.auxOrder = make([]string, len(opts.AuxiliaryTenants)) + copy(p.auxOrder, opts.AuxiliaryTenants) p.scopes = make([]string, len(opts.Scopes)) copy(p.scopes, opts.Scopes)- for tenant, er := range b.auxResources { - as.tenant = tenant - auxTk, err := er.Get(as) + for _, tenant := range b.auxOrder { + er := b.auxResources[tenant] + as.tenant = tenant + auxTk, err := er.Get(as) if err != nil { return err } auxTokens = append(auxTokens, fmt.Sprintf("%s%s", shared.BearerTokenPrefix, auxTk.Token)) }Also applies to: 59-65, 86-95
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceroles_client.go (1)
173-180: Add status code check in listHandleResponse (align with Get).Guard 200 before unmarshalling to avoid surfacing JSON errors for non-OK responses.
func (client *CloudServiceRolesClient) listHandleResponse(resp *http.Response) (CloudServiceRolesClientListResponse, error) { - result := CloudServiceRolesClientListResponse{} + if !runtime.HasStatusCode(resp, http.StatusOK) { + return CloudServiceRolesClientListResponse{}, runtime.NewResponseError(resp) + } + result := CloudServiceRolesClientListResponse{} if err := runtime.UnmarshalAsJSON(resp, &result.CloudServiceRoleListResult); err != nil { return CloudServiceRolesClientListResponse{}, err } return result, nil }e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_register_rp.go (1)
23-23: Alias imported azcore/runtime to avoid package-name ambiguity.Importing azcore/runtime as runtime within package runtime is confusing. Prefer an alias (e.g., azruntime) for readability.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/op/op.go (1)
95-110: Close previous polling response body before overwriting to prevent leaksEach Poll overwrites p.resp without closing the prior body, leaking connections over long-running operations.
func (p *Poller[T]) Poll(ctx context.Context) (*http.Response, error) { err := pollers.PollHelper(ctx, p.OpLocURL, p.pl, func(resp *http.Response) (string, error) { if !poller.StatusCodeValid(resp) { - p.resp = resp + if p.resp != nil && p.resp.Body != nil { + _ = p.resp.Body.Close() + } + p.resp = resp return "", exported.NewResponseError(resp) } state, err := poller.GetStatus(resp) if err != nil { return "", err } else if state == "" { return "", errors.New("the response did not contain a status") } - p.resp = resp + if p.resp != nil && p.resp.Body != nil { + _ = p.resp.Body.Close() + } + p.resp = resp p.CurState = state return p.CurState, nil })e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/loc/loc.go (1)
88-103: Close previous polling response body before overwriting to prevent leaksAvoid accumulating open bodies across Poll calls.
func (p *Poller[T]) Poll(ctx context.Context) (*http.Response, error) { err := pollers.PollHelper(ctx, p.PollURL, p.pl, func(resp *http.Response) (string, error) { // location polling can return an updated polling URL if h := resp.Header.Get(shared.HeaderLocation); h != "" { p.PollURL = h } @@ - provState, _ := poller.GetProvisioningState(resp) - p.resp = resp + provState, _ := poller.GetProvisioningState(resp) + if p.resp != nil && p.resp.Body != nil { + _ = p.resp.Body.Close() + } + p.resp = respe2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.go (1)
89-97: Minor doc/logic mismatchComment says “at least a namespace and type name”, but len(parts)==1 is accepted (built-ins). Consider updating the comment for clarity.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/fake/fake.go (1)
98-113: Close previous polling response body before overwriting to prevent leaksFree connections as the LRO progresses.
func (p *Poller[T]) Poll(ctx context.Context) (*http.Response, error) { ctx = context.WithValue(ctx, shared.CtxAPINameKey{}, p.APIName) err := pollers.PollHelper(ctx, p.FakeURL, p.pl, func(resp *http.Response) (string, error) { if !poller.StatusCodeValid(resp) { - p.resp = resp + if p.resp != nil && p.resp.Body != nil { + _ = p.resp.Body.Close() + } + p.resp = resp return "", exported.NewResponseError(resp) } @@ - p.resp = resp + if p.resp != nil && p.resp.Body != nil { + _ = p.resp.Body.Close() + } + p.resp = resp p.FakeStatus = fakeStatus return p.FakeStatus, 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 selected for processing (107)
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/CHANGELOG.md(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/LICENSE.txt(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/README.md(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_identifier.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy/policy.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_identifier.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_type.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/pipeline.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_bearer_token.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_register_rp.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_trace_namespace.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/runtime.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/ci.yml(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/cloud.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/core.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/errors.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/etag.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/exported.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/pipeline.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/request.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/response_error.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/log/log.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/async/async.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/body/body.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/fake/fake.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/loc/loc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/op/op.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/poller.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/util.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/constants.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/shared.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/log.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/policy.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/errors.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pager.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pipeline.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_api_version.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_bearer_token.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_body_download.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_header.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_trace.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_include_response.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_key_credential.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_logging.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_request_id.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_retry.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_sas_credential.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_telemetry.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/poller.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/request.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/response.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_other.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_wasm.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_http_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/progress.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/constants.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/tracing.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/LICENSE.txt(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/diag.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/errorinfo.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/exported/exported.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/log.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/poller/util.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/temporal/resource.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/doc.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/uuid.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/CHANGELOG.md(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/LICENSE.txt(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/README.md(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/assets.json(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/autorest.md(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/availabilitysets_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/build.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/capacityreservationgroups_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/capacityreservations_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/ci.yml(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/client_factory.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceoperatingsystems_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceroleinstances_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceroles_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudservices_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudservicesupdatedomain_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleries_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleryimages_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleryimageversions_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/dedicatedhostgroups_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/dedicatedhosts_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskaccesses_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskencryptionsets_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskrestorepoint_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/disks_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleries_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryapplications_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryapplicationversions_client.go(1 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryimages_client.go(1 hunks)
⛔ Files not processed due to max files limit (2)
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryimageversions_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/gallerysharingprofile_client.go
✅ Files skipped from review due to trivial changes (1)
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/doc.go
🚧 Files skipped from review as they are similar to previous changes (33)
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/LICENSE.txt
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/ci.yml
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/README.md
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/CHANGELOG.md
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/LICENSE.txt
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_other.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/runtime.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/pipeline.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/cloud.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_body_download.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/uuid.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_retry.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/constants.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/errors.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/LICENSE.txt
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/errors.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleries_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/poller/util.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/ci.yml
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/policy.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/errorinfo.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/temporal/resource.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_identifier.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/log/log.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/build.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/constants.go
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/CHANGELOG.md
249-249: Link text should be descriptive
(MD059, descriptive-link-text)
582-582: Bare URL used
(MD034, no-bare-urls)
583-583: Bare URL used
(MD034, no-bare-urls)
586-586: Bare URL used
(MD034, no-bare-urls)
587-587: Bare URL used
(MD034, no-bare-urls)
588-588: Bare URL used
(MD034, no-bare-urls)
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/README.md
14-14: Link text should be descriptive
(MD059, descriptive-link-text)
68-68: Bare URL used
(MD034, no-bare-urls)
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/autorest.md
3-3: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (32)
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/autorest.md (1)
5-13: AutoRest config looks correct and reproducible.azure-arm enabled, specs pinned by commit SHA, license header set, module-version 5.7.0, and tag package-2024-03-01 are consistent with vendored ARM Compute v5.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/doc.go (1)
1-7: Critical inconsistency: Azure SDK additions in a Nutanix-focused PR.The PR title and description indicate this PR is intended to "Add infra support for Nutanix" to support NutanixCluster. However, this file (and per the AI summary, the entire changeset) appears to add Azure SDK for Go components, which are unrelated to Nutanix infrastructure.
Please verify:
- Are these Azure SDK vendor files required as dependencies for the Nutanix implementation?
- Was the wrong branch/changeset pushed to this PR?
- Is the PR description outdated or incorrect?
The file content itself is standard (build constraint, copyright, package declaration), but the presence of Azure SDK files in a Nutanix-focused PR requires clarification.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskrestorepoint_client.go (1)
1-367: Inconsistency: Azure SDK in a Nutanix infrastructure PR.This file is an auto-generated Azure SDK client for managing Azure disk restore points, but the PR objectives state: "Add infraCluster changes for Nutanix to support NutanixCluster." There is no apparent connection between Nutanix infrastructure support and Azure SDK dependencies.
Please verify:
- Is this Azure SDK dependency actually required for Nutanix support? If so, please explain the relationship.
- Was this file included in the PR by mistake?
- Should this vendored dependency be removed?
Note: This is auto-generated code (line 6: "Code generated by Microsoft (R) AutoRest Code Generator. DO NOT EDIT."), so manual code review is not applicable, but the inclusion of this dependency in a Nutanix-focused PR requires justification.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/diag.go (3)
1-13: LGTM!Build constraints, copyright, package declaration, and imports are all correct and appropriate for the diagnostic functionality.
19-26: LGTM!The
Callerfunction implementation is correct:
- Properly adjusts
skipFrames + 1to account for itself- Handles failure cases by returning an empty string
- Produces clear diagnostic output
32-51: LGTM!The
StackTracefunction implementation is correct:
- Properly handles the case when no frames are captured (line 34-36)
- Uses
strings.Builderfor efficient string concatenation- Correctly iterates through frames using the
moreflag to determine when to stop- Produces well-formatted diagnostic output
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/doc.go (1)
1-10: No action required on vendored Azure SDK file. This doc.go is a standard, non-functional part of the e2e/vendor dependency tree.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/doc.go (2)
1-2: Build constraints look correct.Dual //go:build and // +build for go1.18 are in the right order. No issues.
4-7: Verify Azure SDK vendoring necessity and upstream alignment
- e2e/go.mod targets Go 1.24 (≥1.18).
- LICENSE files for azcore, sdk/internal, armcompute (v5.7.0) are present under e2e/vendor.
- Confirm vendored azure-sdk-for-go packages (v1.18.0, v1.11.1, v5.7.0) are intentional for this Nutanix PR and match upstream.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/exported/exported.go (1)
1-129: Clarify Azure SDK vendoring in Nutanix support PR.This PR adds vendored Azure SDK code (
e2e/vendor/github.com/Azure/azure-sdk-for-go/...) but its description targets Nutanix infrastructure—please confirm whether these Azure SDK updates are intentional and required.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/etag.go (1)
1-57: Vendored Azure SDK ETag implementation requires no changes.The ETag code follows RFC 7232 and is part of the existing e2e vendored dependencies.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskaccesses_client.go (1)
1-846: Vendor mismatch — Azure SDK vendored file present in Nutanix PR; confirm intent or remove.The PR includes Nutanix provider code under vendor/ (e.g. vendor/github.com/nutanix-cloud-native/...), but the reviewed file (e2e/vendor/github.com/Azure/azure-sdk-for-go/.../diskaccesses_client.go) is autogenerated Azure SDK vendored code unrelated to Nutanix changes. Either:
- Document in the PR why Azure SDK was vendored (show go.mod/go.sum diffs and rationale, e.g., required for e2e tests), or
- Remove/split unrelated vendor changes so the Nutanix feature PR only contains Nutanix-relevant edits.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/capacityreservationgroups_client.go (1)
1-437: Confirm vendored Azure SDK belongs in this Nutanix-focused PR.The file e2e/vendor/github.com/Azure/azure-sdk-for-go/... is autogenerated vendored Azure SDK code; the repo already contains Azure platform integrations (pkg/controllers/infracluster/azure.go, e2e/e2e_test.go, webhook Azure validation, etc.), so this looks like a vendor refresh rather than a code change you should edit. Confirm whether the vendor update was intentional for this PR; if unintentional, remove the vendor changes from the PR. Do not modify autogenerated vendor files.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/progress.go (2)
62-70: Verify nil pointer safety in progress.Seek
The Seek implementation calls p.rsc.Seek without checking for nil, but NewResponseProgress sets rsc=nil—invoking Seek (e.g. via a type assertion) will panic.
- Check if upgrading to a newer azure-sdk-for-go release adds a nil check in progress.Seek.
- Confirm no code asserts the returned io.ReadCloser as io.ReadSeekCloser.
- If needed, wrap or override Seek to handle a nil rsc safely.
1-89: Exclude Azure SDK vendor code from this PR.
This is third-party code under e2e/vendor and unrelated to Nutanix support changes; vendored dependencies should be managed in a separate dependency-bump PR.Likely an incorrect or invalid review comment.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/log.go (1)
37-74: LGTM: Clean logging implementation.The conditional logging logic is well-structured:
Shouldcorrectly checks listener presence and event filtersWriteandWritefappropriately guard expensive operations- Proper separation between public and internal surface areas
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/log.go (1)
1-55: Vendored Azure SDK is expected – ignore. The e2e module’s vendor directory pre-vendors Azure SDK for its existing Azure provider tests; this Nutanix support PR did not mistakenly add it.Likely an incorrect or invalid review comment.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_key_credential.go (1)
48-49: Fix typos in doc comment.
[ suggest_optional_refactor ]-// Do implementes the Do method on the [policy.Polilcy] interface. +// Do implements the Do method on the [policy.Policy] interface.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryapplicationversions_client.go (1)
1-464: Vendored generated Azure SDK client.This is auto-generated vendor code from the Azure SDK for Go. The implementation follows standard Azure SDK patterns for ARM resource management with proper error handling, parameter validation, and LRO support via pollers.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_http_client.go (1)
18-48: LGTM: Default HTTP client configuration.The default HTTP client is properly configured with:
- TLS 1.2 minimum and client-initiated renegotiation (intentional per changelog)
- Reasonable timeouts for production use
- HTTP/2 health checks to work around Go issue #59690
- Standard Azure SDK transport patterns
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/poller.go (1)
1-24: LGTM: Clear enum-like type definition.The
FinalStateViatype and its constants are well-documented and follow standard patterns for string-based enums in Go.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_request_id.go (1)
17-34: LGTM: Request ID policy implementation.The policy correctly generates and sets a unique request ID header when not already present. Error handling for UUID generation is appropriate.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_telemetry.go (1)
21-83: LGTM: Telemetry policy implementation.The telemetry policy correctly:
- Normalizes and truncates ApplicationID as needed
- Preserves existing User-Agent values
- Caches platform information at initialization
- Respects the Disabled option
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_trace_namespace.go (1)
18-30: LGTM: Tracing namespace policy.The policy correctly adds namespace attributes to spans when tracing is enabled. Silently continuing on parse errors is appropriate for non-critical tracing functionality.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_header.go (1)
17-40: LGTM: HTTP header policy with clear deprecation path.The policy correctly handles custom headers with proper canonicalization. The deprecated
WithHTTPHeaderwrapper clearly directs users topolicy.WithHTTPHeader.e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_include_response.go (1)
17-28: LGTM. Response capture policy and wrapper behave as expected.Also applies to: 33-35
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_api_version.go (1)
37-42: LGTM. API version policy correctly applies header/query and validates options.Also applies to: 56-75
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_trace.go (1)
46-95: LGTM: HTTP trace policy correctly sanitizes and annotates spansCaptures method/URL with sanitization, user-agent, client and service request IDs, and error status without leaking full URLs.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceoperatingsystems_client.go (1)
30-45: LGTM: constructor and client wiring look correct.Consistent with other ARM clients; request/response handling and pagers follow standard patterns.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/client_factory.go (1)
23-38: LGTM: Factory initialization is correct and consistent.Subscription/arm.Client propagation and API surface look sound.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceroleinstances_client.go (1)
91-95: LRO status codes correctly include 202.The long-running operations (Delete, Rebuild, Reimage, Restart) properly check for
http.StatusAccepted(202) alongside success codes, which is correct for Azure async patterns.Also applies to: 449-453, 532-536, 613-617
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryimages_client.go (1)
6-7: Review comment not applicable:galleryimages_client.goin the vendor tree was not modified.Likely an incorrect or invalid review comment.
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/client.go
Outdated
Show resolved
Hide resolved
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/pipeline.go
Outdated
Show resolved
Hide resolved
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/response_error.go
Outdated
Show resolved
Hide resolved
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/loc/loc.go
Outdated
Show resolved
Hide resolved
e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/util.go
Outdated
Show resolved
Hide resolved
...zure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/diskencryptionsets_client.go
Outdated
Show resolved
Hide resolved
.../github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/disks_client.go
Outdated
Show resolved
Hide resolved
...com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryimages_client.go
Outdated
Show resolved
Hide resolved
...com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/galleryimages_client.go
Outdated
Show resolved
Hide resolved
...vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/README.md
Outdated
Show resolved
Hide resolved
|
/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. |
|
/test e2e-aws-capi-techpreview |
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
damdo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e/nutanix_test.go (3)
22-28: AfterEach skip is redundantThis AfterEach only skips on non‑Nutanix and is otherwise a no‑op. Safe to remove.
- AfterEach(func() { - if platform != configv1.NutanixPlatformType { - // Because AfterEach always runs, even when tests are skipped, we have to - // explicitly skip it here for other platforms. - Skip("Skipping Nutanix E2E tests") - } - }) + // AfterEach not required; tests are gated in BeforeAll.
41-45: Condense ManagedBy assertionUse HaveKeyWithValue for brevity.
- Expect(nutanixCluster.Annotations).To(HaveKey(clusterv1.ManagedByAnnotation)) - Expect(nutanixCluster.Annotations[clusterv1.ManagedByAnnotation]).To(Equal(managedByAnnotationValueClusterCAPIOperatorInfraClusterController)) + Expect(nutanixCluster.Annotations).To(HaveKeyWithValue( + clusterv1.ManagedByAnnotation, + managedByAnnotationValueClusterCAPIOperatorInfraClusterController, + ))
30-39: Minor test robustness: avoid shared state across ItsConsider re-fetching the NutanixCluster/CAPI Cluster in each It to avoid stale state and reduce ordering coupling, even within Ordered blocks.
Also applies to: 41-51, 53-82, 84-97, 109-130
📜 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)
e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
.golangci.yml(1 hunks)cmd/cluster-capi-operator/main.go(3 hunks)e2e/e2e_test.go(2 hunks)e2e/go.mod(2 hunks)e2e/nutanix_test.go(1 hunks)
🔇 Additional comments (4)
e2e/e2e_test.go (1)
9-9: Scheme registration for Nutanix CRDs — LGTMImport alias and AddToScheme are correct and consistent with lint config.
Also applies to: 53-53
.golangci.yml (1)
97-98: Added importas alias nutanixv1 — aligned with codeMatches package path used in imports.
e2e/nutanix_test.go (1)
1-131: Overall Nutanix E2E coverage — LGTMGood platform gating and readiness checks; assertions cover key wiring.
cmd/cluster-capi-operator/main.go (1)
55-55: Nutanix platform wiring verified — LGTMAll integration points confirmed:
- Root go.mod includes
github.com/nutanix-cloud-native/cluster-api-provider-nutanix v1.7.2- Images ConfigMap references nutanix-cluster-api-controllers image
- ClusterOperator manifest defines nutanixclusters, nutanixmachines, and nutanixmachinetemplates resources with appropriate permissions
- e2e tests include Nutanix provider dependency, confirming gated test paths
| github.com/google/go-cmp v0.7.0 | ||
| github.com/metal3-io/baremetal-operator/apis v0.5.1 | ||
| github.com/metal3-io/cluster-api-provider-metal3/api v1.10.1 | ||
| github.com/nutanix-cloud-native/cluster-api-provider-nutanix v1.7.2 |
There was a problem hiding this comment.
🧩 Analysis chain
Add Nutanix deps: verify compatibility and build/vendor wiring
Looks good. Please:
- Ensure the root module also requires the Nutanix provider (cmd/cluster-capi-operator imports nutanixv1).
- Run tidy/vendor in both root and e2e modules and commit go.sum/vendor changes.
- Double-check CAPN v1.7.2 compatibility with CAPI v1.10.4 in this repo.
Also applies to: 80-80
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check root go.mod has CAPN dep (exclude e2e/):"
fd -a go.mod | grep -v '^e2e/' | xargs rg -n 'nutanix-cloud-native/cluster-api-provider-nutanix' || { echo "Missing CAPN in root go.mod"; exit 1; }
echo "Search for Nutanix images/manifests (operators install step needs these):"
fd -a -t f -E 'e2e/**' 'dev-images.json' | xargs -r rg -n -i 'nutanix' || true
fd -a -t f -E 'e2e/**' -e '.yaml' -e '.yml' | xargs -r rg -n -i 'nutanix' || true
echo "Tidy both modules (no network in CI mirrors may be required):"
if [ -f go.mod ]; then
echo "Root tidy"; grep -q '^module ' go.mod && true
fi
if [ -f e2e/go.mod ]; then
echo "e2e tidy check"; grep -q '^module ' e2e/go.mod && true
fiLength of output: 28397
---
🏁 Script executed:
```shell
#!/bin/bash
set -euo pipefail
echo "Check CAPI version in root go.mod:"
rg -n 'sigs\.k8s\.io/cluster-api' /home/jailuser/git/go.mod || echo "CAPI not found"
Length of output: 383
---
🌐 Web query:
```web
cluster-api-provider-nutanix v1.7.2 release notes CAPI compatibility
💡 Result:
cluster-api-provider-nutanix v1.7.2 (CAPX v1.7.x) is validated to work with Cluster API (CAPI) v1.3.x through v1.9.x. [1][2]
Sources:
- Nutanix CAPX validated integrations (CAPX v1.7.x → CAPI v1.3.x–v1.9.x). [1]
- Cluster API v1.7.2 release notes (context for CAPI v1.7.x). [2]
CAPN v1.7.2 is validated only for CAPI v1.3.x–v1.9.x, but the root go.mod specifies CAPI v1.10.4. Upgrade CAPN to a version supporting v1.10.4 or downgrade CAPI to v1.9.x or earlier.
Root go.mod already correctly declares CAPN v1.7.2 (line 14), and manifests include Nutanix provider assets. However, resolve the version incompatibility before merging.
🤖 Prompt for AI Agents
In e2e/go.mod around line 10, the project pins
github.com/nutanix-cloud-native/cluster-api-provider-nutanix v1.7.2 which is
incompatible with the root CAPI version v1.10.4; update the Nutanix provider to
a release that declares compatibility with CAPI v1.10.x (or alternatively
downgrade the root CAPI to v1.9.x or earlier). Modify e2e/go.mod to reference a
CAPN version that supports CAPI v1.10.4 (or change the root go.mod CAPI
dependency to a compatible v1.9.x), then run go mod tidy and verify
manifests/builds to ensure the versions resolve and integration tests pass
before merging.
45e77fa to
3fb9740
Compare
abhay-nutanix
force-pushed
the
nutanixInfraChange
branch
from
3fb9740 to
750bf25
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go (1)
130-141: Fix invalid JSON tag and make Cluster truly optional.
json:"cluster,omitzero"is not a supported tag; non-pointer field also forces CRD to treat it as required. Switch to pointer + omitempty.- // +kubebuilder:validation:Optional - Cluster NutanixResourceIdentifier `json:"cluster,omitzero"` + // +kubebuilder:validation:Optional + Cluster *NutanixResourceIdentifier `json:"cluster,omitempty"`Note: Regenerate deepcopy/code after this change.
🧹 Nitpick comments (13)
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixclustertemplate_types.go (2)
28-31: Add explicit resource path/shortName and storage version marker for consistency.Align with other CRDs to aid discovery and clusterctl tooling.
-//+kubebuilder:object:root=true -//+kubebuilder:resource:categories=cluster-api +//+kubebuilder:object:root=true +//+kubebuilder:resource:path=nutanixclustertemplates,shortName=nctmpl,scope=Namespaced,categories=cluster-api +//+kubebuilder:storageversion
19-26: Consider allowing metadata on the template resource (labels/annotations).This matches the MachineTemplate resource pattern and helps propagate labels/annotations.
import ( - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + capiv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) @@ type NutanixClusterTemplateResource struct { - Spec NutanixClusterSpec `json:"spec"` + // Standard object metadata carrying labels/annotations for child objects. + // +optional + ObjectMeta capiv1.ObjectMeta `json:"metadata,omitempty"` + Spec NutanixClusterSpec `json:"spec"` }Also applies to: 52-55
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixfailuredomain_types.go (2)
49-53: Unify Conditions type with capiv1.Conditions.Other types in this provider use capiv1.Conditions. Align here for consistency and to match CAPI conventions.
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"` }
40-46: Optional: clarify list semantics for Subnets.If order is irrelevant, consider listType=set to avoid duplicates and make CEL equality less order-sensitive.
-// +kubebuilder:validation:MinItems=1 +// +kubebuilder:validation:MinItems=1 +// +listType=setvendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_types.go (1)
36-39: Add clusterctl move label to aid move operations.This helps clusterctl recognize the object during move.
//+kubebuilder:object:root=true -//+kubebuilder:resource:path=nutanixmachinetemplates,shortName=nmtmpl,scope=Namespaced,categories=cluster-api +//+kubebuilder:resource:path=nutanixmachinetemplates,shortName=nmtmpl,scope=Namespaced,categories=cluster-api //+kubebuilder:storageversion +//+kubebuilder:metadata:labels=clusterctl.cluster.x-k8s.io/move=vendor/github.com/nutanix-cloud-native/prism-go-client/environment/types/types.go (2)
14-16: Use errors.New for static error value.Minor idiomatic tidy-up.
-import ( - "fmt" - "net/url" -) +import ( + "errors" + "net/url" +) @@ -var ErrNotFound = fmt.Errorf("environment key not found") +var ErrNotFound = errors.New("environment key not found")
49-59: Clarify Get contract vs comment (nil value vs not found).Comment suggests returning a success flag, but API returns (interface{}, error). Either:
- Update comment to state ErrNotFound indicates absence, or
- Change signature to (interface{}, bool, error). The former avoids a breaking change.
-// Return whether lookup was successful to distinguish from nil as value. -Get(topology Topology, key string) (interface{}, error) +// Returns ErrNotFound if the key is absent; nil error with a nil value is a valid, present value. +Get(topology Topology, key string) (interface{}, error)vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go (3)
52-56: Make optional field omitempty to avoid serializing empty objects.Keeps Spec compact and avoids spurious diffs.
-ControlPlaneEndpoint capiv1.APIEndpoint `json:"controlPlaneEndpoint"` +ControlPlaneEndpoint capiv1.APIEndpoint `json:"controlPlaneEndpoint,omitempty"`
108-115: Add clusterctl move label to CRD.Consistent with FailureDomain; helps clusterctl operations.
// +kubebuilder:subresource:status // +kubebuilder:storageversion // +kubebuilder:printcolumn:name="ControlplaneEndpoint",type="string",JSONPath=".spec.controlPlaneEndpoint.host",description="ControlplaneEndpoint" // +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.ready",description="in ready status" // +kubebuilder:printcolumn:name="FailureDomains",type="string",JSONPath=".status.failureDomains",description="NutanixCluster FailureDomains" +// +kubebuilder:metadata:labels=clusterctl.cluster.x-k8s.io/move=
176-181: Returning (nil, nil) on unexpected credential kind may hide misconfig.Recommend returning a typed error or at least logging to surface unsupported kinds.
- if prismCentralInfo.CredentialRef.Kind != credentialTypes.SecretKind { - return nil, nil - } + if prismCentralInfo.CredentialRef.Kind != credentialTypes.SecretKind { + return nil, fmt.Errorf("unsupported credentialRef.kind %q for cluster %s/%s; only %q is supported", + prismCentralInfo.CredentialRef.Kind, ncl.Namespace, ncl.Name, credentialTypes.SecretKind) + }vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/types.go (1)
67-79: Mark optional namespace as omitempty.Avoid serializing empty strings for optional refs.
type NutanixCredentialReference struct { ... - Namespace string `json:"namespace"` + Namespace string `json:"namespace,omitempty"` }vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go (2)
227-231: Optional pointer without omitempty serializes null.Add
omitemptyto avoid emitting nulls for absent storageContainer.- StorageContainer *NutanixResourceIdentifier `json:"storageContainer"` + StorageContainer *NutanixResourceIdentifier `json:"storageContainer,omitempty"`
91-93: Inconsistent default marker syntax for Format.Consider
+kubebuilder:default="capx-{{.BaseOS}}-{{.K8sVersion}}-*"for consistency with other defaults.- // +kubebuilder:default:="capx-{{.BaseOS}}-{{.K8sVersion}}-*" + // +kubebuilder:default="capx-{{.BaseOS}}-{{.K8sVersion}}-*"
📜 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 (2)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
.golangci.yml(1 hunks)cmd/cluster-capi-operator/main.go(3 hunks)e2e/e2e_test.go(2 hunks)e2e/go.mod(2 hunks)e2e/nutanix_test.go(1 hunks)go.mod(2 hunks)manifests/0000_30_cluster-api_01_images.configmap.yaml(1 hunks)manifests/0000_30_cluster-api_12_clusteroperator.yaml(1 hunks)manifests/image-references(1 hunks)pkg/controllers/capiinstaller/component_customizer.go(2 hunks)pkg/controllers/common.go(3 hunks)pkg/controllers/corecluster/corecluster_controller.go(1 hunks)pkg/controllers/infracluster/infracluster_controller.go(1 hunks)pkg/controllers/infracluster/nutanix.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/credentials/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/nutanix_test.go
- .golangci.yml
- e2e/e2e_test.go
- e2e/go.mod
- cmd/cluster-capi-operator/main.go
🔇 Additional comments (26)
manifests/0000_30_cluster-api_12_clusteroperator.yaml (1)
142-153: Manifest additions are consistent and complete.The three new Nutanix infrastructure resource entries follow the established pattern used for all other infrastructure providers (AWS, Azure, GCP, IBMPower, Metal3, OpenStack, vSphere). All required resource types are included (clusters, machines, machinetemplates), and the group, namespace, and structure are correct.
Verify that the Nutanix cluster API types are properly registered and that the operator's reconciliation logic handles these resource types correctly.
manifests/0000_30_cluster-api_01_images.configmap.yaml (1)
23-23: LGTM!The Nutanix image mapping follows the established pattern for infrastructure provider controllers and is correctly placed in the images configmap.
manifests/image-references (1)
49-52: LGTM!The Nutanix image reference is correctly structured and follows the same pattern as other infrastructure provider image references in the ImageStream.
pkg/controllers/capiinstaller/component_customizer.go (2)
35-36: LGTM!The Nutanix case correctly maps to the "nutanix-cluster-api-controllers" image key, consistent with the image mapping added in the configmap.
46-46: LGTM!Nutanix is appropriately grouped with AWS, GCP, and IBMCloud providers that follow the
./bin/cluster-api-provider-{name}-controller-managercommand pattern.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/doc.go (1)
1-18: LGTM!Standard package documentation file for the Nutanix v1beta1 API. The license header and package declaration are appropriate for a vendored dependency.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/LICENSE (1)
1-202: LGTM!Standard Apache License 2.0 text for the vendored Nutanix cluster API provider dependency.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/groupversion_info.go (1)
1-44: LGTM!Standard group version setup for the Nutanix v1beta1 API following Cluster API conventions. The group name
infrastructure.cluster.x-k8s.ioaligns with other CAPI infrastructure providers.vendor/github.com/nutanix-cloud-native/prism-go-client/LICENSE (1)
1-202: LGTM!Standard Apache License 2.0 text for the vendored Nutanix Prism Go client dependency.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/conditions.go (1)
1-94: LGTM!Well-organized condition type and reason constants for the Nutanix provider. The naming conventions are consistent, and the use of
capiv1.ConditionTypeis appropriate for Kubernetes status conditions.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanix_types.go (1)
1-155: LGTM!The Nutanix type definitions and helper methods are well-implemented. The
EqualTomethod properly handles nil checks, and theDisplayStringmethod provides useful debugging output. The kubebuilder validation annotations ensure proper API validation.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachinetemplate_conversion.go (1)
1-23: LGTM!Standard conversion hub marker pattern for Kubernetes API versioning. The empty
Hub()methods designate these types as conversion hubs, consistent with Cluster API conventions.pkg/controllers/infracluster/infracluster_controller.go (1)
236-242: LGTM!The Nutanix platform integration follows the established pattern used for other platforms (AWS, GCP, Azure, etc.). Error handling is consistent with the existing code, and the placement in the switch statement is appropriate.
pkg/controllers/corecluster/corecluster_controller.go (1)
231-232: LGTM! Nutanix platform mapping correctly added.The Nutanix platform case correctly maps to "NutanixCluster" with the v1beta1 API version, consistent with other platform mappings in this function.
pkg/controllers/common.go (2)
22-22: LGTM! Import added for Nutanix types.
50-51: LGTM! Nutanix provider support correctly added.Both factory functions now properly handle NutanixPlatformType, returning the appropriate Nutanix Machine and Cluster types, consistent with other provider implementations.
Also applies to: 69-70
go.mod (1)
14-15: LGTM! Dependencies added for Nutanix integration.The required dependencies for Nutanix cluster-api-provider and Prism client are properly added, along with transitive kustomize dependencies.
Also applies to: 306-307
vendor/modules.txt (1)
956-962: LGTM! Vendor modules properly documented.vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_conversion.go (1)
19-23: LGTM! Conversion hub markers properly implemented.Standard conversion hub pattern for CAPI types, required for multi-version CRD support.
vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/parsecreds.go (1)
10-37: Note the acknowledged limitation on line 16.The TODO comment indicates only a single API endpoint is currently supported. The function iterates through credentials but returns immediately upon finding the first BasicAuth credential. This early return pattern is consistent with the TODO comment's intention.
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_conversion.go (1)
19-23: LGTM! Conversion hub markers properly implemented.pkg/controllers/infracluster/nutanix.go (2)
38-91: LGTM! NutanixCluster provisioning correctly implemented.The function follows the established pattern for other providers:
- Checks for existing cluster before creating
- Properly parses API endpoint configuration
- Sets the ManagedBy annotation to prevent CAPI provider interference
- Includes appropriate error handling
94-125: No issues—design verified as correct.The ControlPlaneFailureDomains field expects
[]corev1.LocalObjectReference, which contains only a Name field. This is the correct design: failure domain resources must already exist in the cluster, and this code creates references to them—exactly as implemented. This represents an API evolution from the deprecated inline FailureDomains pattern to the current reference-based approach.vendor/github.com/nutanix-cloud-native/prism-go-client/environment/types/types.go (1)
17-26: Security hygiene note for credentials.Ensure these structs are never logged with default fmt/JSON marshalling in error paths.
Can you confirm we scrub/redact ApiCredentials fields (Username/Password/KeyPair) and ManagementEndpoint.AdditionalTrustBundle in logs?
Also applies to: 31-42
vendor/github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixcluster_types.go (2)
184-193: Confirm intended semantics for inline trust bundles.Method returns nil when AdditionalTrustBundle.Kind is string. If inline PEM is configured, do we intentionally ignore it here because callers expect only references?
If intentional, add a short comment to prevent regressions.
19-27: Go version requirement is already satisfied.The go.mod file specifies Go 1.24.0, which exceeds the minimum Go 1.21+ requirement for
cmp.Or. No changes needed.
...ub.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go
Show resolved
Hide resolved
...ub.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go
Show resolved
Hide resolved
...ub.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go
Show resolved
Hide resolved
...ub.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/nutanixmachine_types.go
Show resolved
Hide resolved
...b.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1/zz_generated.deepcopy.go
Show resolved
Hide resolved
vendor/github.com/nutanix-cloud-native/prism-go-client/environment/credentials/types.go
Show resolved
Hide resolved
|
/retest |
abhay-nutanix
force-pushed
the
nutanixInfraChange
branch
from
78bf406 to
fb3771d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e/nutanix_test.go (1)
1-13: License header present; goheader should pass now.Header matches Apache-2.0 block used in the repo; resolves the earlier linter complaint.
🧹 Nitpick comments (8)
e2e/nutanix_test.go (8)
35-41: Remove redundant AfterEach skip.This AfterEach only calls Skip and does nothing else; it’s safe to drop and reduces noise.
- AfterEach(func() { - if platform != configv1.NutanixPlatformType { - // Because AfterEach always runs, even when tests are skipped, we have to - // explicitly skip it here for other platforms. - Skip("Skipping Nutanix E2E tests") - } - })
43-52: Make NutanixCluster fetch resilient with Eventually to avoid flakes.Wrap the initial Get in Eventually; clusters can be slow to reconcile.
It("should have a NutanixCluster created by the infracluster controller", func() { By("Fetching the NutanixCluster object") - nutanixCluster = &nutanixv1.NutanixCluster{} - err := cl.Get(ctx, client.ObjectKey{ - Name: clusterName, - Namespace: framework.CAPINamespace, - }, nutanixCluster) - Expect(err).ToNot(HaveOccurred(), "should be able to get the NutanixCluster") - Expect(nutanixCluster).ToNot(BeNil()) + var got nutanixv1.NutanixCluster + Eventually(func() error { + return cl.Get(ctx, client.ObjectKey{ + Name: clusterName, + Namespace: framework.CAPINamespace, + }, &got) + }, framework.WaitLong /* timeout */, framework.PollIntervalMedium /* interval */).Should(Succeed(), "should be able to get the NutanixCluster") + nutanixCluster = &got + Expect(nutanixCluster).ToNot(BeNil()) })If PollIntervalMedium isn’t defined in e2e/framework, use the closest available interval constant.
26-26: Add Ginkgo labels for targeted runs.Labels help filter jobs (e.g., -label-filter nutanix).
-var _ = Describe("Cluster API Nutanix InfraCluster", Ordered, func() { +var _ = Describe("Cluster API Nutanix InfraCluster", Ordered, Label("nutanix", "infra"), func() {
77-95: Use HaveLen and keep membership check.Cleaner, and the preceding Infra check guarantees >0.
- Expect(nutanixCluster.Spec.ControlPlaneFailureDomains).ToNot(BeEmpty(), "failure domains should be configured") - Expect(len(nutanixCluster.Spec.ControlPlaneFailureDomains)).To(Equal(len(mapiInfrastructure.Spec.PlatformSpec.Nutanix.FailureDomains))) + Expect(nutanixCluster.Spec.ControlPlaneFailureDomains). + To(HaveLen(len(mapiInfrastructure.Spec.PlatformSpec.Nutanix.FailureDomains))), "failure domains should be configured")
97-110: Specify a poll interval for Eventually.Explicit interval improves stability and reduces API pressure.
- }, framework.WaitLong).Should(BeTrue(), "NutanixCluster should eventually become ready") + }, framework.WaitLong, framework.PollIntervalMedium). + Should(BeTrue(), "NutanixCluster should eventually become ready")If PollIntervalMedium isn’t available, use an existing interval constant from e2e/framework.
113-113: Add labels to the second suite as well.-var _ = Describe("Cluster API Nutanix Cluster", Ordered, func() { +var _ = Describe("Cluster API Nutanix Cluster", Ordered, Label("nutanix", "cluster"), func() {
132-137: Namespace assertion can be overly strict; allow empty (same-ns) refs.InfraRef.Namespace may be empty, implying the Cluster’s namespace.
By("Validating infrastructure reference") Expect(capiCluster.Spec.InfrastructureRef).ToNot(BeNil()) Expect(capiCluster.Spec.InfrastructureRef.Kind).To(Equal("NutanixCluster")) Expect(capiCluster.Spec.InfrastructureRef.Name).To(Equal(clusterName)) - Expect(capiCluster.Spec.InfrastructureRef.Namespace).To(Equal(framework.CAPINamespace)) + Expect(capiCluster.Spec.InfrastructureRef.Namespace). + To(Or(Equal(framework.CAPINamespace), BeEmpty()))If you want to enforce same-namespace explicitly, derive ns := first non-empty of InfraRef.Namespace or capiCluster.Namespace and compare to framework.CAPINamespace.
139-143: Avoid shared-state dependency; re-fetch the Cluster before endpoint assertions.Prevents nil deref if the earlier It fails.
It("should have control plane endpoint initialized", func() { By("Validating CAPI Cluster control plane endpoint") - Expect(capiCluster.Spec.ControlPlaneEndpoint.Host).ToNot(BeEmpty()) - Expect(capiCluster.Spec.ControlPlaneEndpoint.Port).To(BeNumerically(">", 0)) + tmp := &clusterv1.Cluster{} + err := cl.Get(ctx, client.ObjectKey{ + Name: clusterName, + Namespace: framework.CAPINamespace, + }, tmp) + Expect(err).ToNot(HaveOccurred(), "should be able to re-fetch the CAPI Cluster") + Expect(tmp.Spec.ControlPlaneEndpoint.Host).ToNot(BeEmpty()) + Expect(tmp.Spec.ControlPlaneEndpoint.Port).To(BeNumerically(">", 0)) })
📜 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 (2)
e2e/nutanix_test.go(1 hunks)pkg/controllers/infracluster/nutanix.go(1 hunks)
🔇 Additional comments (3)
pkg/controllers/infracluster/nutanix.go (3)
1-37: LGTM: Clean setup with appropriate imports and error definitions.The package declaration, imports, and error definitions are well-structured and follow Go conventions.
39-92: LGTM: Solid implementation with proper error handling.The
ensureNutanixClustermethod follows the established pattern for other providers, with appropriate error handling, logging, and the correctManagedByannotation for CAPI integration.
114-123: ****The previous review was based on deprecated API design. The
FailureDomainsfield is deprecated and replaced byControlPlaneFailureDomains, which accepts onlycorev1.LocalObjectReferenceobjects—each containing just a name reference. ControlPlaneFailureDomains holds references to NutanixFailureDomain objects that separately manage topology details like Cluster and Subnets. The current code correctly maps only the Name field and should not attempt to include Cluster/Subnets in ControlPlaneFailureDomains, as this field type does not support them.Likely an incorrect or invalid review comment.
|
@coderabbitai configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: true
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: false
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
commit_status: true
fail_commit_status: false
collapse_walkthrough: false
changed_files_summary: true
sequence_diagrams: true
estimate_code_review_effort: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
labeling_instructions: []
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
in_progress_fortune: true
poem: true
path_filters: []
path_instructions: []
abort_on_close: true
disable_cache: false
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
ignore_usernames: []
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
pre_merge_checks:
docstrings:
mode: warning
threshold: 80
title:
mode: warning
requirements: ''
description:
mode: warning
issue_assessment:
mode: warning
custom_checks: []
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
phpmd:
enabled: true
phpcs:
enabled: true
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
flake8:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
clang:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
clippy:
enabled: true
sqlfluff:
enabled: true
prismaLint:
enabled: true
pylint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
luacheck:
enabled: true
brakeman:
enabled: true
dotenvLint:
enabled: true
htmlhint:
enabled: true
checkmake:
enabled: true
osvScanner:
enabled: true
chat:
art: true
auto_reply: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
code_guidelines:
enabled: true
filePatterns: []
learnings:
scope: auto
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
mcp:
usage: auto
disabled_servers: []
code_generation:
docstrings:
language: en-US
path_instructions: []
unit_tests:
path_instructions: []
path_filters:
- '!**/vendor/**'
- '!vendor/**'
|
|
/retest |
|
/test e2e-openstack-ovn-techpreview |
|
/lgtm |
|
Scheduling tests matching the |
|
/assign @mdbooth |
|
/retest |
abhay-nutanix
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@abhay-nutanix: This pull request explicitly references no jira issue. 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. |
|
@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. |
|
hi @mdbooth @RadekManak @damdo |
5 participants
Add infraCluster changes for Nutanix to support NutanixCluster
Summary by CodeRabbit
New Features
Tests
Manifests