docs(rest-api): Add missing attribute descriptions in OpenAPI schema, fix all lint errors#2379
docs(rest-api): Add missing attribute descriptions in OpenAPI schema, fix all lint errors#2379thossain-nv wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2379.docs.buildwithfern.com/infra-controller |
91c4177 to
44a7cb4
Compare
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-06-10 19:58:02 UTC | Commit: 91c4177 |
44a7cb4 to
9a92f21
Compare
| description: |- | ||
| Expected Switch identifies a Switch that is expected to be discovered at a Site. Infrastructure Providers can pre-register Expected Switches using BMC, NvOS credentials | ||
| and serial numbers to help with Switch discovery and ingestion. | ||
| Expected Switch identifies an NVLink Switch that is expected to be discovered at a Site. Infrastructure Providers can pre-register Expected Switches using BMC, NVOS credentials |
There was a problem hiding this comment.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
rest-api/sdk/standard/model_batch_update_rack_power_state_request.go (1)
1-264:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRegenerate SDK files instead of manually editing generated code.
All eight files in this review are code-generated from the OpenAPI specification (header comment:
Code generated by OpenAPI Generator ... DO NOT EDIT), yet they contain modifications that were not produced by runningmake generate-sdk. The pipeline failure confirms this:openapi/spec.yaml was modified, but rest-api/sdk/standard/ was not updated. Please ensure you have run: make generate-sdk
Manual edits to generated files violate the project's documented workflow and will be silently discarded the next time the SDK is regenerated from the OpenAPI spec. To correctly apply the OpenAPI schema changes (attribute descriptions, new stats fields, etc.):
- Ensure
rest-api/openapi/spec.yamlcontains all intended schema updates- Run
make generate-sdkto regenerate these files from the spec- Commit only the regenerated output
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/sdk/standard/model_batch_update_rack_power_state_request.go` around lines 1 - 264, The SDK file rest-api/sdk/standard/model_batch_update_rack_power_state_request.go (types BatchUpdateRackPowerStateRequest, NullableBatchUpdateRackPowerStateRequest and methods like UnmarshalJSON, ToMap, MarshalJSON) was edited manually instead of being regenerated from the OpenAPI spec; revert manual changes by ensuring rest-api/openapi/spec.yaml contains your intended schema updates, run make generate-sdk to regenerate the entire rest-api/sdk/standard/ package (which will recreate BatchUpdateRackPowerStateRequest and related methods), and commit only the regenerated files so the generated UnmarshalJSON/ToMap/MarshalJSON implementations are consistent with the spec.rest-api/sdk/standard/model_site_machine_stats_by_status.go (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSDK files are out of sync with OpenAPI spec; regeneration required.
The pipeline failure indicates
rest-api/openapi/spec.yamlwas modified butrest-api/sdk/standard/was not regenerated. Per the coding guidelines, after any OpenAPI schema change you must runmake generate-sdkto ensure the SDK stays aligned with the served API.All seven SDK files in this review share this root cause: the OpenAPI generator has not been re-run against the updated schema.
Run the following to regenerate and commit the updated SDK:
make generate-sdk git add rest-api/sdk/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/sdk/standard/model_site_machine_stats_by_status.go` at line 1, The SDK is out of sync with the updated OpenAPI spec; regenerate the SDK by running the project's SDK generation task (make generate-sdk), then stage and commit the regenerated SDK artifacts (including the updated model_site_machine_stats_by_status.go and the other six SDK files) so the rest-api SDK matches rest-api/openapi/spec.yaml and the pipeline passes.rest-api/sdk/standard/model_machine_summary.go (1)
1-1:⚠️ Potential issue | 🟡 MinorOpenAPI spec changes must regenerate REST SDK and publish docs
When updatingrest-api/openapi/spec.yaml, regeneraterest-api/sdk/standard/and updaterest-api/docs/index.htmlby runningmake -C rest-api generate-sdk(ormake rest-api/generate-sdk) and thenmake -C rest-api publish-openapiso CI doesn’t fail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/sdk/standard/model_machine_summary.go` at line 1, You updated the OpenAPI spec but did not regenerate the REST SDK or publish docs; run the SDK/doc generation steps to update the generated code and docs: regenerate the SDK under the standard SDK output (rest-api/sdk/standard/) and refresh the docs/index.html by executing the make targets (e.g., make -C rest-api generate-sdk or make rest-api/generate-sdk, then make -C rest-api publish-openapi), commit the resulting changes to the SDK and docs, and verify the generated files reflect the spec.yaml changes.rest-api/openapi/spec.yaml (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRegenerate SDK and documentation after OpenAPI schema changes.
The pipeline failure indicates that the OpenAPI schema was modified without regenerating the corresponding SDK and documentation artifacts. This violates the build contract.
Run the following commands to regenerate the required artifacts:
make generate-sdk make publish-openapiThen commit the generated files in
rest-api/sdk/standard/andrest-api/docs/index.html.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/openapi/spec.yaml` at line 1, OpenAPI schema was changed but generated SDK/docs were not updated; run the regen commands `make generate-sdk` and `make publish-openapi`, then add and commit the updated artifacts located under rest-api/sdk/standard/ and rest-api/docs/index.html so the build contract is satisfied.rest-api/sdk/standard/model_tenant_identity_token_delegation_create_or_update_request.go (1)
1-227:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: SDK files out of sync with OpenAPI specification.
The CI pipeline is failing because
rest-api/openapi/spec.yamlwas modified butmake generate-sdkwas not executed to regenerate these SDK files. All seven SDK model files in this review share this issue.Per the CI check:
"rest-api/openapi/spec.yaml was modified, but rest-api/sdk/standard/ was not updated. Please ensure you have run:
make generate-sdk"These files are auto-generated (line 12:
Code generated by OpenAPI Generator) and must be regenerated whenever the OpenAPI specification changes.Required action:
make generate-sdkThen commit the regenerated files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/sdk/standard/model_tenant_identity_token_delegation_create_or_update_request.go` around lines 1 - 227, The generated SDK models (e.g., type TenantIdentityTokenDelegationCreateOrUpdateRequest and its methods MarshalJSON, ToMap, UnmarshalJSON, and NullableTenantIdentityTokenDelegationCreateOrUpdateRequest) are out of sync with rest-api/openapi/spec.yaml; run the generator to refresh these files by executing `make generate-sdk`, verify regenerated code (including the constructor NewTenantIdentityTokenDelegationCreateOrUpdateRequest, UnmarshalJSON validation, and nullable helpers) is updated, and commit the regenerated files so CI passes.rest-api/sdk/standard/model_instance_type.go (1)
1-670:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGenerated SDK files require regeneration.
The pipeline failure indicates the OpenAPI spec was modified but the SDK was not regenerated. Per coding guidelines, files in
rest-api/sdk/standard/are auto-generated and must not be hand-edited. Runmake generate-sdkto regenerate this file from the updated OpenAPI schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/sdk/standard/model_instance_type.go` around lines 1 - 670, The file contains generated SDK code (types like InstanceType, ToMap, MarshalJSON and NullableInstanceType) that is out of sync with the OpenAPI spec; do not edit it manually — regenerate the SDK from the updated spec by running the project generation task (run make generate-sdk) so the file and related symbols (InstanceType, ToMap, MarshalJSON, NullableInstanceType) are recreated correctly and commit the regenerated files instead of hand changes.
🧹 Nitpick comments (1)
rest-api/api/pkg/api/model/networksecuritygroup.go (1)
676-678: ⚖️ Poor tradeoffDirect
time.Now()call couples deprecation logic to wall-clock, hindering testability.Line 676 uses
time.Now().Before(...)to gate whether the deprecatedObjectIDDeprecatedfield is populated. This makes the behavior non-deterministic and prevents unit tests from exercising both branches (before/after deprecation) without waiting for wall-clock time to advance.The proto-conversion convention prefers side inputs passed as arguments (see AGENTS.md: "Side inputs... are passed as additional arguments"). A clock interface or an explicit
nowFunc func() time.Timeparameter would allow tests to control the current time and verify both the pre-deadline and post-deadline code paths.♻️ Testability improvement (optional)
Introduce a package-level clock func that defaults to
time.Nowbut can be overridden in tests:+// nowFunc returns the current time; overridable in tests. +var nowFunc = time.Now + -func NewAPINetworkSecurityGroupPropagationDetails(s *cdbm.NetworkSecurityGroupPropagationDetails) *APINetworkSecurityGroupPropagationDetails { +func NewAPINetworkSecurityGroupPropagationDetails( + s *cdbm.NetworkSecurityGroupPropagationDetails, +) *APINetworkSecurityGroupPropagationDetails { if s == nil { return nil } details := &APINetworkSecurityGroupPropagationDetails{ ObjectID: s.NetworkSecurityGroupPropagationObjectStatus.Id, Details: s.NetworkSecurityGroupPropagationObjectStatus.Details, RelatedInstanceIds: s.NetworkSecurityGroupPropagationObjectStatus.RelatedInstanceIds, UnpropagatedInstanceIds: s.NetworkSecurityGroupPropagationObjectStatus.UnpropagatedInstanceIds, } - if time.Now().Before(networkSecurityGroupPropagationObjectIDDeprecationTime) { + if nowFunc().Before(networkSecurityGroupPropagationObjectIDDeprecationTime) { details.ObjectIDDeprecated = cutil.GetPtr(s.NetworkSecurityGroupPropagationObjectStatus.Id) }Tests can then inject a fake clock:
func TestDeprecationCutover(t *testing.T) { defer func(orig func() time.Time) { nowFunc = orig }(nowFunc) // Before deadline nowFunc = func() time.Time { return time.Date(2026, 7, 1, 0, 0, 0, 0, time.UTC) } details := NewAPINetworkSecurityGroupPropagationDetails(...) require.NotNil(t, details.ObjectIDDeprecated) // After deadline nowFunc = func() time.Time { return time.Date(2026, 7, 10, 0, 0, 0, 0, time.UTC) } details = NewAPINetworkSecurityGroupPropagationDetails(...) require.Nil(t, details.ObjectIDDeprecated) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/model/networksecuritygroup.go` around lines 676 - 678, Replace the direct time.Now() usage with an injectable clock function to make the deprecation check testable: introduce a package-level variable nowFunc (defaulting to time.Now) and use nowFunc().Before(networkSecurityGroupPropagationObjectIDDeprecationTime) where the code currently sets details.ObjectIDDeprecated = cutil.GetPtr(...); update the deprecation check to call nowFunc() instead of time.Now() so tests can override nowFunc to simulate times before and after the deprecation cutover and exercise both branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rest-api/api/pkg/api/model/allocationconstraint.go`:
- Around line 30-33: The deprecation entries currently use OldValue:
"resourceTypeId" which duplicates NewValue and hides the actual exported field
name; update each deprecation block that references resourceTypeId (e.g., the
entries using OldValue/NewValue and resourceTypeIDDeprecationTime) so OldValue
is the model's exported attribute name "ResourceTypeID" and NewValue remains the
legacy key via cutil.GetPtr("resourceTypeId"); apply this fix to every
occurrence noted (the three deprecation blocks that currently have OldValue:
"resourceTypeId").
In `@rest-api/api/pkg/api/model/networksecuritygroup.go`:
- Around line 108-121: The package currently discards the error from time.Parse
when initializing networkSecurityGroupPropagationObjectIDDeprecationTime, and
initializes it at package-level which hides errors and couples policy to code;
change the initialization to a safe, error-free expression (e.g., construct the
timestamp with time.Date(2026, time.July, 9, 0, 0, 0, 0, time.UTC)) or move
parsing into an init() where you explicitly handle/log any parse error, then
assign networkSecurityGroupPropagationObjectIDDeprecationTime before building
networkSecurityGroupPropagationDetailsDeprecations so TakeActionBy uses a
validated value (referencing symbols:
networkSecurityGroupPropagationObjectIDDeprecationTime,
networkSecurityGroupPropagationDetailsDeprecations, DeprecatedEntity, and
time.Parse/time.Date).
In `@rest-api/api/pkg/api/model/nvlinkinterface_test.go`:
- Around line 56-63: The test dereferences got returned by NewAPINVLinkInterface
without a nil check; add a nil guard before asserting fields so a nil return
fails cleanly. Update the test in nvlinkinterface_test.go to check that got is
not nil (e.g., using assert.NotNil or t.Fatalf) immediately after calling
NewAPINVLinkInterface(tt.args.dbnvli), then proceed to assert fields (ID,
InstanceID, NVLinkLogicalPartitionID, DeviceInstance, Status, Created, Updated)
only if got is non-nil.
In `@rest-api/api/pkg/api/model/tenantaccount_test.go`:
- Around line 203-211: The assertions in tenantaccount_test.go currently verify
several fields but omit the new deprecation output fields, so add explicit
checks for those to catch mapping regressions: in the same table-driven
assertion block where tc.apiObj and got are compared, add assert.Equal calls
comparing tc.apiObj.Deprecations with got.Deprecations and tc.apiObj.Deprecated
with got.Deprecated (or the exact field names used on the API model if slightly
different), and ensure any test cases in the table populate those tc.apiObj
deprecation fields so the assertions exercise the mapping.
In `@rest-api/openapi/spec.yaml`:
- Line 21574: Fix the typo in the OpenAPI description that reads "retreival
endpoints" by changing it to "retrieval endpoints" in the description associated
with the Attachment statistics/ includeAttachmentStats text (the string
mentioning "Attachment statistics for the Network Security Group. Returned when
`includeAttachmentStats` query param is set to true in retreival endpoints.").
Ensure this description now matches the correctly spelled occurrences elsewhere
(e.g., the other "retrieval endpoints" entry).
In `@rest-api/sdk/standard/model_batch_tray_firmware_update_request.go`:
- Around line 28-30: Regenerate the OpenAPI SDK to sync generated artifacts: run
the project's OpenAPI/SDK generation step and update the generated files under
rest-api/sdk/standard/ so they reflect the latest rest-api/openapi/spec.yaml;
specifically ensure changes to the BatchTrayFirmwareUpdateRequest model (file
model_batch_tray_firmware_update_request.go and its fields SiteId and Filter /
TrayFilter) and any other modified generated files are committed so the SDK and
spec are in lockstep.
In `@rest-api/sdk/standard/model_instance_create_request.go`:
- Around line 44-54: SDK artifacts under the generated Standard model are out of
sync with the OpenAPI schema. Regenerate the SDK from rest-api/openapi/spec.yaml
using the normal generation flow, then commit any resulting updates in generated
types such as ModelInstanceCreateRequest so the generated fields and
schema-derived comments match the current spec. Verify there are no remaining
deltas anywhere under the generated rest-api/sdk/standard/ package before
merging.
In `@rest-api/sdk/standard/model_nv_link_interface_create_or_update_request.go`:
- Around line 26-27: The generated SDK removed deprecated fields too early
(e.g., the NvLinkLogicalPartitionId field in
model_nv_link_interface_create_or_update_request.go); restore the deprecated
attributes in the OpenAPI spec (openapi/spec.yaml) by re-adding the removed
properties and marking them deprecated: true, then regenerate the
rest-api/sdk/standard/ SDK so
model_nv_link_interface_create_or_update_request.go (and other affected
types/methods) include the deprecated fields for the migration window.
In `@rest-api/sdk/standard/model_site_machine_stats_by_health.go`:
- Around line 25-27: The CI failure means the SDK snapshot under
rest-api/sdk/standard/ is out of sync with rest-api/openapi/spec.yaml; rerun the
project's OpenAPI/SDK generation command (the repo's SDK generator script or
make target used to produce rest-api/sdk/standard/) to regenerate all SDK
artifacts (including model_site_machine_stats_by_health.go and the rest of the
standard package), verify rest-api/openapi/spec.yaml and the generated files
match, stage and commit the entire rest-api/sdk/standard/ tree (all generated
files), and re-run the checks to ensure no unstaged changes remain before
pushing.
In `@rest-api/sdk/standard/model_tenant_account.go`:
- Around line 26-47: The SDK model files (including model_tenant_account.go and
the other seven auto-generated models under rest-api/sdk/standard/) were edited
manually but must be regenerated; revert any manual changes and run the
generator instead: run make generate-sdk to regenerate all files from
openapi/spec.yaml, then git add the entire rest-api/sdk/standard/ directory and
commit the generated output so the generated header (// Code generated by
OpenAPI Generator...) and all models (e.g., the struct definitions like Id,
TenantId, StatusHistory in model_tenant_account.go) are updated consistently.
In `@rest-api/sdk/standard/model_vpc_prefix.go`:
- Around line 25-34: The VpcPrefix public model removed the tenantId field which
is a breaking change; restore a deprecated read-only TenantId field on the
VpcPrefix struct to maintain compatibility. Add a field named TenantId of type
*string with the json tag `tenantId,omitempty`, and document it as
Deprecated/read-only in the field comment so consumers know it will be removed
later; keep it present until a planned major-version removal to avoid
compile-time breaks for downstream clients.
---
Outside diff comments:
In `@rest-api/openapi/spec.yaml`:
- Line 1: OpenAPI schema was changed but generated SDK/docs were not updated;
run the regen commands `make generate-sdk` and `make publish-openapi`, then add
and commit the updated artifacts located under rest-api/sdk/standard/ and
rest-api/docs/index.html so the build contract is satisfied.
In `@rest-api/sdk/standard/model_batch_update_rack_power_state_request.go`:
- Around line 1-264: The SDK file
rest-api/sdk/standard/model_batch_update_rack_power_state_request.go (types
BatchUpdateRackPowerStateRequest, NullableBatchUpdateRackPowerStateRequest and
methods like UnmarshalJSON, ToMap, MarshalJSON) was edited manually instead of
being regenerated from the OpenAPI spec; revert manual changes by ensuring
rest-api/openapi/spec.yaml contains your intended schema updates, run make
generate-sdk to regenerate the entire rest-api/sdk/standard/ package (which will
recreate BatchUpdateRackPowerStateRequest and related methods), and commit only
the regenerated files so the generated UnmarshalJSON/ToMap/MarshalJSON
implementations are consistent with the spec.
In `@rest-api/sdk/standard/model_instance_type.go`:
- Around line 1-670: The file contains generated SDK code (types like
InstanceType, ToMap, MarshalJSON and NullableInstanceType) that is out of sync
with the OpenAPI spec; do not edit it manually — regenerate the SDK from the
updated spec by running the project generation task (run make generate-sdk) so
the file and related symbols (InstanceType, ToMap, MarshalJSON,
NullableInstanceType) are recreated correctly and commit the regenerated files
instead of hand changes.
In `@rest-api/sdk/standard/model_machine_summary.go`:
- Line 1: You updated the OpenAPI spec but did not regenerate the REST SDK or
publish docs; run the SDK/doc generation steps to update the generated code and
docs: regenerate the SDK under the standard SDK output (rest-api/sdk/standard/)
and refresh the docs/index.html by executing the make targets (e.g., make -C
rest-api generate-sdk or make rest-api/generate-sdk, then make -C rest-api
publish-openapi), commit the resulting changes to the SDK and docs, and verify
the generated files reflect the spec.yaml changes.
In `@rest-api/sdk/standard/model_site_machine_stats_by_status.go`:
- Line 1: The SDK is out of sync with the updated OpenAPI spec; regenerate the
SDK by running the project's SDK generation task (make generate-sdk), then stage
and commit the regenerated SDK artifacts (including the updated
model_site_machine_stats_by_status.go and the other six SDK files) so the
rest-api SDK matches rest-api/openapi/spec.yaml and the pipeline passes.
In
`@rest-api/sdk/standard/model_tenant_identity_token_delegation_create_or_update_request.go`:
- Around line 1-227: The generated SDK models (e.g., type
TenantIdentityTokenDelegationCreateOrUpdateRequest and its methods MarshalJSON,
ToMap, UnmarshalJSON, and
NullableTenantIdentityTokenDelegationCreateOrUpdateRequest) are out of sync with
rest-api/openapi/spec.yaml; run the generator to refresh these files by
executing `make generate-sdk`, verify regenerated code (including the
constructor NewTenantIdentityTokenDelegationCreateOrUpdateRequest, UnmarshalJSON
validation, and nullable helpers) is updated, and commit the regenerated files
so CI passes.
---
Nitpick comments:
In `@rest-api/api/pkg/api/model/networksecuritygroup.go`:
- Around line 676-678: Replace the direct time.Now() usage with an injectable
clock function to make the deprecation check testable: introduce a package-level
variable nowFunc (defaulting to time.Now) and use
nowFunc().Before(networkSecurityGroupPropagationObjectIDDeprecationTime) where
the code currently sets details.ObjectIDDeprecated = cutil.GetPtr(...); update
the deprecation check to call nowFunc() instead of time.Now() so tests can
override nowFunc to simulate times before and after the deprecation cutover and
exercise both branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cc65b4a6-d244-406f-bd72-d7504ef33e5c
📒 Files selected for processing (123)
rest-api/api/pkg/api/handler/instancetype.gorest-api/api/pkg/api/handler/stats.gorest-api/api/pkg/api/handler/stats_test.gorest-api/api/pkg/api/handler/subnet_test.gorest-api/api/pkg/api/handler/util/common/common.gorest-api/api/pkg/api/handler/util/common/common_test.gorest-api/api/pkg/api/model/allocationconstraint.gorest-api/api/pkg/api/model/deprecation.gorest-api/api/pkg/api/model/instancetype.gorest-api/api/pkg/api/model/ipblock.gorest-api/api/pkg/api/model/ipblock_test.gorest-api/api/pkg/api/model/networksecuritygroup.gorest-api/api/pkg/api/model/nvlinkinterface.gorest-api/api/pkg/api/model/nvlinkinterface_test.gorest-api/api/pkg/api/model/stats.gorest-api/api/pkg/api/model/stats_test.gorest-api/api/pkg/api/model/subnet.gorest-api/api/pkg/api/model/tenant.gorest-api/api/pkg/api/model/tenantaccount.gorest-api/api/pkg/api/model/tenantaccount_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/sdk/simple/instance.gorest-api/sdk/standard/model_allocation.gorest-api/sdk/standard/model_allocation_constraint.gorest-api/sdk/standard/model_allocation_create_request.gorest-api/sdk/standard/model_batch_bring_up_rack_request.gorest-api/sdk/standard/model_batch_instance_create_request.gorest-api/sdk/standard/model_batch_rack_firmware_update_request.gorest-api/sdk/standard/model_batch_tray_firmware_update_request.gorest-api/sdk/standard/model_batch_update_rack_power_state_request.gorest-api/sdk/standard/model_batch_update_tray_power_state_request.gorest-api/sdk/standard/model_component_diff.gorest-api/sdk/standard/model_deprecation.gorest-api/sdk/standard/model_dpu_extension_service_observability_config.gorest-api/sdk/standard/model_infini_band_interface.gorest-api/sdk/standard/model_infini_band_partition.gorest-api/sdk/standard/model_infini_band_partition_update_request.gorest-api/sdk/standard/model_infrastructure_provider.gorest-api/sdk/standard/model_infrastructure_provider_stats.gorest-api/sdk/standard/model_instance.gorest-api/sdk/standard/model_instance_count_by_status.gorest-api/sdk/standard/model_instance_create_request.gorest-api/sdk/standard/model_instance_type.gorest-api/sdk/standard/model_instance_type_create_request.gorest-api/sdk/standard/model_instance_type_stats.gorest-api/sdk/standard/model_instance_type_summary.gorest-api/sdk/standard/model_instance_type_update_request.gorest-api/sdk/standard/model_instance_update_request.gorest-api/sdk/standard/model_interface.gorest-api/sdk/standard/model_interface_create_request.gorest-api/sdk/standard/model_ip_block.gorest-api/sdk/standard/model_ip_block_count_by_status.gorest-api/sdk/standard/model_ip_block_create_request.gorest-api/sdk/standard/model_ip_block_summary.gorest-api/sdk/standard/model_ip_block_update_request.gorest-api/sdk/standard/model_machine.gorest-api/sdk/standard/model_machine_bmc_info.gorest-api/sdk/standard/model_machine_capability.gorest-api/sdk/standard/model_machine_count_by_status.gorest-api/sdk/standard/model_machine_dmi_data.gorest-api/sdk/standard/model_machine_gpu_info.gorest-api/sdk/standard/model_machine_infini_band_interface.gorest-api/sdk/standard/model_machine_instance_type.gorest-api/sdk/standard/model_machine_instance_type_create_request.gorest-api/sdk/standard/model_machine_instance_type_stats.gorest-api/sdk/standard/model_machine_interface.gorest-api/sdk/standard/model_machine_metadata.gorest-api/sdk/standard/model_machine_network_interface.gorest-api/sdk/standard/model_machine_online_repair.gorest-api/sdk/standard/model_machine_status_breakdown.gorest-api/sdk/standard/model_machine_summary.gorest-api/sdk/standard/model_machine_update_request.gorest-api/sdk/standard/model_network_security_group.gorest-api/sdk/standard/model_network_security_group_create_request.gorest-api/sdk/standard/model_network_security_group_propagation_details.gorest-api/sdk/standard/model_network_security_group_rule.gorest-api/sdk/standard/model_network_security_group_stats.gorest-api/sdk/standard/model_network_security_group_update_request.gorest-api/sdk/standard/model_nv_link_interface.gorest-api/sdk/standard/model_nv_link_interface_create_or_update_request.gorest-api/sdk/standard/model_nv_link_logical_partition.gorest-api/sdk/standard/model_nv_link_logical_partition_stats.gorest-api/sdk/standard/model_open_id_configuration.gorest-api/sdk/standard/model_operating_system_site_association.gorest-api/sdk/standard/model_rack.gorest-api/sdk/standard/model_site.gorest-api/sdk/standard/model_site_capabilities.gorest-api/sdk/standard/model_site_create_request.gorest-api/sdk/standard/model_site_machine_stats.gorest-api/sdk/standard/model_site_machine_stats_by_allocation.gorest-api/sdk/standard/model_site_machine_stats_by_health.gorest-api/sdk/standard/model_site_machine_stats_by_status.gorest-api/sdk/standard/model_site_machine_stats_by_status_and_health.gorest-api/sdk/standard/model_site_summary.gorest-api/sdk/standard/model_site_update_request.gorest-api/sdk/standard/model_sku_tpm.gorest-api/sdk/standard/model_ssh_key.gorest-api/sdk/standard/model_ssh_key_group_create_request.gorest-api/sdk/standard/model_ssh_key_group_site_association.gorest-api/sdk/standard/model_ssh_key_group_update_request.gorest-api/sdk/standard/model_ssh_key_update_request.gorest-api/sdk/standard/model_status_detail.gorest-api/sdk/standard/model_subnet.gorest-api/sdk/standard/model_subnet_count_by_status.gorest-api/sdk/standard/model_subnet_create_request.gorest-api/sdk/standard/model_subnet_update_request.gorest-api/sdk/standard/model_tenant_account.gorest-api/sdk/standard/model_tenant_account_count_by_status.gorest-api/sdk/standard/model_tenant_account_create_request.gorest-api/sdk/standard/model_tenant_identity_basic_client_secret_request.gorest-api/sdk/standard/model_tenant_identity_basic_client_secret_response.gorest-api/sdk/standard/model_tenant_identity_config.gorest-api/sdk/standard/model_tenant_identity_token_delegation.gorest-api/sdk/standard/model_tenant_identity_token_delegation_create_or_update_request.gorest-api/sdk/standard/model_tenant_instance_type_allocation_stats.gorest-api/sdk/standard/model_tenant_instance_type_stats.gorest-api/sdk/standard/model_tenant_stats.gorest-api/sdk/standard/model_tray.gorest-api/sdk/standard/model_user.gorest-api/sdk/standard/model_vpc_count_by_status.gorest-api/sdk/standard/model_vpc_prefix.gorest-api/sdk/standard/model_vpc_prefix_update_request.go
💤 Files with no reviewable changes (4)
- rest-api/api/pkg/api/model/ipblock.go
- rest-api/api/pkg/api/model/subnet.go
- rest-api/api/pkg/api/handler/subnet_test.go
- rest-api/api/pkg/api/model/ipblock_test.go
| OldValue: "resourceTypeId", | ||
| NewValue: cutil.GetPtr("resourceTypeId"), | ||
| Type: DeprecationTypeAttribute, | ||
| TakeActionBy: resourceTypeIDDeprecationTime, |
There was a problem hiding this comment.
Deprecation metadata references the wrong deprecated attribute name.
At Line [30], OldValue is set to resourceTypeId, but the deprecated response attribute exposed by this model is ResourceTypeID (Line [102]). This emits a misleading deprecation (resourceTypeId → resourceTypeId) and hides the actual legacy key from clients.
Suggested patch
allocationConstraintDeprecations = []DeprecatedEntity{
{
- OldValue: "resourceTypeId",
+ OldValue: "ResourceTypeID",
NewValue: cutil.GetPtr("resourceTypeId"),
Type: DeprecationTypeAttribute,
TakeActionBy: resourceTypeIDDeprecationTime,
},
}Also applies to: 101-103, 149-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/pkg/api/model/allocationconstraint.go` around lines 30 - 33, The
deprecation entries currently use OldValue: "resourceTypeId" which duplicates
NewValue and hides the actual exported field name; update each deprecation block
that references resourceTypeId (e.g., the entries using OldValue/NewValue and
resourceTypeIDDeprecationTime) so OldValue is the model's exported attribute
name "ResourceTypeID" and NewValue remains the legacy key via
cutil.GetPtr("resourceTypeId"); apply this fix to every occurrence noted (the
three deprecation blocks that currently have OldValue: "resourceTypeId").
| var ( | ||
| // Time when the NetworkSecurityGroup propagation object_id attribute will be deprecated | ||
| networkSecurityGroupPropagationObjectIDDeprecationTime, _ = time.Parse(time.RFC1123, "Thu, 09 Jul 2026 00:00:00 UTC") | ||
|
|
||
| // Deprecations for the NetworkSecurityGroup model | ||
| networkSecurityGroupPropagationDetailsDeprecations = []DeprecatedEntity{ | ||
| { | ||
| OldValue: "object_id", | ||
| NewValue: cutil.GetPtr("objectId"), | ||
| Type: DeprecationTypeAttribute, | ||
| TakeActionBy: networkSecurityGroupPropagationObjectIDDeprecationTime, | ||
| }, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Error handling silently discarded for deprecation timestamp parsing.
Line 110 discards the parse error from time.Parse with _, risking a zero-value timestamp if the RFC1123 string is malformed. A malformed timestamp would cause line 676's time.Now().Before(networkSecurityGroupPropagationObjectIDDeprecationTime) to always evaluate true (comparing against zero time), unintentionally extending the deprecated field's lifespan indefinitely.
Additionally, hardcoding the deprecation deadline in a package-level var initializer couples the deprecation policy to code rather than configuration and obscures intent.
🛡️ Proposed fix
+// NetworkSecurityGroupPropagationObjectIDDeprecationDeadline is the RFC1123
+// timestamp after which the legacy object_id attribute will no longer be
+// included in NetworkSecurityGroupPropagationDetails responses.
+const networkSecurityGroupPropagationObjectIDDeprecationDeadline = "Thu, 09 Jul 2026 00:00:00 UTC"
+
var (
- // Time when the NetworkSecurityGroup propagation object_id attribute will be deprecated
- networkSecurityGroupPropagationObjectIDDeprecationTime, _ = time.Parse(time.RFC1123, "Thu, 09 Jul 2026 00:00:00 UTC")
+ networkSecurityGroupPropagationObjectIDDeprecationTime = mustParseTime(
+ time.RFC1123,
+ networkSecurityGroupPropagationObjectIDDeprecationDeadline,
+ )
// Deprecations for the NetworkSecurityGroup model
networkSecurityGroupPropagationDetailsDeprecations = []DeprecatedEntity{
{
OldValue: "object_id",
NewValue: cutil.GetPtr("objectId"),
Type: DeprecationTypeAttribute,
TakeActionBy: networkSecurityGroupPropagationObjectIDDeprecationTime,
},
}
)
+
+// mustParseTime parses t in the given layout and panics on error.
+// Only safe for package-level init of compile-time-known constants.
+func mustParseTime(layout, value string) time.Time {
+ parsed, err := time.Parse(layout, value)
+ if err != nil {
+ panic(fmt.Sprintf("invalid deprecation timestamp %q: %v", value, err))
+ }
+ return parsed
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/pkg/api/model/networksecuritygroup.go` around lines 108 - 121,
The package currently discards the error from time.Parse when initializing
networkSecurityGroupPropagationObjectIDDeprecationTime, and initializes it at
package-level which hides errors and couples policy to code; change the
initialization to a safe, error-free expression (e.g., construct the timestamp
with time.Date(2026, time.July, 9, 0, 0, 0, 0, time.UTC)) or move parsing into
an init() where you explicitly handle/log any parse error, then assign
networkSecurityGroupPropagationObjectIDDeprecationTime before building
networkSecurityGroupPropagationDetailsDeprecations so TakeActionBy uses a
validated value (referencing symbols:
networkSecurityGroupPropagationObjectIDDeprecationTime,
networkSecurityGroupPropagationDetailsDeprecations, DeprecatedEntity, and
time.Parse/time.Date).
There was a problem hiding this comment.
You can check the string value to confirm that it's parseable. Deprecation time is meant to be fixed, not dynamically configured.
There was a problem hiding this comment.
@thossain-nv, that is a fair point — a compile-time literal whose correctness is trivially verifiable by inspection makes the _ discard safe here. I withdraw the concern.
As a purely optional cleanup, time.Date(2026, time.July, 9, 0, 0, 0, 0, time.UTC) would eliminate the parsing step entirely and make the intent even more explicit, but there is nothing wrong with the current approach.
| got := NewAPINVLinkInterface(tt.args.dbnvli) | ||
| assert.Equal(t, tt.want.ID, got.ID) | ||
| assert.Equal(t, tt.want.InstanceID, got.InstanceID) | ||
| assert.Equal(t, tt.want.NVLinkLogicalPartitionID, got.NVLinkLogicalPartitionID) | ||
| assert.Equal(t, tt.want.DeviceInstance, got.DeviceInstance) | ||
| assert.Equal(t, tt.want.Status, got.Status) | ||
| assert.Equal(t, tt.want.Created, got.Created) | ||
| assert.Equal(t, tt.want.Updated, got.Updated) |
There was a problem hiding this comment.
Add a nil guard before field assertions.
At Line [57], got is dereferenced immediately; if constructor behavior regresses and returns nil, this test panics instead of failing with a clear assertion.
Suggested patch
t.Run(tt.name, func(t *testing.T) {
got := NewAPINVLinkInterface(tt.args.dbnvli)
+ require.NotNil(t, got)
assert.Equal(t, tt.want.ID, got.ID)
assert.Equal(t, tt.want.InstanceID, got.InstanceID)
assert.Equal(t, tt.want.NVLinkLogicalPartitionID, got.NVLinkLogicalPartitionID)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| got := NewAPINVLinkInterface(tt.args.dbnvli) | |
| assert.Equal(t, tt.want.ID, got.ID) | |
| assert.Equal(t, tt.want.InstanceID, got.InstanceID) | |
| assert.Equal(t, tt.want.NVLinkLogicalPartitionID, got.NVLinkLogicalPartitionID) | |
| assert.Equal(t, tt.want.DeviceInstance, got.DeviceInstance) | |
| assert.Equal(t, tt.want.Status, got.Status) | |
| assert.Equal(t, tt.want.Created, got.Created) | |
| assert.Equal(t, tt.want.Updated, got.Updated) | |
| got := NewAPINVLinkInterface(tt.args.dbnvli) | |
| require.NotNil(t, got) | |
| assert.Equal(t, tt.want.ID, got.ID) | |
| assert.Equal(t, tt.want.InstanceID, got.InstanceID) | |
| assert.Equal(t, tt.want.NVLinkLogicalPartitionID, got.NVLinkLogicalPartitionID) | |
| assert.Equal(t, tt.want.DeviceInstance, got.DeviceInstance) | |
| assert.Equal(t, tt.want.Status, got.Status) | |
| assert.Equal(t, tt.want.Created, got.Created) | |
| assert.Equal(t, tt.want.Updated, got.Updated) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/pkg/api/model/nvlinkinterface_test.go` around lines 56 - 63, The
test dereferences got returned by NewAPINVLinkInterface without a nil check; add
a nil guard before asserting fields so a nil return fails cleanly. Update the
test in nvlinkinterface_test.go to check that got is not nil (e.g., using
assert.NotNil or t.Fatalf) immediately after calling
NewAPINVLinkInterface(tt.args.dbnvli), then proceed to assert fields (ID,
InstanceID, NVLinkLogicalPartitionID, DeviceInstance, Status, Created, Updated)
only if got is non-nil.
| assert.Equal(t, tc.apiObj.ID, got.ID) | ||
| assert.Equal(t, tc.apiObj.InfrastructureProviderID, got.InfrastructureProviderID) | ||
| assert.Equal(t, tc.apiObj.InfrastructureProviderOrg, got.InfrastructureProviderOrg) | ||
| assert.Equal(t, tc.apiObj.TenantID, got.TenantID) | ||
| assert.Equal(t, tc.apiObj.TenantOrg, got.TenantOrg) | ||
| assert.Equal(t, tc.apiObj.TenantContact, got.TenantContact) | ||
| assert.Equal(t, tc.apiObj.AllocationCount, got.AllocationCount) | ||
| assert.Equal(t, tc.apiObj.Status, got.Status) | ||
| assert.Equal(t, tc.apiObj.StatusHistory, got.StatusHistory) |
There was a problem hiding this comment.
Assert deprecation-mapping outputs in this test case.
The revised assertions no longer validate newly added Deprecations/deprecated output fields, so mapping regressions introduced in this PR can slip through. Add explicit checks for those fields in this table-driven test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/pkg/api/model/tenantaccount_test.go` around lines 203 - 211, The
assertions in tenantaccount_test.go currently verify several fields but omit the
new deprecation output fields, so add explicit checks for those to catch mapping
regressions: in the same table-driven assertion block where tc.apiObj and got
are compared, add assert.Equal calls comparing tc.apiObj.Deprecations with
got.Deprecations and tc.apiObj.Deprecated with got.Deprecated (or the exact
field names used on the API model if slightly different), and ensure any test
cases in the table populate those tc.apiObj deprecation fields so the assertions
exercise the mapping.
| description: Number of rules in the Network Security Group | ||
| attachmentStats: | ||
| $ref: '#/components/schemas/NetworkSecurityGroupStats' | ||
| description: Attachment statistics for the Network Security Group. Returned when `includeAttachmentStats` query param is set to true in retreival endpoints. |
There was a problem hiding this comment.
Correct spelling: "retreival" → "retrieval".
Line 21574 uses "retreival endpoints" but line 15234 correctly spells it "retrieval endpoints". These descriptions should be consistent.
📝 Suggested fix
- description: Attachment statistics for the Network Security Group. Returned when `includeAttachmentStats` query param is set to true in retreival endpoints.
+ description: Attachment statistics for the Network Security Group. Returned when `includeAttachmentStats` query param is set to true in retrieval endpoints.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: Attachment statistics for the Network Security Group. Returned when `includeAttachmentStats` query param is set to true in retreival endpoints. | |
| description: Attachment statistics for the Network Security Group. Returned when `includeAttachmentStats` query param is set to true in retrieval endpoints. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/openapi/spec.yaml` at line 21574, Fix the typo in the OpenAPI
description that reads "retreival endpoints" by changing it to "retrieval
endpoints" in the description associated with the Attachment statistics/
includeAttachmentStats text (the string mentioning "Attachment statistics for
the Network Security Group. Returned when `includeAttachmentStats` query param
is set to true in retreival endpoints."). Ensure this description now matches
the correctly spelled occurrences elsewhere (e.g., the other "retrieval
endpoints" entry).
| OperatingSystemId NullableString `json:"operatingSystemId,omitempty"` | ||
| // ID of the desired Network Security Group to attach to the Instance | ||
| NetworkSecurityGroupId NullableString `json:"networkSecurityGroupId,omitempty"` | ||
| // Override iPXE script specified in OS, must be specified if Operating System is not specified | ||
| IpxeScript NullableString `json:"ipxeScript,omitempty"` | ||
| // When set to true, the iPXE script specified by OS or overridden here will always be run when rebooting the Instance. OS must be of iPXE type. | ||
| AlwaysBootWithCustomIpxe *bool `json:"alwaysBootWithCustomIpxe,omitempty"` | ||
| // When set to true, the Instance will be enabled with the Phone Home service. | ||
| PhoneHomeEnabled *bool `json:"phoneHomeEnabled,omitempty"` | ||
| Labels map[string]string `json:"labels,omitempty"` | ||
| PhoneHomeEnabled *bool `json:"phoneHomeEnabled,omitempty"` | ||
| // User-defined key-value labels | ||
| Labels map[string]string `json:"labels,omitempty"` |
There was a problem hiding this comment.
Regenerate SDK artifacts to resolve OpenAPI/SDK drift before merge.
The CI warning shows rest-api/openapi/spec.yaml changed but generated SDK artifacts are still considered out of sync. Please rerun make generate-sdk and commit any remaining deltas so schema and SDK stay consistent.
As per coding guidelines, files under rest-api/sdk/standard/ are generated and should be refreshed when OpenAPI changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/sdk/standard/model_instance_create_request.go` around lines 44 - 54,
SDK artifacts under the generated Standard model are out of sync with the
OpenAPI schema. Regenerate the SDK from rest-api/openapi/spec.yaml using the
normal generation flow, then commit any resulting updates in generated types
such as ModelInstanceCreateRequest so the generated fields and schema-derived
comments match the current spec. Verify there are no remaining deltas anywhere
under the generated rest-api/sdk/standard/ package before merging.
Sources: Coding guidelines, Pipeline failures
| NvLinkLogicalPartitionId *string `json:"nvLinkLogicalPartitionId,omitempty"` | ||
| // GPU index for this NVLink interface. Must be non-negative, unique within the request, and within the GPU count exposed by the selected Machine or Instance Type. |
There was a problem hiding this comment.
Consolidated root cause: schema deprecations were converted into immediate SDK removals.
Both files reflect the same integration risk: legacy/deprecated attributes were removed from generated public SDK types/methods without a compatibility bridge. Keep deprecated fields in openapi/spec.yaml through the stated sunset window, mark them deprecated: true, then regenerate rest-api/sdk/standard/ so clients get a migration period instead of an immediate break.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/sdk/standard/model_nv_link_interface_create_or_update_request.go`
around lines 26 - 27, The generated SDK removed deprecated fields too early
(e.g., the NvLinkLogicalPartitionId field in
model_nv_link_interface_create_or_update_request.go); restore the deprecated
attributes in the OpenAPI spec (openapi/spec.yaml) by re-adding the removed
properties and marking them deprecated: true, then regenerate the
rest-api/sdk/standard/ SDK so
model_nv_link_interface_create_or_update_request.go (and other affected
types/methods) include the deprecated fields for the migration window.
Source: Coding guidelines
There was a problem hiding this comment.
Older SDK version will continue to work since both attributes are served by API. Only SDK upgrades will need to change.
There was a problem hiding this comment.
@thossain-nv, that is a valid and well-reasoned distinction. Since the API continues to serve both nvLinklogicalPartitionId and nvLinkLogicalPartitionId during the deprecation window, wire-level compatibility is preserved for existing SDK consumers. The compile-time rename in the Go SDK is a deliberate, scoped migration cost that only materialises upon a voluntary SDK upgrade — which is the appropriate place for such a change. Withdrawing the concern.
83c223f to
fb5f63a
Compare
… fix all lint errors Signed-off-by: Tareque Hossain <thossain@nvidia.com>
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
fb5f63a to
64cf2b6
Compare
Description
Deprecations
Introduces deprecations for the following attributes
NVLinkInterface:
nvLinklogicalPartitionIdattribute is deprecated which was improperly cased, will remain available until July 9thnvLinkLogicalPartitionIdAllocationConstraint
ResourceTypeIDattribute is deprecated which was improperly cased, will remain available until July 9thresourceTypeIdNetworkSecurityGroup.PropagationDetails
object_idattribute is deprecated which was improperly cased, will remain available until July 9thobjectIdTenantAccount
accountNumber,subscriptionId,subscriptionTierwhich do not carry any meaningful information, will remain available until July 9thDeprecated Attributes Cleanup
Both these attributes have been deprecated more than 2 years ago and not returned in API
blockSizeipBlockSizeType of Change
Breaking Changes
Testing