feat(rest-api): iPXE template + templated-OS REST endpoints#3569
Open
pbreton wants to merge 16 commits into
Open
feat(rest-api): iPXE template + templated-OS REST endpoints#3569pbreton wants to merge 16 commits into
pbreton wants to merge 16 commits into
Conversation
pbreton
marked this pull request as draft
July 15, 2026 18:44
|
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. |
Contributor
|
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:
WalkthroughTemplated iPXE support adds API contracts and validation, template retrieval endpoints, Core-proxy synchronization for Operating System lifecycle operations, instance provisioning through synchronized Operating System references, nullable tenant ownership, and revised inventory reconciliation. ChangesTemplated iPXE API contracts
iPXE template retrieval
Operating System site lifecycle
Instance provisioning
Nullable ownership and reconciliation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OperatingSystemAPI
participant Database
participant CoreProxy
participant Site
Client->>OperatingSystemAPI: create templated iPXE Operating System
OperatingSystemAPI->>Database: store OS and site associations
OperatingSystemAPI->>CoreProxy: synchronize OS definition
CoreProxy->>Site: create OS definition
Site-->>CoreProxy: return synchronization status
CoreProxy-->>OperatingSystemAPI: return per-site result
OperatingSystemAPI-->>Client: return OS and aggregate status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3569.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-15 18:46:46 UTC | Commit: f0205bf |
![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: 11
🧹 Nitpick comments (5)
rest-api/api/pkg/api/handler/operatingsystem_templated_proxy_test.go (1)
81-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the proxied method and payload for each lifecycle operation.
The shared
mock.Anythingexpectation accepts create, update, and delete interchangeably, so incorrect method constants or request builders still pass. Capture the workflow input and assert its full method and relevant payload in each subtest.Also applies to: 111-171
🤖 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/operatingsystem_templated_proxy_test.go` around lines 81 - 85, Replace the broad ExecuteWorkflow mock.Anything expectation in each lifecycle subtest with captured workflow input assertions. Verify the proxied method constant and complete relevant request payload for create, update, and delete operations, ensuring incorrect builders or methods fail the test.rest-api/api/pkg/api/routes_test.go (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert both new routes explicitly.
The aggregate count does not verify the method or path. Add
assertRouteExistschecks for/ipxe-templateand/ipxe-template/:id.Proposed test coverage
+ipxeTemplatePath := "/org/:orgName/" + cfg.GetAPIName() + "/ipxe-template" +assertRouteExists(t, got, http.MethodGet, ipxeTemplatePath) +assertRouteExists(t, got, http.MethodGet, ipxeTemplatePath+"/: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 `@rest-api/api/pkg/api/routes_test.go` at line 65, Add explicit assertRouteExists checks in the route test for both /ipxe-template and /ipxe-template/:id, alongside the existing aggregate route count assertion. Keep the count assertion, and verify each route’s method and path through the established helper.rest-api/openapi/spec.yaml (3)
16912-16916: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
scopeandvisibilityfields lackenumconstraints despite documenting a fixed value set.
OperatingSystem.scope(16912-16916) is documented as "Local, Global, or Limited" in prose only, whileOperatingSystemCreateRequest.scope(declared later in the same file) already enforces this viaenum.IpxeTemplate.visibility(17068-17070) is similarly documented as "Internal or Public" without an enum. Both are inconsistent with the file-wide convention of declaringenumfor any fixed-value string field, weakening generated-SDK type safety and request/response validation.🔧 Proposed enum additions
scope: type: - string - 'null' + enum: + - Local + - Global + - Limited + - null description: 'Synchronization scope for iPXE-based Operating Systems (Local, Global, or Limited)'visibility: type: string + enum: + - Internal + - Public description: 'Template visibility: Internal or Public'Also applies to: 17068-17070
🤖 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 16912 - 16916, Update the OperatingSystem.scope and IpxeTemplate.visibility schema definitions to include enum constraints matching their documented values: Local, Global, and Limited for scope, and Internal and Public for visibility. Keep their existing nullable string types and descriptions unchanged, and align them with the corresponding OperatingSystemCreateRequest.scope constraint.Source: Path instructions
17385-17404: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUpdate-request
ipxeTemplateParameters/ipxeTemplateArtifactsdon't document replace-vs-merge semantics.Other array-typed update fields in this file (
labels,sshKeyGroupIds) explicitly warn that the array is a full replacement and instruct callers to fetch-then-include to retain existing entries.OperatingSystemUpdateRequest.ipxeTemplateParameters/ipxeTemplateArtifactscarry no such note, leaving callers to guess whether a partial update merges or silently drops unlisted parameters/artifacts — a real risk of unintended data loss for Templated iPXE OS definitions.📝 Proposed clarification
ipxeTemplateParameters: type: array items: $ref: '`#/components/schemas/OperatingSystemIpxeParameter`' - description: 'Parameters passed to the iPXE template (Templated iPXE only).' + description: 'Parameters passed to the iPXE template (Templated iPXE only). Replaces the existing parameter set; to retain existing parameters, fetch them first and include them in this request.' ipxeTemplateArtifacts: type: array items: $ref: '`#/components/schemas/OperatingSystemIpxeArtifact`' - description: 'Artifacts (kernel, initrd, ISO, ...) for the iPXE OS definition (Templated iPXE only).' + description: 'Artifacts (kernel, initrd, ISO, ...) for the iPXE OS definition (Templated iPXE only). Replaces the existing artifact set; to retain existing artifacts, fetch them first and include them in this request.'🤖 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 17385 - 17404, Add descriptions to OperatingSystemUpdateRequest.ipxeTemplateParameters and ipxeTemplateArtifacts documenting that each array fully replaces the existing values, not merges; instruct callers to fetch the current OS definition and include retained entries when updating either field.Source: Path instructions
11719-11792: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew iPXE Template endpoints omit required-role documentation, and the
iPXE Templatetag has no top-level description.Both
get-all-ipxe-templateandget-ipxe-templatedescribe what the caller must be authorized for ("at least one Site") but, unlike every single other operation in this file, never state which role suffix (TENANT_ADMIN/PROVIDER_ADMIN) is required. Separately,tags: [iPXE Template]is used here but "iPXE Template" is never declared with adescriptionin the top-leveltags:array (unlike "SKU", "Host Firmware Config", etc.), so the generated docs section for this feature will render without a description.📝 Proposed doc fixes
get: summary: Get all iPXE templates - description: 'Get all iPXE templates propagated from nico-core. Optionally restrict to one or more sites with the siteId query parameter.' + description: |- + Get all iPXE templates propagated from nico-core, restricted to Sites the caller can access. Optionally restrict to one or more sites with the siteId query parameter. + + User must have authorization role with `TENANT_ADMIN` or `PROVIDER_ADMIN` suffix. operationId: get-all-ipxe-templateget: summary: Retrieve an iPXE template - description: 'Retrieve an iPXE template by its stable core ID. The caller must be authorized for at least one Site at which the template is available.' + description: |- + Retrieve an iPXE template by its stable core ID. The caller must be authorized for at least one Site at which the template is available. + + User must have authorization role with `TENANT_ADMIN` or `PROVIDER_ADMIN` suffix. operationId: get-ipxe-templateAlso add a tag description alongside the other entries (outside the selected range, near the
SKUtag):- name: iPXE Template description: |- iPXE Template exposes read-only, core-propagated iPXE script templates used by Templated iPXE Operating Systems.🤖 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 11719 - 11792, Add the required role suffix documentation to both get-all-ipxe-template and get-ipxe-template, explicitly stating whether TENANT_ADMIN or PROVIDER_ADMIN is required, consistent with other operations. Also declare the iPXE Template entry in the top-level tags array with the provided read-only core-propagated template description.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/instance.go`:
- Around line 216-226: Require a shared validator for the templated OS selection
paths that verifies authorization, tenant/resource ownership, and a Synced
OS/site association before sending the templated Operating System ID. Apply it
to create in rest-api/api/pkg/api/handler/instance.go lines 216-226, update in
rest-api/api/pkg/api/handler/instance.go lines 2112-2122, and batch creation in
rest-api/api/pkg/api/handler/instancebatch.go lines 189-199; reject Local
templated OSs and Global OSs not synchronized to the site before proceeding.
In `@rest-api/api/pkg/api/handler/ipxetemplate.go`:
- Around line 317-360: The callerHasAccessToAnyAssociatedSite authorization
helper must return (bool, error) instead of converting provider or tenant DAO
failures into false. Propagate errors from the provider and tenant GetAll
lookups, update its caller around the authorization check to map lookup errors
to HTTP 500, and retain HTTP 403 only when lookups succeed but no associated
site is accessible.
In `@rest-api/api/pkg/api/handler/operatingsystem_templated_proxy_test.go`:
- Around line 173-177: Update the assertion in the osDAO.GetByID soft-deletion
test to require that gerr equals cdb.ErrDoesNotExist, rather than merely
asserting any error. Preserve the existing lookup and test message so the test
specifically verifies the operating system was soft-deleted after site cleanup.
In `@rest-api/api/pkg/api/handler/operatingsystem.go`:
- Around line 590-622: Update reloadOperatingSystemForResponse to accept the
existing status details and site associations as inputs, then initialize ssds
and ossas from those prior values before attempting reloads. Keep those values
unchanged when sdDAO.GetRecentByEntityIDs or ossaDAO.GetAll fails, while
replacing them only on successful reads; preserve the existing reloadedOS
fallback behavior.
- Around line 1493-1506: The proxy synchronization flow around
syncOperatingSystemToSitesViaProxy and updateOperatingSystemAggregateStatus must
preserve a deactivated operating system’s Deactivated status. Skip aggregate
readiness updates for systems already marked Deactivated, or restore that status
after synchronization, while retaining Ready/Error aggregation for active
systems.
- Around line 130-155: Handle the empty result from getRegisteredTenantSites in
the Global-scope templated iPXE OS creation/update flow: when no registered
tenant sites are available, reject the request or explicitly transition the OS
to a terminal state instead of proceeding without associations. Ensure the
post-commit synchronization guard cannot leave the OS in Syncing when the target
set is empty.
- Around line 1794-1812: Update the templated iPXE deletion flow around
osDAO.Update, sdDAO.Create, and the post-commit site-processing path so every
affected site association is persisted as Deleting and then Error on failure.
Replace the current remaining-only accounting with durable retry scheduling for
failed site deletions, or propagate an actionable failure instead of returning
202 while work is stranded. Ensure the OS cannot remain permanently in Deleting
when site cleanup fails.
- Around line 53-128: Update updateOSSAStatusViaProxy to persist the association
status and status detail atomically using cdb.WithTx or cdb.WithTxResult, and
return any persistence error instead of only logging it. In
syncOperatingSystemToSitesViaProxy, treat returned bookkeeping errors as
siteErrors alongside proxy failures. Ensure the caller passes the combined error
count to updateOperatingSystemAggregateStatus so Ready is reported only when all
association persistence and proxy operations succeed.
- Around line 76-79: Update the operating-system proxy flow around
ExecuteCoreGRPC to redact or encrypt each nested
ipxeTemplateArtifacts[].authToken before the request enters the workflow
payload, while preserving the existing site-ID encryption context. Add coverage
that verifies the serialized proxy input does not contain plaintext artifact
tokens.
In `@rest-api/api/pkg/api/model/operatingsystem.go`:
- Around line 933-957: Update validateIpxeTemplateArtifacts to validate each
non-blank a.URL with the existing validation.Field(..., is.URL) pattern, rather
than only checking trimmed content. Preserve the current required-field error
for blank URLs and return the established validation error when a non-blank URL
is malformed.
In `@rest-api/openapi/spec.yaml`:
- Around line 11719-11760: Update the get-all-ipxe-template operation’s
pageNumber, pageSize, and orderBy parameter definitions to match the
descriptions, constraints, and bounds used by other paginated list routes.
Document the X-Pagination response header in the 200 response while preserving
the existing array response schema and siteId filtering behavior.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/operatingsystem_templated_proxy_test.go`:
- Around line 81-85: Replace the broad ExecuteWorkflow mock.Anything expectation
in each lifecycle subtest with captured workflow input assertions. Verify the
proxied method constant and complete relevant request payload for create,
update, and delete operations, ensuring incorrect builders or methods fail the
test.
In `@rest-api/api/pkg/api/routes_test.go`:
- Line 65: Add explicit assertRouteExists checks in the route test for both
/ipxe-template and /ipxe-template/:id, alongside the existing aggregate route
count assertion. Keep the count assertion, and verify each route’s method and
path through the established helper.
In `@rest-api/openapi/spec.yaml`:
- Around line 16912-16916: Update the OperatingSystem.scope and
IpxeTemplate.visibility schema definitions to include enum constraints matching
their documented values: Local, Global, and Limited for scope, and Internal and
Public for visibility. Keep their existing nullable string types and
descriptions unchanged, and align them with the corresponding
OperatingSystemCreateRequest.scope constraint.
- Around line 17385-17404: Add descriptions to
OperatingSystemUpdateRequest.ipxeTemplateParameters and ipxeTemplateArtifacts
documenting that each array fully replaces the existing values, not merges;
instruct callers to fetch the current OS definition and include retained entries
when updating either field.
- Around line 11719-11792: Add the required role suffix documentation to both
get-all-ipxe-template and get-ipxe-template, explicitly stating whether
TENANT_ADMIN or PROVIDER_ADMIN is required, consistent with other operations.
Also declare the iPXE Template entry in the top-level tags array with the
provided read-only core-propagated template description.
🪄 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: 5a293074-d8da-4a1a-b13d-d72fdd8b623f
⛔ Files ignored due to path filters (9)
rest-api/sdk/standard/api_ipxe_template.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_operating_system.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_ipxe_template.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_artifact.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_parameter.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (14)
rest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instancebatch.gorest-api/api/pkg/api/handler/ipxetemplate.gorest-api/api/pkg/api/handler/ipxetemplate_test.gorest-api/api/pkg/api/handler/operatingsystem.gorest-api/api/pkg/api/handler/operatingsystem_templated_proxy_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/api/pkg/api/model/ipxetemplate.gorest-api/api/pkg/api/model/operatingsystem.gorest-api/api/pkg/api/model/operatingsystem_templated_test.gorest-api/api/pkg/api/routes.gorest-api/api/pkg/api/routes_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yaml
pbreton
force-pushed
the
feat/ipxe-templated-os-3
branch
from
July 15, 2026 22:14
1b39a0c to
9cf43f9
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
rest-api/openapi/spec.yaml (3)
16993-17037: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
OperatingSystemIpxeParameterandOperatingSystemIpxeArtifacthave norequiredfields.Both schemas are used directly inside
OperatingSystemCreateRequest/UpdateRequest(client-supplied input), yet neither declares arequiredlist.name/valueon the parameter, andname/urlon the artifact, appear essential for the objects to be meaningful — withoutrequired, clients can submit empty parameter/artifact entries that pass schema validation but are meaningless to nico-core.🩹 Proposed fix
OperatingSystemIpxeParameter: title: OperatingSystemIpxeParameter type: object description: A name/value parameter passed to an iPXE template + required: + - name + - value properties: ... OperatingSystemIpxeArtifact: title: OperatingSystemIpxeArtifact type: object description: An artifact (kernel, initrd, ISO, ...) referenced by an iPXE OS definition + required: + - name + - url properties:🤖 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 16993 - 17037, Update the OperatingSystemIpxeParameter and OperatingSystemIpxeArtifact schemas to declare required fields: name and value for parameters, and name and url for artifacts. Keep optional fields such as sha, authType, authToken, and cacheStrategy unchanged.
16916-16920: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ipxeTemplateIdshould declareformat: uuidfor consistency.
IpxeTemplate.id(line 17064) is documented asformat: uuid, butipxeTemplateIdonOperatingSystem,OperatingSystemCreateRequest, andOperatingSystemUpdateRequestis a barestring. Since this PR also ships a regenerated Go SDK, addingformat: uuidhere keeps generated client types consistent and self-documenting.🩹 Proposed fix (repeat for all three locations)
ipxeTemplateId: type: - string - 'null' + format: uuid description: 'ID of the iPXE template used, only present for Templated iPXE Operating System'Also applies to: 17273-17277, 17404-17408
🤖 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 16916 - 16920, Update the ipxeTemplateId schema property in OperatingSystem, OperatingSystemCreateRequest, and OperatingSystemUpdateRequest to declare format: uuid alongside its existing string/null type definition, matching IpxeTemplate.id.
16688-16694: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a Templated iPXE example.
OperatingSystem,OperatingSystemCreateRequest, andOperatingSystemUpdateRequestall carry examples foriPXEandImagetypes, but none for the newTemplated iPXEtype. Given this is a net-new, non-trivial variant (template ID, parameters, artifacts, scope), an example would materially help API consumers adopting it.Also applies to: 17146-17148
🤖 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 16688 - 16694, Extend the examples for the OperatingSystem, OperatingSystemCreateRequest, and OperatingSystemUpdateRequest schemas to include a representative Templated iPXE variant. Show the required template ID, parameters, artifacts, and scope fields using valid values consistent with the schema and existing iPXE/Image examples.
🤖 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/ipxetemplate_test.go`:
- Around line 198-211: Extend the successful-response assertions in the ipxe
template list test around the response body decoding to read and validate the
X-Pagination header. Decode the header using the existing pagination
representation, then assert the expected page number, page size, and total for
each test case, while retaining the existing template-name and orphan-template
checks.
In `@rest-api/api/pkg/api/handler/ipxetemplate.go`:
- Around line 81-94: Update the handler containing the requestedSiteIDStrs
parsing to call common.ValidateKnownQueryParams for the supported query
parameters and return its 400 response for unsupported parameters. Change the
siteId loop to reject empty values with a 400 error instead of continuing, while
preserving UUID parsing validation and collection of repeated non-empty siteId
values.
In `@rest-api/api/pkg/api/handler/operatingsystem_templated_proxy_test.go`:
- Around line 82-86: Strengthen the tests around the workflow mock setup in
operatingsystem templated proxy tests: match the expected create/update/delete
method path, site ID, and protobuf payload instead of using broad mock.Anything
values, then assert all mock expectations. Add isolated cases for proxy errors
and timeouts, verifying the resulting association and aggregate statuses while
covering the ExecuteWorkflow arguments and timeout behavior.
In `@rest-api/api/pkg/api/handler/operatingsystem.go`:
- Around line 123-143: The updateOperatingSystemAggregateStatus function should
return an error, accept the operation-specific status-detail message from
callers, and execute both the OperatingSystemDAO.Update and
StatusDetailDAO.Create writes inside a cdb.WithTx closure using the transaction
session. Propagate transaction or database failures to callers instead of only
logging them, while preserving the Ready/Error status selection and ensuring
deletion operations provide a deletion-appropriate message rather than the sync
message.
In `@rest-api/api/pkg/api/model/operatingsystem.go`:
- Around line 135-138: Update all three validation error branches in the
operating-system model to use the client-facing JSON field name “imageUrl”
instead of “imageURL”. Apply the change consistently to the validation keys in
the branches involving iPXE fields and ImageURL, without altering their
conditions or messages.
- Around line 897-902: Update validateTemplatedIpxeOS in
rest-api/api/pkg/api/model/operatingsystem.go to validate every SiteIDs element
with validation.Each(validationis.UUID...) when the scope is Limited, while
preserving the existing presence and scope checks. Add a non-UUID SiteIDs case
to rest-api/api/pkg/api/model/operatingsystem_templated_test.go covering the
resulting validation error.
In `@rest-api/api/pkg/api/routes_test.go`:
- Line 65: Update the route tests near the "ipxe-template" expected count to
explicitly call assertRouteExists for both the collection GET route and the
detail GET route, verifying their HTTP methods and paths while retaining the
aggregate route count assertion.
In `@rest-api/api/pkg/api/routes.go`:
- Around line 732-735: Rename the detail route parameter from id to
ipxeTemplateId and update the corresponding handler lookup and
godoc/OpenAPI/SDK-facing parameter names together. Keep the route behavior
unchanged while ensuring all published representations use ipxeTemplateId
consistently.
In `@rest-api/openapi/spec.yaml`:
- Around line 11711-11724: Add a top-level tags entry for “iPXE Template” with a
descriptive summary, matching the existing tag definitions used by the OpenAPI
spec. Ensure the entry covers both operations under the iPXE template endpoints
so generated documentation displays the tag description consistently.
- Around line 11719-11722: Update the descriptions for the get-all-ipxe-template
and get-ipxe-template operations to document their required authorization roles,
matching the established wording and format used by other operations in the
spec. Include the enforced Provider Admin/Viewer and Tenant Admin role
requirements without changing the endpoint behavior or other metadata.
- Around line 17288-17297: Remove Local from the create scope enum in the scope
schema, leaving Global, Limited, and null as the client-accepted values.
Preserve Local only in any separate read-only response schema or documentation
if internal responses must expose it.
---
Nitpick comments:
In `@rest-api/openapi/spec.yaml`:
- Around line 16993-17037: Update the OperatingSystemIpxeParameter and
OperatingSystemIpxeArtifact schemas to declare required fields: name and value
for parameters, and name and url for artifacts. Keep optional fields such as
sha, authType, authToken, and cacheStrategy unchanged.
- Around line 16916-16920: Update the ipxeTemplateId schema property in
OperatingSystem, OperatingSystemCreateRequest, and OperatingSystemUpdateRequest
to declare format: uuid alongside its existing string/null type definition,
matching IpxeTemplate.id.
- Around line 16688-16694: Extend the examples for the OperatingSystem,
OperatingSystemCreateRequest, and OperatingSystemUpdateRequest schemas to
include a representative Templated iPXE variant. Show the required template ID,
parameters, artifacts, and scope fields using valid values consistent with the
schema and existing iPXE/Image examples.
🪄 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: 2415b85d-3a40-4ee2-b686-8e6d02580327
⛔ Files ignored due to path filters (9)
rest-api/sdk/standard/api_ipxe_template.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_operating_system.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_ipxe_template.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_artifact.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_parameter.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (15)
rest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_templated_os_test.gorest-api/api/pkg/api/handler/instancebatch.gorest-api/api/pkg/api/handler/ipxetemplate.gorest-api/api/pkg/api/handler/ipxetemplate_test.gorest-api/api/pkg/api/handler/operatingsystem.gorest-api/api/pkg/api/handler/operatingsystem_templated_proxy_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/api/pkg/api/model/ipxetemplate.gorest-api/api/pkg/api/model/operatingsystem.gorest-api/api/pkg/api/model/operatingsystem_templated_test.gorest-api/api/pkg/api/routes.gorest-api/api/pkg/api/routes_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yaml
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
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/api/pkg/api/handler/operatingsystem.go (1)
291-296: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd an iPXe template lookup before persisting OS records
Validate()/validateTemplatedIpxeOS()only check request shape and scope. They do not confirm thatipxeTemplateIdexists or is available for the chosen sites, so both create and update can persist a bad template id and only fail later during Core sync.🤖 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/operatingsystem.go` around lines 291 - 296, Before persisting the operating system record, extend the validation flow around apiRequest.Validate() to look up and validate the requested ipxeTemplateId, confirming it exists and is available for the selected sites. Apply this check to both OS creation and update handlers, return the same bad-request error style on failure, and prevent persistence when the template is invalid.
🧹 Nitpick comments (2)
rest-api/api/pkg/api/handler/ipxetemplate_test.go (2)
231-252: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete client-facing response shape.
The success cases only verify names or IDs. Assert
template,visibility, timestamps, and that required/reserved/artifact slices serialize as empty arrays rather thannull, so constructor mapping regressions cannot pass unnoticed.As per coding guidelines, endpoint tests must cover response shape; path instructions require client-facing model compatibility.
Also applies to: 355-359
🤖 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/ipxetemplate_test.go` around lines 231 - 252, Expand the successful response assertions in the ipxe template endpoint tests around the response iteration to validate each client-facing template field, including template, visibility, and timestamps, not just names or IDs. Also assert required, reserved, and artifact slices are empty arrays rather than null, and apply the same complete-shape checks to the additional success-case assertions near the other referenced block.Sources: Coding guidelines, Path instructions
170-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the malformed UUID validation branch.
Add a
siteId=not-a-uuidcase. The empty-value test exits beforeuuid.Parse, so the non-empty parsing failure remains untested.As per coding guidelines, endpoint tests must cover validation. Based on learnings, each failure-path test should isolate the exact validation branch it targets.
🤖 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/ipxetemplate_test.go` around lines 170 - 192, Extend the endpoint test cases around the existing empty siteId validation in the relevant test table to add a non-empty malformed value such as “not-a-uuid”. Expect the same bad-request error and status, while ensuring this case reaches and covers the UUID parsing failure branch separately from the empty-value validation.Sources: Coding guidelines, Learnings
🤖 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/operatingsystem.go`:
- Around line 410-419: The global-scope Templated iPXE creation flow leaves the
operating system in Syncing when getRegisteredTenantSites returns no sites.
Update the handling around getRegisteredTenantSites and the post-commit sync
block gated by len(dbossa) > 0 to either reject creation when no Registered
tenant sites exist or explicitly transition the persisted OS to a terminal
status such as Ready or Error, ensuring it cannot remain in Syncing.
- Around line 143-175: Move the Operating System status read and deactivation
guard into the cdb.WithTx closure, acquire the applicable advisory lock at the
start of that transaction, then re-read via GetByID using the transaction before
updating. Preserve the existing behavior for deactivated systems and ensure the
aggregate status update and StatusDetail creation use the same locked
transaction state.
---
Outside diff comments:
In `@rest-api/api/pkg/api/handler/operatingsystem.go`:
- Around line 291-296: Before persisting the operating system record, extend the
validation flow around apiRequest.Validate() to look up and validate the
requested ipxeTemplateId, confirming it exists and is available for the selected
sites. Apply this check to both OS creation and update handlers, return the same
bad-request error style on failure, and prevent persistence when the template is
invalid.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/ipxetemplate_test.go`:
- Around line 231-252: Expand the successful response assertions in the ipxe
template endpoint tests around the response iteration to validate each
client-facing template field, including template, visibility, and timestamps,
not just names or IDs. Also assert required, reserved, and artifact slices are
empty arrays rather than null, and apply the same complete-shape checks to the
additional success-case assertions near the other referenced block.
- Around line 170-192: Extend the endpoint test cases around the existing empty
siteId validation in the relevant test table to add a non-empty malformed value
such as “not-a-uuid”. Expect the same bad-request error and status, while
ensuring this case reaches and covers the UUID parsing failure branch separately
from the empty-value validation.
🪄 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: 059840d0-71ee-4787-9eed-00aee1b7e625
⛔ Files ignored due to path filters (9)
rest-api/sdk/standard/api_ipxe_template.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_operating_system.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_ipxe_template.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_artifact.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_parameter.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (15)
rest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_templated_os_test.gorest-api/api/pkg/api/handler/instancebatch.gorest-api/api/pkg/api/handler/ipxetemplate.gorest-api/api/pkg/api/handler/ipxetemplate_test.gorest-api/api/pkg/api/handler/operatingsystem.gorest-api/api/pkg/api/handler/operatingsystem_templated_proxy_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/api/pkg/api/model/ipxetemplate.gorest-api/api/pkg/api/model/operatingsystem.gorest-api/api/pkg/api/model/operatingsystem_templated_test.gorest-api/api/pkg/api/routes.gorest-api/api/pkg/api/routes_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yaml
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
rest-api/api/pkg/api/model/operatingsystem_templated_test.go (1)
144-167: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert artifact credential mappings in both Core requests.
The fixture sets
AuthTypeandAuthToken, but neither mapping is asserted. Verify both fields for create and update requests while retaining the separate REST-response redaction test.As per coding guidelines, model tests must cover protobuf mappings.
Also applies to: 170-182
🤖 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/operatingsystem_templated_test.go` around lines 144 - 167, Extend the “create request maps all fields” assertions in the operating-system model tests to verify the artifact’s AuthType and AuthToken protobuf mappings, and add the same credential assertions to the corresponding update-request test. Reuse the existing fixture values and preserve the separate REST-response redaction test.Source: Coding guidelines
🤖 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/instance_templated_os_test.go`:
- Around line 162-178: Extend the “rejects templated OS not synchronized to
Site” test around buildOS and buildInstanceCreateRequestOsConfig with separate
existing Site associations in Syncing and Error states. Keep all other
prerequisites valid, then assert each case returns BadRequest with nil osConfig
and osID, ensuring only the Synced association is accepted.
In `@rest-api/api/pkg/api/handler/ipxetemplate.go`:
- Line 52: Restrict both iPXE template handlers to organizations with a Tenant
Account and a role ending in TENANT_ADMIN, removing Provider Admin/Viewer and
provider-site authorization paths. Update the endpoint godoc to describe
tenant-only access, and revise authorization, unauthorized-site, and
orphan-template tests so their fixtures are tenant-based and satisfy all other
prerequisites before exercising the intended failure branch.
In `@rest-api/api/pkg/api/handler/operatingsystem.go`:
- Around line 1367-1369: Within the OS update transaction around the status
handling for cdbm.OperatingSystemTypeTemplatedIPXE in the relevant handler,
transition all affected templated site associations and their status details to
Syncing before any proxy updates occur. Preserve the aggregate osStatus update,
and ensure validateTemplatedIpxeOsForSite observes the association-level Syncing
state during the transaction.
- Around line 643-652: Serialize lifecycle proxy operations per Operating System
using a durable sequence or version: in
rest-api/api/pkg/api/handler/operatingsystem.go:643-652, assign the initial
create operation its sequence/version; in
rest-api/api/pkg/api/handler/operatingsystem.go:1563-1581, serialize updates or
ensure Core rejects stale versions; and in
rest-api/api/pkg/api/handler/operatingsystem.go:1939-1987, ensure deletion
supersedes all earlier create/update operations before removing database state,
with TOCTOU-prone mutations performed under a lock against re-read state.
In `@rest-api/api/pkg/api/model/operatingsystem.go`:
- Around line 75-80: Replace the public cdbm.OperatingSystemIpxeParameter and
cdbm.OperatingSystemIpxeArtifact fields and related REST-model usages with
API-owned parameter and artifact DTOs. Add receiver-based typed-list validation
and protobuf conversion methods on those DTOs, then update the affected
request/response models and conversion paths to use them. Ensure response DTOs
structurally omit artifact tokens rather than removing them manually.
- Around line 116-120: Update operating system create validation in
rest-api/api/pkg/api/model/operatingsystem.go:116-120 and update validation in
rest-api/api/pkg/api/model/operatingsystem.go:416-420 to require non-empty
ipxeTemplateId values to be valid UUIDs while preserving optional-field
handling. In rest-api/api/pkg/api/model/operatingsystem_templated_test.go:25,
use a UUID for valid fixtures and add coverage for an invalid identifier.
---
Nitpick comments:
In `@rest-api/api/pkg/api/model/operatingsystem_templated_test.go`:
- Around line 144-167: Extend the “create request maps all fields” assertions in
the operating-system model tests to verify the artifact’s AuthType and AuthToken
protobuf mappings, and add the same credential assertions to the corresponding
update-request test. Reuse the existing fixture values and preserve the separate
REST-response redaction test.
🪄 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: fb926162-effb-4f89-95ef-7ce6881994bc
⛔ Files ignored due to path filters (9)
rest-api/sdk/standard/api_ipxe_template.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_operating_system.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_ipxe_template.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_artifact.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_parameter.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (15)
rest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_templated_os_test.gorest-api/api/pkg/api/handler/instancebatch.gorest-api/api/pkg/api/handler/ipxetemplate.gorest-api/api/pkg/api/handler/ipxetemplate_test.gorest-api/api/pkg/api/handler/operatingsystem.gorest-api/api/pkg/api/handler/operatingsystem_templated_proxy_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/api/pkg/api/model/ipxetemplate.gorest-api/api/pkg/api/model/operatingsystem.gorest-api/api/pkg/api/model/operatingsystem_templated_test.gorest-api/api/pkg/api/routes.gorest-api/api/pkg/api/routes_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yaml
pbreton
force-pushed
the
feat/ipxe-templated-os-3
branch
2 times, most recently
from
July 21, 2026 01:16
b7189c3 to
0edb13a
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/admin-cli/src/operating_system/common.rs (1)
123-156: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve absent ownership in JSON output.
SerializableOsis used for--format json, but this maps a provider-owned OS fromNoneto"". That loses the nullable wire contract and makes the output misleading. MakeSerializableOs.orgoptional and pass throughtenant_organization_id; retain blank formatting only for human-readable tables.Proposed fix
pub struct SerializableOs { - pub org: String, + pub org: Option<String>, } - org: os.tenant_organization_id.unwrap_or_default(), + org: os.tenant_organization_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 `@crates/admin-cli/src/operating_system/common.rs` around lines 123 - 156, Update SerializableOs and its From<OperatingSystem> conversion so org remains optional, passing through tenant_organization_id instead of replacing None with an empty string. Preserve blank formatting only in the human-readable table rendering path.
🧹 Nitpick comments (3)
rest-api/openapi/spec.yaml (3)
17093-17095: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
enumforvisibilityinstead of prose-only values.
visibilityis a brand-new field with exactly two allowed values ("Internal or Public"), but is defined as a plainstringand only documents the values in free text. Every other fixed-value string field introduced elsewhere in this spec (HostFirmwareComponentType,cacheStrategy,authTypeexcepted per legacy pattern) usesenum:so clients and generated SDKs get compile-time validation instead of relying on a human reading the description.As per coding guidelines, `rest-api/openapi/**/*.{json,yaml,yml}`: "Update the OpenAPI schema whenever API endpoints are added or changed" — since this is a brand-new schema with no legacy baggage, it's a good candidate to apply the enum convention up front rather than propagate a new loose-string pattern.🔧 Proposed fix
visibility: type: string - description: 'Template visibility: Internal or Public' + enum: + - Internal + - Public + description: Template visibility🤖 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 17093 - 17095, Update the OpenAPI property visibility to constrain its string values with an enum containing exactly Internal and Public, while retaining its existing description and schema location.Source: Coding guidelines
192-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPublic-facing tag description leaks internal implementation detail.
The description names
IpxeTemplateSiteAssociation, which reads like an internal DB/model type rather than customer-facing API vocabulary. If that internal name is renamed during a future refactor, this doc silently goes stale, and external consumers gain no benefit from knowing the internal record name — the same information ("per-site availability isn't a template attribute, filter withsiteId") can be conveyed without it.✏️ Proposed rewording
- iPXE Templates are read-only definitions propagated from nico-core into the REST API. Each template has a stable core UUID and a globally unique name; per-site availability is tracked in IpxeTemplateSiteAssociation records rather than on the template row itself. List and retrieve operations return templates reported at sites the caller is authorized for (optionally filtered by `siteId`). Templated iPXE Operating Systems reference a template by `ipxeTemplateId`. + iPXE Templates are read-only definitions propagated from nico-core into the REST API. Each template has a stable core UUID and a globally unique name; per-site availability is tracked separately from the template itself. List and retrieve operations return templates reported at sites the caller is authorized for (optionally filtered by `siteId`). Templated iPXE Operating Systems reference a template by `ipxeTemplateId`.🤖 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 192 - 194, Update the iPXE Template tag description in the OpenAPI specification to remove the internal `IpxeTemplateSiteAssociation` model name. Preserve the customer-facing behavior by describing per-site availability generically and retaining the guidance that callers can filter with `siteId`.
7904-8013: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo example payload anywhere for the new Templated iPXE Operating System.
The
ImageandiPXEOS types each get worked examples across the list, retrieve, create, and update sections, but the newTemplated iPXEtype — despite introducing three new fields (ipxeTemplateId,ipxeTemplateParameters,ipxeTemplateArtifacts) with non-obvious mutual-exclusivity and redaction rules — has zero example anywhere in the file. Given this is the headline feature of the PR, at least one create-request example (and one response example showingauthToken: nullredaction onipxeTemplateArtifacts) would meaningfully help API consumers get the shape right on the first try.
As per path instructions,rest-api/openapi/spec.yaml: "Review the OpenAPI specification for request/response compatibility, schema correctness, required/nullable semantics, examples, operation naming, and consistency with implemented handlers." and "Ensure any REST API model attribute additions/changes and any new endpoint definitions are reflected here."Also applies to: 16742-16849, 17149-17188, 17304-17321
🤖 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 7904 - 8013, The OpenAPI specification lacks examples for the new Templated iPXE operating-system type. Add representative Templated iPXE examples to the relevant list/retrieve response and create/update request sections, including ipxeTemplateId, ipxeTemplateParameters, and ipxeTemplateArtifacts; ensure at least one response example demonstrates authToken redaction as null and reflects the documented exclusivity and nullable semantics.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/workflow/pkg/activity/operatingsystem/operatingsystem.go`:
- Around line 566-577: Update the tenant resolution switch in the Operating
System processing flow so the terr != nil error branch skips assigning
tenantOrgToID[tenantOrg], allowing later Operating Systems for that organization
to retry resolution. Continue caching tenantID for successful lookups and the
len(tenants) == 0 no-tenant result.
---
Outside diff comments:
In `@crates/admin-cli/src/operating_system/common.rs`:
- Around line 123-156: Update SerializableOs and its From<OperatingSystem>
conversion so org remains optional, passing through tenant_organization_id
instead of replacing None with an empty string. Preserve blank formatting only
in the human-readable table rendering path.
---
Nitpick comments:
In `@rest-api/openapi/spec.yaml`:
- Around line 17093-17095: Update the OpenAPI property visibility to constrain
its string values with an enum containing exactly Internal and Public, while
retaining its existing description and schema location.
- Around line 192-194: Update the iPXE Template tag description in the OpenAPI
specification to remove the internal `IpxeTemplateSiteAssociation` model name.
Preserve the customer-facing behavior by describing per-site availability
generically and retaining the guidance that callers can filter with `siteId`.
- Around line 7904-8013: The OpenAPI specification lacks examples for the new
Templated iPXE operating-system type. Add representative Templated iPXE examples
to the relevant list/retrieve response and create/update request sections,
including ipxeTemplateId, ipxeTemplateParameters, and ipxeTemplateArtifacts;
ensure at least one response example demonstrates authToken redaction as null
and reflects the documented exclusivity and nullable semantics.
🪄 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: 6983c4be-407f-4266-86e2-95578f8e4ec4
⛔ Files ignored due to path filters (12)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/sdk/standard/api_ipxe_template.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_operating_system.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_ipxe_template.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_artifact.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_parameter.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_ssh_key_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_identity_config_create_or_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (34)
crates/admin-cli/src/operating_system/common.rscrates/admin-cli/src/operating_system/create/args.rscrates/admin-cli/src/operating_system/show/cmd.rscrates/api-core/src/handlers/operating_system.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/instance_os.rscrates/api-core/tests/integration/operating_system.rscrates/api-db/migrations/20260717120000_operating_systems_org_nullable.sqlcrates/api-db/src/instance.rscrates/api-db/src/operating_system.rscrates/api-model/src/operating_system_definition.rscrates/api-web/src/operating_system.rscrates/rpc/proto/forge.protorest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_templated_os_test.gorest-api/api/pkg/api/handler/instancebatch.gorest-api/api/pkg/api/handler/ipxetemplate.gorest-api/api/pkg/api/handler/ipxetemplate_test.gorest-api/api/pkg/api/handler/operatingsystem.gorest-api/api/pkg/api/handler/operatingsystem_templated_proxy_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/api/pkg/api/model/ipxetemplate.gorest-api/api/pkg/api/model/operatingsystem.gorest-api/api/pkg/api/model/operatingsystem_templated_test.gorest-api/api/pkg/api/routes.gorest-api/api/pkg/api/routes_test.gorest-api/db/pkg/db/model/operatingsystem.gorest-api/db/pkg/db/model/operatingsystem_ipxe_test.gorest-api/db/pkg/migrations/20260717190000_drop_operating_system_ipxe_os_scope.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/proto/core/src/v1/nico_nico.protorest-api/workflow/pkg/activity/operatingsystem/operatingsystem.gorest-api/workflow/pkg/activity/operatingsystem/operatingsystem_test.go
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
rest-api/api/pkg/api/model/operatingsystem.go (1)
1063-1072: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the artifact
cacheStrategyerror message fromvalidCacheStrategiesinstead of hardcoding it.The error text at line 136 (
"cacheStrategy must be one of CacheAsNeeded, LocalOnly, CachedOnly, RemoteOnly") is a separately maintained literal describing the same set already derived fromcdbm.OperatingSystemIpxeArtifactCacheStrategyToProtoMapat lines 1066-1072. If the DB map's strategy set changes, this message can silently drift out of sync.♻️ Proposed fix
var validCacheStrategies = func() map[string]struct{} { m := make(map[string]struct{}, len(cdbm.OperatingSystemIpxeArtifactCacheStrategyToProtoMap)) for name := range cdbm.OperatingSystemIpxeArtifactCacheStrategyToProtoMap { m[name] = struct{}{} } return m }() + +var validCacheStrategyNames = func() []string { + names := make([]string, 0, len(validCacheStrategies)) + for name := range validCacheStrategies { + names = append(names, name) + } + sort.Strings(names) + return names +}()if _, ok := validCacheStrategies[a.CacheStrategy]; !ok { - return validation.Errors{"ipxeTemplateArtifacts": fmt.Errorf("entry %d (%s): cacheStrategy must be one of CacheAsNeeded, LocalOnly, CachedOnly, RemoteOnly", i, a.Name)} + return validation.Errors{"ipxeTemplateArtifacts": fmt.Errorf("entry %d (%s): cacheStrategy must be one of %s", i, a.Name, strings.Join(validCacheStrategyNames, ", "))} }Also applies to: 124-152
🤖 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/operatingsystem.go` around lines 1063 - 1072, Update the cacheStrategy validation error construction in the relevant model validation function to derive the listed accepted values from validCacheStrategies rather than using the hardcoded strategy names. Preserve the existing validation behavior and format the generated message consistently with the current error text.rest-api/openapi/spec.yaml (2)
17093-17095: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConstrain
visibilityto an enum.
IpxeTemplate.visibilityis typed as a free-form string, but its own description states it can only be "Internal or Public." Every comparably-scoped field elsewhere in this spec (AllocationStatus,NetworkSecurityGroupRule.protocol, etc.) uses an explicitenum, which improves validation and produces a proper typed constant in the generated Go SDK instead of a bare string.🩹 Proposed fix
visibility: type: string + enum: + - Internal + - Public description: 'Template visibility: Internal or Public'🤖 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 17093 - 17095, Update the IpxeTemplate visibility schema field to declare an enum containing only Internal and Public, while retaining its existing string type and description.
8038-8038: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRename the new enum value to a single token
Templated iPXEwill be sanitized into awkward generated SDK constants. UseTemplatedIpxein both enum lists instead.🤖 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 8038, Rename the new enum value “Templated iPXE” to the single-token value “TemplatedIpxe” in both enum lists in the OpenAPI specification, preserving all other enum entries unchanged.
🤖 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/operatingsystem.go`:
- Around line 1038-1061: Update validateTemplatedIpxeOS in
rest-api/api/pkg/api/model/operatingsystem.go:1038-1061 to iterate over
oscr.SiteIDs after the non-empty check, parse each value with uuid.Parse, and
return a siteIds validation error for any malformed UUID. Add the “templated
with non-UUID siteId is rejected” case with SiteIDs containing “not-a-uuid” and
expectErr: true in
rest-api/api/pkg/api/model/operatingsystem_templated_test.go:25-47.
In `@rest-api/openapi/spec.yaml`:
- Around line 16999-17103: Add required arrays to the new schemas
OperatingSystemIpxeParameter, OperatingSystemIpxeArtifact, and IpxeTemplate.
Require name and value for parameters, name and url for artifacts, and the
template’s core fields consistent with its documented API contract, including
id, name, template, requiredParams, reservedParams, requiredArtifacts, and
visibility; preserve nullable handling for optional artifact fields and
read-only timestamp behavior.
In `@rest-api/workflow/pkg/activity/operatingsystem/operatingsystem.go`:
- Around line 798-803: The OperatingSystem update path leaves the previous
ownership column populated when ownership changes. In the flow constructing
OperatingSystemUpdateInput, detect whether ownership is tenant-owned or
provider-owned and use OperatingSystemClearInput to null the inactive owner
column while retaining the active owner update; ensure the resulting row has
only one ownership column set.
---
Nitpick comments:
In `@rest-api/api/pkg/api/model/operatingsystem.go`:
- Around line 1063-1072: Update the cacheStrategy validation error construction
in the relevant model validation function to derive the listed accepted values
from validCacheStrategies rather than using the hardcoded strategy names.
Preserve the existing validation behavior and format the generated message
consistently with the current error text.
In `@rest-api/openapi/spec.yaml`:
- Around line 17093-17095: Update the IpxeTemplate visibility schema field to
declare an enum containing only Internal and Public, while retaining its
existing string type and description.
- Line 8038: Rename the new enum value “Templated iPXE” to the single-token
value “TemplatedIpxe” in both enum lists in the OpenAPI specification,
preserving all other enum entries unchanged.
🪄 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: dedd2072-15ab-4e14-9f72-90b9e7c1d70b
⛔ Files ignored due to path filters (12)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/sdk/standard/api_ipxe_template.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_operating_system.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_ipxe_template.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_artifact.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_ipxe_parameter.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_operating_system_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_ssh_key_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_identity_config_create_or_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (34)
crates/admin-cli/src/operating_system/common.rscrates/admin-cli/src/operating_system/create/args.rscrates/admin-cli/src/operating_system/show/cmd.rscrates/api-core/src/handlers/operating_system.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/instance_os.rscrates/api-core/tests/integration/operating_system.rscrates/api-db/migrations/20260717120000_operating_systems_org_nullable.sqlcrates/api-db/src/instance.rscrates/api-db/src/operating_system.rscrates/api-model/src/operating_system_definition.rscrates/api-web/src/operating_system.rscrates/rpc/proto/forge.protorest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_templated_os_test.gorest-api/api/pkg/api/handler/instancebatch.gorest-api/api/pkg/api/handler/ipxetemplate.gorest-api/api/pkg/api/handler/ipxetemplate_test.gorest-api/api/pkg/api/handler/operatingsystem.gorest-api/api/pkg/api/handler/operatingsystem_templated_proxy_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/api/pkg/api/model/ipxetemplate.gorest-api/api/pkg/api/model/operatingsystem.gorest-api/api/pkg/api/model/operatingsystem_templated_test.gorest-api/api/pkg/api/routes.gorest-api/api/pkg/api/routes_test.gorest-api/db/pkg/db/model/operatingsystem.gorest-api/db/pkg/db/model/operatingsystem_ipxe_test.gorest-api/db/pkg/migrations/20260717190000_drop_operating_system_ipxe_os_scope.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/proto/core/src/v1/nico_nico.protorest-api/workflow/pkg/activity/operatingsystem/operatingsystem.gorest-api/workflow/pkg/activity/operatingsystem/operatingsystem_test.go
Add the REST API layer for the Templated iPXE Operating System variant and the read-only iPXE template endpoints: - iPXE template read/list handlers filtered by per-site availability (IpxeTemplateSiteAssociation), with provider- and tenant-account site authorization and an optional siteId filter. - Operating System handler support for the Templated iPXE type: type derivation from request fields, osScope validation/normalization (raw iPXE -> Global; Templated iPXE requires Global/Limited; Image rejects scope), Global auto-expansion to registered tenant sites, and the Core gRPC proxy push branch (image path preserved). Artifact authTokens are redacted in API responses. - Instance / instance-batch wiring for the Templated iPXE OS type. - Route registration, API models, OpenAPI spec (new endpoints, schemas and examples) and regenerated Go SDK. The iPXE template API/SDK/OpenAPI field is named `visibility` (Internal/Public) to match the DB column and the nico-core proto and to avoid confusion with the Operating System `scope` concept.
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
- Strengthen templated proxy tests with strict mock expectations and isolated execute-error/timeout cases - Preserve Deactivated OS status during aggregate proxy sync - Make updateOperatingSystemAggregateStatus transactional and return operation-specific status-detail messages to callers - Normalize imageUrl validation error keys across create/update paths - Validate ipxe-template list query params; assert X-Pagination in tests - Add explicit iPXE template route assertions and OpenAPI tag description
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
…ilable at that site Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
…ite count Removes the Local/Global/Limited OS scope concept. Sync direction for Templated iPXE OS definitions is now derived from the number of associated sites: exactly one site is bidirectional (nico-core drives the definition, most-recent-wins inbound updates + reconciliation-by-absence); two or more makes carbide-rest the source of truth (no inbound definition updates, no absence-deletion). Ownership on inbound create honors nico-core's now-optional tenant_organization_id (tenant-owned when present, provider-owned otherwise). - db: drop ipxe_os_scope column (forward migration) and remove the scope field/constants/filter/Create/Update/Clear handling. - reconcile: replace isLocalScope with a single-site (OSSA count==1) rule; resolve tenant/provider ownership; rework delete-by-absence candidate set. - api: remove scope field + validation; make siteIds mandatory and immutable for Templated iPXE; drop the Global auto-expand branch. - openapi/sdk/docs: remove scope, require siteIds, deprecate raw iPXE. - tests: cover single-site vs multi-site source-of-truth, tenant/provider inbound ownership, and immutable site list.
iPXE and Templated iPXE OS site associations share the operating_system_site_association table but are reconciled exclusively by the OperatingSystem inventory path (UpdateOperatingSystemsInDB). They never appear in the OS Image inventory, so UpdateOsImagesInDB was repeatedly flagging them "missing on Site" and flipping their status to Error every cycle, fighting the OperatingSystem reconcile that resets them to Synced. This surfaced as OS aggregate status flapping Ready<->Error. Scope UpdateOsImagesInDB to image-based associations only, and add a regression test asserting an iPXE association is left untouched while an absent image association is still flagged missing.
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Stop embedding cdbm persistence structs (OperatingSystemIpxeParameter/ OperatingSystemIpxeArtifact) in the Operating System API request/response models. Introduce API-owned DTOs with typed-list validation and DB-model conversion helpers, and give the response artifact type no AuthToken field so stored secrets are dropped structurally rather than by manual redaction.
In the inbound OS reconcile tenant-resolution switch, a transient tenant DAO error cached a nil tenant id for the org, causing every later OS for that org in the same cycle to skip without retrying. Only cache on a successful lookup or a definitive no-tenant result; leave the cache untouched on error so subsequent OSes retry resolution.
pbreton
force-pushed
the
feat/ipxe-templated-os-3
branch
from
July 21, 2026 19:49
f6a0589 to
d1d22da
Compare
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Contributor
Author
|
/retest |
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
pbreton
marked this pull request as ready for review
July 22, 2026 21:30
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
WARNING: draft since this is based on #3689 (waiting to be merged).
This PR continues implementation of Templated iPXE Operating System in NICo REST API. Adds the REST API layer for the Templated iPXE OS variant plus the read-only iPXE template endpoints. This builds on the DB models, migrations, proto conversions and inbound sync landed in the previous PRs — no new DB or workflow changes here; this is purely the external API surface, OpenAPI spec and regenerated Go SDK.
Follow-up PR will implement Provider-specific behavior and capabilities.
Related issues
Builds on #3232
Type of Change
Breaking Changes
Testing