feat(rest-api): Add SKU management endpoints - #3633
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds REST API support for creating, updating, and deleting SKUs through validated request models, Core-backed handlers, route registration, OpenAPI contracts, component mappings, authorization checks, projection reconciliation, and tests. ChangesSKU lifecycle operations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SKUHandlers
participant Core
participant RESTDatabase
Client->>SKUHandlers: Submit SKU mutation
SKUHandlers->>SKUHandlers: Validate request and authorize Site
SKUHandlers->>Core: Create, update, or delete SKU
Core-->>SKUHandlers: Return mutation result
SKUHandlers->>RESTDatabase: Reconcile or remove SKU projection
RESTDatabase-->>SKUHandlers: Persist projection result
SKUHandlers-->>Client: Return HTTP 201, 200, or 204
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3633.docs.buildwithfern.com/infra-controller |
🔐 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-07-20 15:02:10 UTC | Commit: fafa06e |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rest-api/openapi/spec.yaml (1)
167-171: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale tag description now contradicts the new SKU mutation endpoints.
This description still asserts
"SKUs are automatically derived from machine hardware characteristics and used to group similar machines. SKUs are read-only and managed by the system."This PR addsPOST,PATCH, andDELETEunder theSKUtag (create-sku, update-sku, delete-sku), so the tag-level claim of "read-only" is now inaccurate and will mislead SDK consumers and documentation readers about the resource's actual mutability.📝 Proposed fix
- name: SKU description: |- SKU (Stock Keeping Unit) defines one or more hardware configurations or Machine Bill of Materials (BOM). - SKUs are automatically derived from machine hardware characteristics and used to group similar machines. SKUs are read-only and managed by the system. + SKUs are automatically derived from machine hardware characteristics and used to group similar machines. Most SKUs are managed by the system, but Provider Admins can create, update, and delete SKUs on a Site's NICo Core service via the mutation endpoints below.As per path instructions, "Review OpenAPI docs and examples for accuracy, deprecation clarity, client-facing compatibility, spelling, and consistency with
spec.yaml."🤖 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` around lines 167 - 171, Update the SKU tag description in spec.yaml to remove the claim that SKUs are read-only and system-managed, and revise it to accurately reflect that SKUs can be created, updated, and deleted through the documented mutation endpoints while preserving the valid description of their purpose.Source: Path instructions
🧹 Nitpick comments (4)
rest-api/api/pkg/api/handler/sku_management_test.go (1)
55-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for update/delete against a non-existent SKU ID.
No test exercises the 404 path through
findSkuByIDViaCoreforUpdateSkuHandleror the equivalent not-found path forDeleteSkuHandler. Given the OpenAPI contract explicitly documents404 -> NotFoundErrorfor both operations, this is a critical path worth locking down with a regression 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/handler/sku_management_test.go` around lines 55 - 97, Add regression tests alongside TestUpdateSkuHandler_MergesPatchBeforeReplace and TestDeleteSkuHandler_ProxiesDelete that invoke each handler with a non-existent SKU ID, assert HTTP 404, and verify the expected not-found error response without issuing the update or delete core request. Reuse the existing newSkuManagementFixture and request helpers.rest-api/api/pkg/api/model/sku.go (1)
106-122: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRefactor
APISkuUpdateRequest.Validate()to compose via ozzo instead of early-returning.The manual
ifchecks bypassValidateStruct's error aggregation: ifSiteIDis invalid andSchemaVersion == 0, only the schemaVersion error is returned to the client — the SiteID error is silently swallowed by the earlyreturn. As per path instructions, "validation should prefer ozzo-validation built-in rules and composition over reinvented custom validation helpers," and as per coding guidelines, cross-field checks should use named receiver methods withvalidation.By.♻️ Suggested refactor
func (r APISkuUpdateRequest) Validate() error { - if r.SchemaVersion != nil && *r.SchemaVersion == 0 { - return validation.Errors{"schemaVersion": validation.NewError("validation_min", "schemaVersion must be greater than zero")} - } - if err := validation.ValidateStruct(&r, + return validation.ValidateStruct(&r, validation.Field(&r.SiteID, validation.Required.Error(validationErrorValueRequired), validationis.UUID.Error(validationErrorInvalidUUID)), - ); err != nil { - return err - } - if r.Description == nil && r.SchemaVersion == nil && r.DeviceType == nil && r.Components == nil { - return validation.Errors{"request": validation.NewError("validation_required", "at least one mutable field is required")} - } - return nil + validation.Field(&r.SchemaVersion, validation.By(r.validateSchemaVersion)), + validation.Field(&r, validation.By(r.validateHasMutableField)), + ) +} + +func (r APISkuUpdateRequest) validateSchemaVersion(value interface{}) error { + v, _ := value.(*uint32) + if v != nil && *v == 0 { + return validation.NewError("validation_min", "schemaVersion must be greater than zero") + } + return nil +} + +func (r APISkuUpdateRequest) validateHasMutableField(interface{}) error { + if r.Description == nil && r.SchemaVersion == nil && r.DeviceType == nil && r.Components == nil { + return validation.NewError("validation_required", "at least one mutable field is required") + } + return nil }🤖 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/sku.go` around lines 106 - 122, Refactor APISkuUpdateRequest.Validate to compose all checks through ozzo-validation so schemaVersion, SiteID, and mutable-field requirements are aggregated instead of returned early. Move the cross-field rules into named receiver methods and attach them with validation.By, while preserving the existing validation messages and requiring schemaVersion to be greater than zero and at least one mutable field.Sources: Coding guidelines, Path instructions
rest-api/openapi/spec.yaml (2)
22656-22699: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
SkuMutationResponseomitsupdated, unlike the siblingSkuschema.
Sku(line ~22578) exposes bothcreatedandupdatedas read-only timestamps, butSkuMutationResponseonly carries a nullablecreated. For theupdate-sku(PATCH) response in particular, callers have no way to confirm the Core-side update timestamp from the response body — they'd need a follow-upGETjust to seeupdated.Consider adding a nullable
updatedfield toSkuMutationResponsefor parity withSkuand to spare clients an extra round-trip afterPATCH.🤖 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` around lines 22656 - 22699, Add a nullable date-time updated property to the SkuMutationResponse schema, matching the existing updated field definition and read-only semantics from the sibling Sku schema. Keep it optional, consistent with created, and do not alter the existing required fields.
4637-4662: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
examples:for the new SKU mutation contracts.None of the new
SkuCreateRequest,SkuUpdateRequest,SkuDeleteRequest, orSkuMutationResponseschemas — nor thecreate-sku/update-sku/delete-skurequest bodies — include anexamples:block. Virtually every other mutation schema andrequestBodyin this file (e.g.,TenantAccountCreateRequest,SiteCreateRequest,HostFirmwareConfigCreateOrUpdateRequest) includes at least one example. This inconsistency degrades generated SDK docs and Redoc/Swagger UI previews for the new endpoints.Please add representative
examples:blocks to the four new schemas (or at minimum to therequestBody/responsesin the three new operations), consistent with the rest of the file.As per path instructions, when model attributes are added "verify the OpenAPI spec is updated with matching schema, required/nullable semantics, and examples when applicable."
Also applies to: 4780-4831, 22586-22699
🤖 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` around lines 4637 - 4662, Add representative examples for the new SKU mutation contracts: SkuCreateRequest, SkuUpdateRequest, SkuDeleteRequest, and SkuMutationResponse, and ensure the create-sku, update-sku, and delete-sku request/response definitions expose them where needed. Match the existing schema property names, required fields, and nullable semantics, following the example structure used by nearby mutation contracts.Source: Path instructions
🤖 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/handler/sku_management.go`:
- Around line 73-88: The post-create lookup failure in the handler currently
turns a successful Core mutation into an error response. Update the
findSkuByIDViaCore failure path to return a successful creation response built
from the validated apiReq and returned ids.Ids[0], while preserving the existing
fetched-SKU response when lookup succeeds and the existing create-call error
handling.
- Around line 141-152: Update UpdateSkuHandler’s read-modify-replace flow to
carry the version observed in current as an explicit compare-and-swap
precondition on updatedReq or the Core request, rather than relying on
schema_version in the mutable payload. Ensure ExecuteCoreGRPC propagates that
precondition so Core rejects stale concurrent updates, preserving the existing
API error handling for rejected replacements.
---
Outside diff comments:
In `@rest-api/openapi/spec.yaml`:
- Around line 167-171: Update the SKU tag description in spec.yaml to remove the
claim that SKUs are read-only and system-managed, and revise it to accurately
reflect that SKUs can be created, updated, and deleted through the documented
mutation endpoints while preserving the valid description of their purpose.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/sku_management_test.go`:
- Around line 55-97: Add regression tests alongside
TestUpdateSkuHandler_MergesPatchBeforeReplace and
TestDeleteSkuHandler_ProxiesDelete that invoke each handler with a non-existent
SKU ID, assert HTTP 404, and verify the expected not-found error response
without issuing the update or delete core request. Reuse the existing
newSkuManagementFixture and request helpers.
In `@rest-api/api/pkg/api/model/sku.go`:
- Around line 106-122: Refactor APISkuUpdateRequest.Validate to compose all
checks through ozzo-validation so schemaVersion, SiteID, and mutable-field
requirements are aggregated instead of returned early. Move the cross-field
rules into named receiver methods and attach them with validation.By, while
preserving the existing validation messages and requiring schemaVersion to be
greater than zero and at least one mutable field.
In `@rest-api/openapi/spec.yaml`:
- Around line 22656-22699: Add a nullable date-time updated property to the
SkuMutationResponse schema, matching the existing updated field definition and
read-only semantics from the sibling Sku schema. Keep it optional, consistent
with created, and do not alter the existing required fields.
- Around line 4637-4662: Add representative examples for the new SKU mutation
contracts: SkuCreateRequest, SkuUpdateRequest, SkuDeleteRequest, and
SkuMutationResponse, and ensure the create-sku, update-sku, and delete-sku
request/response definitions expose them where needed. Match the existing schema
property names, required fields, and nullable semantics, following the example
structure used by nearby mutation contracts.
🪄 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: 034348da-93bf-47db-9932-3b6a9b36c675
⛔ Files ignored due to path filters (9)
rest-api/sdk/standard/api_sku.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/model_sku_chassis.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_sku_components.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_sku_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_sku_delete_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_sku_ethernet_device.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_sku_infiniband_device.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_sku_mutation_response.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_sku_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (7)
rest-api/api/pkg/api/handler/sku_management.gorest-api/api/pkg/api/handler/sku_management_test.gorest-api/api/pkg/api/model/sku.gorest-api/api/pkg/api/model/sku_management_test.gorest-api/api/pkg/api/routes.gorest-api/api/pkg/api/routes_test.gorest-api/openapi/spec.yaml
|
@nvlitagaki moved the PR description text to this comment since it doesn't really need to show up in the final commit message: VerificationHow we verified itVerified revision The default local API image contained an amd64 Go binary despite being tagged as arm64 because the local Dockerfile defaults Hands-on results through REST → Temporal → Site Agent → real Core:
Supporting check: How to reproduce the verificationPrerequisites:
git checkout --detach 3e1fdcbdbea4fcb42969396499adfbe195aabbab
cd rest-api
make ensure-postgres
docker build --platform linux/arm64 \
--build-arg TARGETOS=linux \
--build-arg TARGETARCH=arm64 \
--build-arg VERSION=3e1fdcbd \
-t localhost:5000/nico-rest-api:sku-rest-arm64 \
-f docker/local/Dockerfile.nico-rest-api .
kind load docker-image localhost:5000/nico-rest-api:sku-rest-arm64 --name nico-rest-local
kubectl --context kind-nico-rest-local -n nico-rest \
set image deployment/nico-rest-api api=localhost:5000/nico-rest-api:sku-rest-arm64
kubectl --context kind-nico-rest-local -n nico-rest \
rollout status deployment/nico-rest-api --timeout=240s
KUBECONFIG=/tmp/do-work-kind-nico-rest-local.kubeconfig \
make configure-local-core-site-agent
kubectl --context kind-nico-rest-local -n nico-rest \
rollout status statefulset/nico-rest-site-agent --timeout=240s
kubectl --context kind-nico-rest-local -n nico-rest \
logs statefulset/nico-rest-site-agent --tail=100 | grep 'forge.Forge/Version.*code=Ok'Expected checkpoint: the Site Agent reports a successful Core
TOKEN="$(curl -fsS -X POST http://localhost:8082/realms/nico/protocol/openid-connect/token \
-H 'Content-Type: application/x-www-form-urlencoded' \
--data-urlencode 'client_id=nico-rest-api' \
--data-urlencode 'username=admin@example.com' \
--data-urlencode 'password=adminpassword' \
--data-urlencode 'grant_type=password' | jq -r .access_token)"
SITE_ID="$(curl -fsS http://localhost:8388/v2/org/test-org/nico/site \
-H "Authorization: Bearer $TOKEN" | jq -r '.[0].id')"
SKU_ID="sku-rest-verification-$(date +%s)"
curl -i -X POST http://localhost:8388/v2/org/test-org/nico/sku \
-H "Authorization: Bearer $TOKEN" \
-H 'Content-Type: application/json' \
--data "$(jq -n --arg id "$SKU_ID" --arg site "$SITE_ID" '{
id: $id,
siteId: $site,
description: "REST Core verification",
schemaVersion: 5,
deviceType: "verification-node",
storage: [{
model: "TEST-NVME",
count: 2,
minSizeMb: 3600000,
maxSizeMb: 3900000,
pciPatterns: ["^/devices/pci.*nvme[0-1]$"]
}],
chassis: {
manufacturer: "NVIDIA",
model: "Verification Chassis",
architecture: "aarch64"
}
}')"Expected checkpoint: HTTP 201 and a response containing the same v5 storage and chassis fields.
curl -i -X PATCH "http://localhost:8388/v2/org/test-org/nico/sku/$SKU_ID?siteId=$SITE_ID" \
-H "Authorization: Bearer $TOKEN" \
-H 'Content-Type: application/json' \
--data '{"description":"REST Core verification updated"}'Expected checkpoint: HTTP 200; only the description changes.
curl -i -X POST http://localhost:8388/v2/org/test-org/nico/sku \
-H "Authorization: Bearer $TOKEN" \
-H 'Content-Type: application/json' \
--data "$(jq -n --arg id "$SKU_ID-legacy" --arg site "$SITE_ID" '{
id: $id,
siteId: $site,
schemaVersion: 5,
storage: [{model: "TEST-NVME", count: 1, capacityMb: 3600000}]
}')"
curl -i -X DELETE \
"http://localhost:8388/v2/org/test-org/nico/sku/$SKU_ID?siteId=$SITE_ID" \
-H "Authorization: Bearer $TOKEN"
curl -i -X PATCH \
"http://localhost:8388/v2/org/test-org/nico/sku/$SKU_ID?siteId=$SITE_ID" \
-H "Authorization: Bearer $TOKEN" \
-H 'Content-Type: application/json' \
--data '{"description":"must not exist"}'Expected results: legacy POST 400, DELETE 204, post-delete PATCH 404. |
(IMO the "What this PR does" should probably stay, but yeah the rest should move) |
c0f658d to
1335142
Compare
48fcac3 to
507093a
Compare
507093a to
859868e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
What this PR doesThe current revision writes the Core-authoritative SKU projection into the REST database immediately after successful create and update operations, and removes it immediately after delete. REST reads therefore reflect the mutation without waiting for the periodic inventory sync. How we verified itRevision: Environment: macOS on Apple Silicon, Docker Desktop, kind cluster At The Site Agent recorded successful real-Core RPCs ( Supporting handler verification also passed: cd rest-api
go test ./api/pkg/api/handler -run '^(TestCreateSkuHandler|TestUpdateSkuHandler|TestDeleteSkuHandler)$' -count=1 -vHow to reproduce the verificationPrerequisites: Docker Desktop, kind, kubectl, jq, Go, Rust, a local REST stack, and the local Core prerequisites documented by
|
a90633b to
615c781
Compare
|
@coderabbitai review |
5039819 to
060bb6a
Compare
thossain-nv
left a comment
There was a problem hiding this comment.
Thanks for the implementation @nvlitagaki, added a few notes.
Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
55b5bdf to
6d4f2a7
Compare
eae95a1 to
01a2286
Compare
Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
thossain-nv
left a comment
There was a problem hiding this comment.
Thanks for the updates @nvlitagaki, added some notes regarding the models.
Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
thossain-nv
left a comment
There was a problem hiding this comment.
@nvlitagaki sorry if I didn't write it correctly in my previous comment - APIActionResourceRequest is only needed if it differs from the original object. Usually request objects are different from response object, but in case SKU, they seem to be usable both as request and response.
| // Vendor is retained for response compatibility. | ||
| // | ||
| // Deprecated: Core returns an empty string and does not use this field for matching. | ||
| Vendor string `json:"vendor"` |
There was a problem hiding this comment.
We should turn the deprecated attributes into *string Also it might be a good idea to issue a deprecation notice for them? 2.1 will be released September 30, we can set that as deprecation date.
There was a problem hiding this comment.
I've realized deprecation is the wrong call here. Existing sites that have SKUs with schema version 4 will continue to have and rely on these fields until anyone sees fit to update them. We can't know when that will happen (might be never), as there is nothing forcing a migration.
Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
|
Thanks for the changes @nvlitagaki |
Provider Admins can create, partially update, and delete a SKU for a selected Site. The REST API sends the mutation through Temporal and the Site Agent to that Site's real NICo Core service. POST and PATCH return the Core-backed SKU shape; PATCH preserves fields omitted from the request. This adds REST API support for managing SKUs. Resolves NVIDIA#2810 ### What this PR does Adds Site-scoped REST endpoints to create, partially update, and delete Core-backed SKUs. Mutations run through Temporal and the Site Agent to the selected Site's NICo Core. The writable schema includes SKU v5 storage size bounds and PCI patterns; ignored legacy write fields such as `capacityMb` are rejected. --------- Signed-off-by: Leah Itagaki <litagaki@nvidia.com> Signed-off-by: Alex Ball <aball@nvidia.com>
Provider Admins can create, partially update, and delete a SKU for a selected Site. The REST API sends the mutation through Temporal and the Site Agent to that Site's real NICo Core service. POST and PATCH return the Core-backed SKU shape; PATCH preserves fields omitted from the request.
This adds REST API support for managing SKUs.
Resolves #2810
What this PR does
Adds Site-scoped REST endpoints to create, partially update, and delete Core-backed SKUs. Mutations run through Temporal and the Site Agent to the selected Site's NICo Core. The writable schema includes SKU v5 storage size bounds and PCI patterns; ignored legacy write fields such as
capacityMbare rejected.