This repository was archived by the owner on Jun 2, 2026. It is now read-only.
feat: Infer Provider/Tenant from caller's org across Tenant Account #603
Draft
parmani-nv wants to merge 1 commit into
Draft
feat: Infer Provider/Tenant from caller's org across Tenant Account #603parmani-nv wants to merge 1 commit into
parmani-nv wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedDraft detected. 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:
WalkthroughThis PR shifts API handlers, request models, and generated SDK code from explicit ChangesTenant Account Handler & Model Refactoring
SDK Request Model Constructors & Field Types
SDK API Deprecation Documentation & Pagination Refactoring
OpenAPI Specification & CLI Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR spans multiple layers of significant behavioral changes: handler authorization logic refactoring with org-membership inference, request model constructor signature changes affecting API contracts, widespread pagination behavior refactoring across ~30 SDK generated files, and comprehensive OpenAPI specification updates. While many individual SDK pagination changes are repetitive, the handler logic (especially GetAllTenantAccountHandler merging logic) and model changes require careful validation of pointer semantics, nil handling, and backward compatibility. The substantial cross-cutting impact on authorization flows and request validation demands careful review. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
/ok to test 0e16f06 |
🔐 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-02 00:04:04 UTC | Commit: 0e16f06 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/standard/client.go (1)
588-605:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFile closed before being read; multipart uploads will fail.
The refactored error handling closes the file at line 593, but
io.Copy(part, file)at line 602 attempts to read from the closed file descriptor. This will cause all multipart file uploads to fail with a "file already closed" or "bad file descriptor" error.🔧 Proposed fix: Move close after io.Copy
func addFile(w *multipart.Writer, fieldName, path string) error { file, err := os.Open(filepath.Clean(path)) if err != nil { return err } - err = file.Close() - if err != nil { - return err - } + defer file.Close() part, err := w.CreateFormFile(fieldName, filepath.Base(path)) if err != nil { return err } _, err = io.Copy(part, file) return err }🤖 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 `@sdk/standard/client.go` around lines 588 - 605, The file is being closed before it is read in addFile; move the file.Close so the file remains open for io.Copy(part, file) (e.g., open the file in addFile, defer file.Close() immediately after successful open or call Close after io.Copy completes) and ensure error handling still returns any errors from io.Copy, referencing the addFile function, the multipart.Writer usage, w.CreateFormFile, io.Copy, and file.Close.
🧹 Nitpick comments (2)
api/pkg/api/handler/tenantaccount_test.go (1)
780-941: ⚡ Quick winAdd a dual-role org-membership case for this auth refactor.
The updated handlers now depend on combined provider/tenant visibility, but the suite still only exercises single-role callers. Please add a case where the same caller is authorized through both paths, especially for
GetAll, so a regression back to first-match short-circuiting is caught. Based on learnings: "RBAC should not short-circuit on the first matching role ... Evaluate both authorization paths independently ... Add tests covering mixed-role scenarios ..."Also applies to: 1114-1497
🤖 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 `@api/pkg/api/handler/tenantaccount_test.go` around lines 780 - 941, Add a test case exercising a dual-role org membership (caller authorized as both provider and tenant) to the tests table used by the GetAll/Get handlers: create or use a user that has both provider and tenant roles (e.g., a new dualRoleUser composed from ipUser and tnUser roles), add an entry in the tests slice with that user, set reqOrgName to the org that gives both visibilities (e.g., ipOrg1/tnOrg1 as appropriate), provide the same expected success fields as the single-role success cases (expectedStatus http.StatusOK, expectedID ta11.ID.String(), expectedTenantOrg/expectedInfrastructureProviderOrg, expectedTenantContact, expectedAllocationCount, verifyChildSpanner where applicable), and replicate the same dual-role test addition in the other table blocks referenced (lines 1114-1497) so mixed-role authorization regressions are caught.cli/tui/commands.go (1)
2313-2321: ⚡ Quick winMake the org-based inference explicit in the prompt.
This flow intentionally omits
infrastructureProviderId, but the interactive text does not say that the provider is inferred from the current org. A short note here would make the changed workflow self-explanatory for operators.♻️ Proposed tweak
func cmdTenantAccountCreate(s *Session, _ []string) error { - tenantOrg, err := PromptText("Tenant org", true) + tenantOrg, err := PromptText("Tenant org (infrastructure provider inferred from current org)", true) if err != nil { return err }As per coding guidelines, "Document when you have intentionally omitted code that the reader might otherwise expect to be present".
🤖 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 `@cli/tui/commands.go` around lines 2313 - 2321, Update the interactive prompt in cmdTenantAccountCreate to explicitly state that the infrastructure provider is inferred from the current org: change the PromptText call and/or its prompt string (the "Tenant org" prompt passed to PromptText) to include a short note like "(infrastructure provider will be inferred from current org)" so operators know why infrastructureProviderId is omitted; keep the same trimming and LogCmd usage but ensure the new prompt text is used when constructing the body and logging.
🤖 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 `@api/pkg/api/handler/ipblock.go`:
- Around line 269-270: Update the Swagger param descriptions for the query
params infrastructureProviderId and tenantId in api/pkg/api/handler/ipblock.go
to state that these params are deprecated and are ignored for authorization
because the handler now derives Provider/Tenant context from the org claim
(i.e., authorization is based on org-derived context, not these query params);
adjust the same deprecated text for the other occurrence in this file (the other
param block) and ensure the OpenAPI schema files under the openapi/ directory
(and any api*.go swagger comments) are updated to reflect that org-based
inference is used instead of these query params.
In `@api/pkg/api/handler/tenantaccount.go`:
- Around line 399-413: The per-row call to aDAO.GetCount inside the loop causes
N DB round-trips; instead, collect all (InfrastructureProviderID, TenantID)
pairs for rows where TenantID != nil while iterating over tas, call a new or
existing batched DAO method (e.g., Add/GetCountsForTenantPairs or GetCounts(ctx,
nil, []AllocationFilterInput)) once to return a map keyed by
(infrastructure_provider_id,tenant_id) -> count, then in the loop look up
allocCount from that map and pass it to model.NewAPITenantAccount; update or add
the DAO method signature and its call-site to accept the slice of filter inputs
and return the counts mapping, and use ssdMap[ta.ID.String()] and
tmpTa.ID/tenant fields unchanged when mapping results back.
- Around line 340-372: The current handler materializes all matching
tenant-account IDs by calling taDAO.GetAll up to twice then merging IDs in
memory (mergedTenantAccountIDs), causing full scans before pagination; instead,
extend the DAO query to perform the provider/tenant union in SQL and call
taDAO.GetAll only once. Update the TenantAccountFilterInput (and its use in
taDAO.GetAll) to accept an InfrastructureProviderID and/or TenantID (or a
combined flag) and implement the SQL logic in the DAO to apply an OR/UNION
between provider-scoped and tenant-scoped rows so count and LIMIT/OFFSET are
executed by the database; remove the in-handler loops and mergedTenantAccountIDs
usage and pass the new combined filter into taDAO.GetAll with
pageRequest.ConvertToDB() and qIncludeRelations so pagination and total are
computed in SQL. Ensure existing callers of taDAO.GetAll remain compatible or
are updated to the new filter fields.
In `@openapi/spec.yaml`:
- Around line 536-540: The tenantId query parameter is declared as a scalar
string but the description says it may be supplied multiple times; update the
parameter definition to be an array: replace the scalar schema with schema:
type: array, items: { type: string, format: uuid } and add query serialization
metadata (e.g., style: form and explode: true) so repeated tenantId query params
are modeled correctly; locate the parameter by the name "tenantId" in the
OpenAPI parameter block and modify its schema and serialization settings
accordingly.
- Around line 13122-13126: The tenantOrg schema's regex pattern
'^[A-Za-z0-9-_]+$' can treat '-' as a range operator; update the pattern in
openapi/spec.yaml for the tenantOrg property to make the hyphen literal (e.g.,
move '-' to the end or escape it) so it becomes something like
'^[A-Za-z0-9_-]+$' ensuring only letters, digits, underscore, and hyphen are
allowed.
In `@sdk/standard/api_site.go`:
- Around line 304-312: The generated setter doc comments emit a standalone "//
Deprecated" line which Go tooling ignores; update the OpenAPI
generator/template/spec so the doc-comment paragraph for setters like
ApiGetAllSiteRequest.InfrastructureProviderId (and the Tenant ID setter) begins
with "Deprecated:" followed by the message (e.g., "Deprecated: Infrastructure
Provider is now inferred...") instead of a separate "// Deprecated" line; modify
the template that renders method comments to prepend "Deprecated: " to the
deprecation sentence so the full paragraph starts with "Deprecated:".
In `@sdk/standard/compat/instance_create.go`:
- Around line 19-50: The compat layer is missing shims for the removed/changed
exported constructors (notably NewTenantAccountCreateRequest) so callers relying
on old arities break; add deprecated wrapper functions in sdk/standard/compat
that preserve the old signatures and delegate to the new constructors (similar
to NewInstanceCreateRequestWithInterfaces and
NewBatchInstanceCreateRequestWithInterfaces): implement
NewTenantAccountCreateRequest(oldTenantOrg string) that calls the new
standard.NewTenantAccountCreateRequest(...) API or constructs a
standard.TenantAccountCreateRequest and sets fields as needed, and if you
removed any other overloaded constructors restore them as deprecated wrappers
that call standard.NewInstanceCreateRequest,
standard.NewBatchInstanceCreateRequest, etc., and apply any missing SetX methods
(e.g., SetTenantId, SetInterfaces) so existing callers continue to work while
marking these shims deprecated.
---
Outside diff comments:
In `@sdk/standard/client.go`:
- Around line 588-605: The file is being closed before it is read in addFile;
move the file.Close so the file remains open for io.Copy(part, file) (e.g., open
the file in addFile, defer file.Close() immediately after successful open or
call Close after io.Copy completes) and ensure error handling still returns any
errors from io.Copy, referencing the addFile function, the multipart.Writer
usage, w.CreateFormFile, io.Copy, and file.Close.
---
Nitpick comments:
In `@api/pkg/api/handler/tenantaccount_test.go`:
- Around line 780-941: Add a test case exercising a dual-role org membership
(caller authorized as both provider and tenant) to the tests table used by the
GetAll/Get handlers: create or use a user that has both provider and tenant
roles (e.g., a new dualRoleUser composed from ipUser and tnUser roles), add an
entry in the tests slice with that user, set reqOrgName to the org that gives
both visibilities (e.g., ipOrg1/tnOrg1 as appropriate), provide the same
expected success fields as the single-role success cases (expectedStatus
http.StatusOK, expectedID ta11.ID.String(),
expectedTenantOrg/expectedInfrastructureProviderOrg, expectedTenantContact,
expectedAllocationCount, verifyChildSpanner where applicable), and replicate the
same dual-role test addition in the other table blocks referenced (lines
1114-1497) so mixed-role authorization regressions are caught.
In `@cli/tui/commands.go`:
- Around line 2313-2321: Update the interactive prompt in cmdTenantAccountCreate
to explicitly state that the infrastructure provider is inferred from the
current org: change the PromptText call and/or its prompt string (the "Tenant
org" prompt passed to PromptText) to include a short note like "(infrastructure
provider will be inferred from current org)" so operators know why
infrastructureProviderId is omitted; keep the same trimming and LogCmd usage but
ensure the new prompt text is used when constructing the body and logging.
🪄 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: 360f474d-add4-4dc8-b414-0f27673e2d27
📒 Files selected for processing (60)
api/pkg/api/handler/ipblock.goapi/pkg/api/handler/tenantaccount.goapi/pkg/api/handler/tenantaccount_test.goapi/pkg/api/model/tenantaccount.goapi/pkg/api/model/tenantaccount_test.gocli/tui/commands.godb/pkg/db/model/tenantaccount.godocs/index.htmlopenapi/spec.yamlsdk/standard/api_allocation.gosdk/standard/api_audit.gosdk/standard/api_dpu_extension_service.gosdk/standard/api_expected_machine.gosdk/standard/api_expected_power_shelf.gosdk/standard/api_expected_rack.gosdk/standard/api_expected_switch.gosdk/standard/api_infini_band_partition.gosdk/standard/api_instance.gosdk/standard/api_instance_type.gosdk/standard/api_ip_block.gosdk/standard/api_machine.gosdk/standard/api_network_security_group.gosdk/standard/api_nv_link_logical_partition.gosdk/standard/api_operating_system.gosdk/standard/api_rack.gosdk/standard/api_site.gosdk/standard/api_sku.gosdk/standard/api_ssh_key.gosdk/standard/api_ssh_key_group.gosdk/standard/api_subnet.gosdk/standard/api_tenant_account.gosdk/standard/api_tray.gosdk/standard/api_vpc.gosdk/standard/api_vpc_peering.gosdk/standard/api_vpc_prefix.gosdk/standard/client.gosdk/standard/compat/instance_create.gosdk/standard/model_batch_instance_create_request.gosdk/standard/model_expected_machine.gosdk/standard/model_expected_machine_create_request.gosdk/standard/model_expected_machine_update_request.gosdk/standard/model_expected_power_shelf.gosdk/standard/model_expected_power_shelf_create_request.gosdk/standard/model_expected_power_shelf_update_request.gosdk/standard/model_expected_rack.gosdk/standard/model_expected_rack_create_request.gosdk/standard/model_expected_rack_update_request.gosdk/standard/model_expected_switch.gosdk/standard/model_expected_switch_create_request.gosdk/standard/model_expected_switch_update_request.gosdk/standard/model_infini_band_partition.gosdk/standard/model_infini_band_partition_create_request.gosdk/standard/model_infini_band_partition_update_request.gosdk/standard/model_instance_create_request.gosdk/standard/model_instance_update_request.gosdk/standard/model_machine_update_request.gosdk/standard/model_tenant_account_create_request.gosdk/standard/model_vpc.gosdk/standard/model_vpc_create_request.gosdk/standard/model_vpc_update_request.go
💤 Files with no reviewable changes (21)
- sdk/standard/api_expected_power_shelf.go
- sdk/standard/api_operating_system.go
- sdk/standard/api_expected_machine.go
- sdk/standard/api_dpu_extension_service.go
- sdk/standard/api_ssh_key.go
- sdk/standard/api_vpc.go
- sdk/standard/api_nv_link_logical_partition.go
- sdk/standard/api_expected_switch.go
- sdk/standard/api_sku.go
- sdk/standard/api_vpc_prefix.go
- sdk/standard/api_rack.go
- sdk/standard/api_ssh_key_group.go
- sdk/standard/api_vpc_peering.go
- sdk/standard/api_infini_band_partition.go
- sdk/standard/api_expected_rack.go
- sdk/standard/api_machine.go
- sdk/standard/api_subnet.go
- sdk/standard/api_tray.go
- sdk/standard/api_network_security_group.go
- sdk/standard/api_instance.go
- sdk/standard/api_audit.go
Comment on lines
+269
to
+270
| // @Param infrastructureProviderId query string false "Deprecated: ID of Infrastructure Provider" | ||
| // @Param tenantId query string false "Deprecated: ID of Tenant" |
Contributor
There was a problem hiding this comment.
Document org-based inference in these deprecated query params.
These descriptions only say the params are deprecated, but the handler now derives Provider/Tenant context from org and ignores these query params for authorization. Please spell that out here as well so the generated Swagger is unambiguous.
As per coding guidelines, {**/*api*.go,openapi/**/*.{yml,yaml,json}}: Update OpenAPI schema whenever API endpoints are added or updated.
Also applies to: 747-748
🤖 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 `@api/pkg/api/handler/ipblock.go` around lines 269 - 270, Update the Swagger
param descriptions for the query params infrastructureProviderId and tenantId in
api/pkg/api/handler/ipblock.go to state that these params are deprecated and are
ignored for authorization because the handler now derives Provider/Tenant
context from the org claim (i.e., authorization is based on org-derived context,
not these query params); adjust the same deprecated text for the other
occurrence in this file (the other param block) and ensure the OpenAPI schema
files under the openapi/ directory (and any api*.go swagger comments) are
updated to reflect that org-based inference is used instead of these query
params.
Comment on lines
+340
to
+372
| mergedTenantAccountIDs := mapset.NewSet[uuid.UUID]() | ||
| totalLimitPage := cdbp.PageInput{Limit: cdb.GetIntPtr(cdbp.TotalLimit)} | ||
|
|
||
| if provider != nil { | ||
| providerFilter := sharedFilter | ||
| providerFilter.InfrastructureProviderID = &provider.ID | ||
| providerFilter.TenantIDs = filterTenantIDs | ||
| tenantAccountsFromProviderPerspective, _, err := taDAO.GetAll(ctx, nil, providerFilter, totalLimitPage, nil) | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("error getting TenantAccounts from Provider perspective") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve TenantAccounts, DB error", nil) | ||
| } | ||
| for _, tenantAccount := range tenantAccountsFromProviderPerspective { | ||
| mergedTenantAccountIDs.Add(tenantAccount.ID) | ||
| } | ||
| } | ||
|
|
||
| filter := cdbm.TenantAccountFilterInput{ | ||
| InfrastructureProviderID: infrastructureProviderID, | ||
| TenantIDs: tenantIDs, | ||
| Statuses: statuses, | ||
| SearchQuery: searchQuery, | ||
| if tenant != nil { | ||
| tenantFilter := sharedFilter | ||
| tenantFilter.TenantIDs = []uuid.UUID{tenant.ID} | ||
| tenantAccountsFromTenantPerspective, _, err := taDAO.GetAll(ctx, nil, tenantFilter, totalLimitPage, nil) | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("error getting TenantAccounts from Tenant perspective") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve TenantAccounts, DB error", nil) | ||
| } | ||
| for _, tenantAccount := range tenantAccountsFromTenantPerspective { | ||
| mergedTenantAccountIDs.Add(tenantAccount.ID) | ||
| } | ||
| } | ||
|
|
||
| tas, total, err := taDAO.GetAll(ctx, nil, filter, cdbp.PageInput{ | ||
| Offset: pageRequest.Offset, | ||
| Limit: pageRequest.Limit, | ||
| OrderBy: pageRequest.OrderBy, | ||
| }, qIncludeRelations) | ||
| tas, total, err := taDAO.GetAll(ctx, nil, cdbm.TenantAccountFilterInput{ | ||
| TenantAccountIDs: mergedTenantAccountIDs.ToSlice(), | ||
| }, pageRequest.ConvertToDB(), qIncludeRelations) |
Contributor
There was a problem hiding this comment.
Push the provider/tenant union into SQL before paginating.
This path now does up to three GetAll calls and materializes every matching tenant-account ID before the page limit is applied. Large orgs will pay the full scan and memory cost even for page 1. Please move the provider/tenant union into the DAO query so count and pagination happen in SQL.
🤖 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 `@api/pkg/api/handler/tenantaccount.go` around lines 340 - 372, The current
handler materializes all matching tenant-account IDs by calling taDAO.GetAll up
to twice then merging IDs in memory (mergedTenantAccountIDs), causing full scans
before pagination; instead, extend the DAO query to perform the provider/tenant
union in SQL and call taDAO.GetAll only once. Update the
TenantAccountFilterInput (and its use in taDAO.GetAll) to accept an
InfrastructureProviderID and/or TenantID (or a combined flag) and implement the
SQL logic in the DAO to apply an OR/UNION between provider-scoped and
tenant-scoped rows so count and LIMIT/OFFSET are executed by the database;
remove the in-handler loops and mergedTenantAccountIDs usage and pass the new
combined filter into taDAO.GetAll with pageRequest.ConvertToDB() and
qIncludeRelations so pagination and total are computed in SQL. Ensure existing
callers of taDAO.GetAll remain compatible or are updated to the new filter
fields.
Comment on lines
399
to
+413
| for _, ta := range tas { | ||
| tmpTa := ta | ||
| // Check for Allocation for Tenant | ||
| total := 0 | ||
| allocCount := 0 | ||
| if tmpTa.TenantID != nil { | ||
| total = allocationCountByTenantIDMap[*tmpTa.TenantID] | ||
| cnt, cerr := aDAO.GetCount(ctx, nil, cdbm.AllocationFilterInput{ | ||
| InfrastructureProviderID: &tmpTa.InfrastructureProviderID, | ||
| TenantIDs: []uuid.UUID{*tmpTa.TenantID}, | ||
| }) | ||
| if cerr != nil { | ||
| logger.Error().Err(cerr).Msg("error retrieving allocation count for Tenant Account from DB") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve Allocations to determine total Allocation count for Tenants", nil) | ||
| } | ||
| allocCount = cnt | ||
| } | ||
| apiTa := model.NewAPITenantAccount(&tmpTa, ssdMap[ta.ID.String()], total) | ||
| apiTa := model.NewAPITenantAccount(&tmpTa, ssdMap[ta.ID.String()], allocCount) |
Contributor
There was a problem hiding this comment.
Avoid the per-row allocation count query.
AllocationDAO.GetCount(...) inside the loop turns one list request into 1+N database round trips. That will scale poorly as page size grows. Please batch these counts in one DAO call keyed by (infrastructure_provider_id, tenant_id) and map them back in memory.
🤖 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 `@api/pkg/api/handler/tenantaccount.go` around lines 399 - 413, The per-row
call to aDAO.GetCount inside the loop causes N DB round-trips; instead, collect
all (InfrastructureProviderID, TenantID) pairs for rows where TenantID != nil
while iterating over tas, call a new or existing batched DAO method (e.g.,
Add/GetCountsForTenantPairs or GetCounts(ctx, nil, []AllocationFilterInput))
once to return a map keyed by (infrastructure_provider_id,tenant_id) -> count,
then in the loop look up allocCount from that map and pass it to
model.NewAPITenantAccount; update or add the DAO method signature and its
call-site to accept the slice of filter inputs and return the counts mapping,
and use ssdMap[ta.ID.String()] and tmpTa.ID/tenant fields unchanged when mapping
results back.
Comment on lines
+536
to
+540
| type: string | ||
| format: uuid | ||
| in: query | ||
| name: tenantId | ||
| description: Filter TenantAccounts by Tenant ID | ||
| description: 'Filter TenantAccounts by Tenant ID (Provider role only; for Tenant role the tenant is inferred from org membership and this param is ignored). Can be specified multiple times to filter on more than one Tenant ID.' |
Contributor
There was a problem hiding this comment.
Model tenantId as an array if it can be repeated.
The description says this query parameter may be supplied multiple times, but the schema is still a scalar string. That will generate incorrect SDKs/docs for the provider-side narrowing flow.
Suggested OpenAPI fix
- schema:
- type: string
- format: uuid
+ type: array
+ items:
+ type: string
+ format: uuid
in: query
name: tenantId
+ style: form
+ explode: true
description: 'Filter TenantAccounts by Tenant ID (Provider role only; for Tenant role the tenant is inferred from org membership and this param is ignored). Can be specified multiple times to filter on more than one Tenant ID.'📝 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.
Suggested change
| type: string | |
| format: uuid | |
| in: query | |
| name: tenantId | |
| description: Filter TenantAccounts by Tenant ID | |
| description: 'Filter TenantAccounts by Tenant ID (Provider role only; for Tenant role the tenant is inferred from org membership and this param is ignored). Can be specified multiple times to filter on more than one Tenant ID.' | |
| type: array | |
| items: | |
| type: string | |
| format: uuid | |
| in: query | |
| name: tenantId | |
| style: form | |
| explode: true | |
| description: 'Filter TenantAccounts by Tenant ID (Provider role only; for Tenant role the tenant is inferred from org membership and this param is ignored). Can be specified multiple times to filter on more than one Tenant ID.' |
🤖 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 `@openapi/spec.yaml` around lines 536 - 540, The tenantId query parameter is
declared as a scalar string but the description says it may be supplied multiple
times; update the parameter definition to be an array: replace the scalar schema
with schema: type: array, items: { type: string, format: uuid } and add query
serialization metadata (e.g., style: form and explode: true) so repeated
tenantId query params are modeled correctly; locate the parameter by the name
"tenantId" in the OpenAPI parameter block and modify its schema and
serialization settings accordingly.
Comment on lines
+304
to
+312
| // Filter Sites by Infrastructure Provider ID. Deprecated: Infrastructure Provider is now inferred from the org's membership. | ||
| // Deprecated | ||
| func (r ApiGetAllSiteRequest) InfrastructureProviderId(infrastructureProviderId string) ApiGetAllSiteRequest { | ||
| r.infrastructureProviderId = &infrastructureProviderId | ||
| return r | ||
| } | ||
|
|
||
| // Filter Sites by Tenant ID | ||
| // Filter Sites by Tenant ID. Deprecated: Tenant is now inferred from the org's membership. | ||
| // Deprecated |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Go doc comments, does a deprecated symbol need a paragraph beginning with Deprecated:for tools like godoc/pkg.go.dev/gopls to recognize it, or is// Deprecated without the colon sufficient?
💡 Result:
For Go doc comments, the deprecation convention recognized by Go doc tooling is to include a paragraph that begins exactly with Deprecated: (with the colon). Go’s official doc comment guidelines state: “To signal that an identifier should not be used, add a paragraph to its doc comment that begins with Deprecated: …” [1]. The same convention is reiterated in the Go wiki: “add a paragraph … that begins with Deprecated:” [2]. Therefore, // Deprecated without the colon is not the stated convention; to ensure tools recognize the deprecation notice, use // Deprecated: ... as the first text of a paragraph in the doc comment [1][2].
Citations:
Fix Go deprecation doc comments in generated setters
In sdk/standard/api_site.go (lines ~304-312), the doc comments emit a standalone // Deprecated line. Go doc tooling recognizes deprecations only when a doc-comment paragraph begins with Deprecated: (with the colon). Update the OpenAPI generator/template/spec so these setters render Deprecated: ... rather than // Deprecated.
🤖 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 `@sdk/standard/api_site.go` around lines 304 - 312, The generated setter doc
comments emit a standalone "// Deprecated" line which Go tooling ignores; update
the OpenAPI generator/template/spec so the doc-comment paragraph for setters
like ApiGetAllSiteRequest.InfrastructureProviderId (and the Tenant ID setter)
begins with "Deprecated:" followed by the message (e.g., "Deprecated:
Infrastructure Provider is now inferred...") instead of a separate "//
Deprecated" line; modify the template that renders method comments to prepend
"Deprecated: " to the deprecation sentence so the full paragraph starts with
"Deprecated:".
Comment on lines
19
to
50
| // NewInstanceCreateRequestWithInterfaces preserves the pre-`auto` | ||
| // `NewInstanceCreateRequest` signature, which took an explicit | ||
| // `interfaces` argument before that field became optional on the | ||
| // `NewInstanceCreateRequest` signature, which took explicit `tenantId` | ||
| // and `interfaces` arguments before those fields became optional on the | ||
| // REST API. Callers should migrate to | ||
| // `standard.NewInstanceCreateRequest(name, tenantId, vpcId)` followed | ||
| // by `SetInterfaces` (or `SetAutoNetwork` for zero-DPU instances). | ||
| // `standard.NewInstanceCreateRequest(name, vpcId)`, optionally | ||
| // `SetTenantId`, then `SetInterfaces` (or `SetAutoNetwork` for zero-DPU | ||
| // instances). | ||
| // | ||
| // Deprecated: use `standard.NewInstanceCreateRequest` + `SetInterfaces`. | ||
| func NewInstanceCreateRequestWithInterfaces(name, tenantID, vpcID string, interfaces []standard.InterfaceCreateRequest) *standard.InstanceCreateRequest { | ||
| req := standard.NewInstanceCreateRequest(name, tenantID, vpcID) | ||
| req := standard.NewInstanceCreateRequest(name, vpcID) | ||
| if tenantID != "" { | ||
| req.SetTenantId(tenantID) | ||
| } | ||
| req.SetInterfaces(interfaces) | ||
| return req | ||
| } | ||
|
|
||
| // NewBatchInstanceCreateRequestWithInterfaces preserves the pre-`auto` | ||
| // `NewBatchInstanceCreateRequest` signature, which took an explicit | ||
| // `interfaces` argument before that field became optional. Callers | ||
| // should migrate to `standard.NewBatchInstanceCreateRequest(...)` | ||
| // followed by `SetInterfaces` (or `SetAutoNetwork` for zero-DPU batches). | ||
| // `NewBatchInstanceCreateRequest` signature, which took explicit `tenantId` | ||
| // and `interfaces` arguments before those fields became optional. Callers | ||
| // should migrate to `standard.NewBatchInstanceCreateRequest(...)`, optionally | ||
| // `SetTenantId`, then `SetInterfaces` (or `SetAutoNetwork` for zero-DPU batches). | ||
| // | ||
| // Deprecated: use `standard.NewBatchInstanceCreateRequest` + `SetInterfaces`. | ||
| func NewBatchInstanceCreateRequestWithInterfaces(namePrefix string, count int32, tenantID, instanceTypeID, vpcID string, interfaces []standard.InterfaceCreateRequest) *standard.BatchInstanceCreateRequest { | ||
| req := standard.NewBatchInstanceCreateRequest(namePrefix, count, tenantID, instanceTypeID, vpcID) | ||
| req := standard.NewBatchInstanceCreateRequest(namePrefix, count, instanceTypeID, vpcID) | ||
| if tenantID != "" { | ||
| req.SetTenantId(tenantID) | ||
| } | ||
| req.SetInterfaces(interfaces) | ||
| return req |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Generated exported constructors:\n'
rg -n '^func New(TenantAccountCreateRequest|InstanceCreateRequest|BatchInstanceCreateRequest)\(' sdk/standard
printf '\nCompat constructors present in this PR:\n'
rg -n '^func New.*CreateRequest' sdk/standard/compatRepository: NVIDIA/infra-controller-rest
Length of output: 1048
Major: sdk/standard/compat shims don’t cover the exported constructor breaking arity changes
The exported constructors in sdk/standard now have reduced signatures:
NewTenantAccountCreateRequest(tenantOrg string)(sdk/standard/model_tenant_account_create_request.go)NewInstanceCreateRequest(name string, vpcId string)(sdk/standard/model_instance_create_request.go)NewBatchInstanceCreateRequest(namePrefix string, count int32, instanceTypeId string, vpcId string)(sdk/standard/model_batch_instance_create_request.go)
But sdk/standard/compat only adds NewInstanceCreateRequestWithInterfaces and NewBatchInstanceCreateRequestWithInterfaces (sdk/standard/compat/instance_create.go), with no tenant-account compat shim and no adapter that preserves the old call patterns for standard.New*CreateRequest without forcing callers onto the *WithInterfaces functions.
If this is not an intentional breaking change, restore backward compatibility by keeping the old exported constructor signatures (deprecated) and adding the new ones; otherwise, provide a complete, documented migration/versioning plan.
🤖 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 `@sdk/standard/compat/instance_create.go` around lines 19 - 50, The compat
layer is missing shims for the removed/changed exported constructors (notably
NewTenantAccountCreateRequest) so callers relying on old arities break; add
deprecated wrapper functions in sdk/standard/compat that preserve the old
signatures and delegate to the new constructors (similar to
NewInstanceCreateRequestWithInterfaces and
NewBatchInstanceCreateRequestWithInterfaces): implement
NewTenantAccountCreateRequest(oldTenantOrg string) that calls the new
standard.NewTenantAccountCreateRequest(...) API or constructs a
standard.TenantAccountCreateRequest and sets fields as needed, and if you
removed any other overloaded constructors restore them as deprecated wrappers
that call standard.NewInstanceCreateRequest,
standard.NewBatchInstanceCreateRequest, etc., and apply any missing SetX methods
(e.g., SetTenantId, SetInterfaces) so existing callers continue to work while
marking these shims deprecated.
…andlers Signed-off-by: Parham Armani <parmani@nvidia.com>
parmani-nv
force-pushed
the
feat/tenant-handler-inferId
branch
from
0e16f06 to
49ee436
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…andlers
Description
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes