Refactor NodePool models to include generation field #20
Conversation
|
Warning Rate limit exceeded@rh-amarin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds a required integer field Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
models/nodepools/model.tsp (1)
71-78: Create request/response spreads keep NodePool shapes in syncUsing
...NodePoolBase;forNodePoolCreateRequestand...NodePool;forNodePoolCreateResponseensures the wire format for create flows stays consistent with the main NodePool model, including the newgenerationfield, without duplicating field lists.If you ever need a variant create response, consider a small wrapper model over
NodePoolrather than changing this spread, to avoid subtle generator differences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
models-core/nodepool/example_nodepool.tsp(1 hunks)models-gcp/nodepool/example_nodepool.tsp(1 hunks)models/clusters/model.tsp(1 hunks)models/nodepools/model.tsp(2 hunks)schemas/core/openapi.yaml(5 hunks)schemas/gcp/openapi.yaml(5 hunks)schemas/gcp/swagger.yaml(5 hunks)
🔇 Additional comments (11)
models-core/nodepool/example_nodepool.tsp (1)
11-11: Core NodePool example correctly includes the newgenerationfieldThe example now matches the NodePool model and schemas (
generationpresent and starting at1), so consumers will see a consistent representation across specs and examples.models-gcp/nodepool/example_nodepool.tsp (1)
10-10: GCP NodePool example aligned with newgenerationfieldThe GCP example now exposes
generation: 1, matching the shared NodePool model and the updated GCP schemas.models/clusters/model.tsp (1)
64-73: Cluster model layout now clearly composesAPICreatedResourceSpreading
...APICreatedResource;underCluster extends ClusterBasewith the existinggenerationandstatusfields matches the documented Cluster schemas and improves readability without changing semantics.schemas/gcp/swagger.yaml (3)
1058-1148: Swagger NodePool schema correctly exposes requiredgeneration
NodePoolnow has a requiredgenerationproperty (int32,minimum: 1with clear description) and the example includesgeneration: 1, matching the TypeSpec model and OpenAPI schemas.
1241-1302: NodePoolCreateResponse includesgenerationin both schema and required setThe create response now returns the same
generationfield as the main NodePool resource (int32,minimum: 1, same description) and marks it required, which is consistent with the TypeSpecNodePoolCreateResponsespreading the fullNodePool.
1397-1409:NodePoolStatus.observed_generationconstrained tominimum: 1Adding
minimum: 1forobserved_generationaligns the status schema with the generation semantics used elsewhere (no generation 0) and matches the TypeSpec annotations for NodePool status.schemas/core/openapi.yaml (2)
901-947: Core NodePool schema updated to requiregenerationwith consistent semanticsThe NodePool schema now has a required
generationfield (int32,minimum: 1, matching the descriptive text) and the example includesgeneration: 1, in line with the TypeSpec model and status/condition semantics.
1041-1070: NodePoolCreateResponse now returns thegenerationfieldMaking
generationa required property onNodePoolCreateResponsewith the same constraints asNodePool.generationensures the initial create response fully reflects the resource’s generation state.models/nodepools/model.tsp (1)
57-69: NodePool model cleanly composes base, created metadata, and newgenerationExtending
NodePoolBase, spreadingAPICreatedResource, and addinggeneration(@minValue(1)),owner_references, andstatusproduces exactly the NodePool shape advertised in the specs, and keeps semantics aligned with Cluster.schemas/gcp/openapi.yaml (2)
955-999: GCP NodePool schema now exposes requiredgenerationAdding a required
generationproperty (int32,minimum: 1with the same description) plusgeneration: 1in the example keeps the GCP NodePool definition consistent with the shared NodePool model and core schemas.
1133-1162: GCP NodePoolCreateResponse includesgenerationwith matching constraints
NodePoolCreateResponsenow returnsgenerationand marks it required, using the same type and description asNodePool.generation, so create responses and subsequent GETs stay structurally aligned.
|
LGTM. Waiting others to review. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
main.tsp(1 hunks)schemas/core/openapi.yaml(6 hunks)schemas/gcp/openapi.yaml(6 hunks)schemas/gcp/swagger.yaml(6 hunks)
🔇 Additional comments (9)
schemas/gcp/swagger.yaml (3)
730-736: LGTM!The
generationfield definition for Cluster is well-structured with appropriate constraints (int32, minimum 1) and a clear description.
1123-1129: LGTM!The
generationfield definition for NodePool matches the Cluster schema and is correctly configured.
1248-1254: LGTM!Including
generationin NodePoolCreateResponse ensures clients receive the initial generation value upon resource creation.schemas/core/openapi.yaml (3)
650-654: LGTM!Cluster
generationfield is consistently defined across OpenAPI schemas.
923-927: LGTM!NodePool
generationfield is consistently defined across OpenAPI schemas.
1065-1069: LGTM!NodePoolCreateResponse includes
generationconsistently with other schemas.schemas/gcp/openapi.yaml (3)
609-613: LGTM!Cluster
generationfield definition is consistent across all OpenAPI/Swagger schemas.
977-981: LGTM!NodePool
generationfield definition is consistent across all OpenAPI/Swagger schemas.
1157-1161: Approved. Schema definition is consistent across models.The
generationfield is properly defined with@minValue(1)constraint in the TypeSpec model and documented consistently with its purpose. Note: database migration implementation details belong in the application repository, not in this API specification repository.
| */ | ||
| @service(#{ title: "HyperFleet API" }) | ||
| @info(#{ version: "1.0.0", contact: #{ name: "HyperFleet Team" }, license: #{ name: "Apache 2.0" ,url: "https://www.apache.org/licenses/LICENSE-2.0"} }) | ||
| @info(#{ version: "1.0.1", contact: #{ name: "HyperFleet Team" }, license: #{ name: "Apache 2.0" ,url: "https://www.apache.org/licenses/LICENSE-2.0"} }) |
There was a problem hiding this comment.
Bump to major version instead of patch.
Adding a new required field to API responses is a breaking change per semantic versioning standards. Breaking changes require a new major API version, so this should be 2.0.0 rather than 1.0.1.
🤖 Prompt for AI Agents
In main.tsp around line 23, the API version was updated only as a patch (1.0.1)
despite introducing a breaking change (new required response field); update the
version string to a new major release (2.0.0) in the @info annotation so the API
follows semantic versioning for breaking changes.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
schemas/gcp/swagger.yaml (1)
150-435: Missing BearerAuth on nodepool endpoints.BearerAuth is applied to cluster operations (lines 74-75, 102-103, 146-147) but is missing from all nodepool-related endpoints:
getNodePoolsByClusterId(line 205)createNodePool(line 237)getNodePoolById(line 266)getNodePoolsStatuses(line 328)getClusterStatuses(line 383)getNodePools(line 435)This creates an inconsistent security posture where cluster resources require authentication but nodepool resources don't.
Apply this pattern to secure nodepool endpoints (example for one endpoint):
operationId: getNodePoolsByClusterId summary: List all nodepools for cluster + security: + - BearerAuth: []Repeat for all nodepool-related operations.
schemas/gcp/openapi.yaml (1)
106-293: Missing BearerAuth on nodepool endpoints.Consistent with the security gap in swagger.yaml, nodepool endpoints lack authentication requirements:
getNodePoolsByClusterId(line 108)createNodePool(line 139)getNodePoolById(line 172)getNodePoolsStatuses(line 205)getClusterStatuses(line 242)getNodePools(line 270)Add security to each nodepool operation:
get: operationId: getNodePoolsByClusterId summary: List all nodepools for cluster + security: + - BearerAuth: []schemas/core/openapi.yaml (1)
106-368: Missing BearerAuth on nodepool endpoints.Consistent with the GCP specifications, nodepool operations lack authentication:
getNodePoolsByClusterId(line 108)createNodePool(line 139)getNodePoolById(line 172)- Status-related endpoints
This creates a security gap in the core API specification.
Add security requirements to nodepool operations:
get: operationId: getNodePoolsByClusterId + security: + - BearerAuth: []
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
schemas/core/openapi.yaml(11 hunks)schemas/gcp/openapi.yaml(11 hunks)schemas/gcp/swagger.yaml(17 hunks)services/clusters.tsp(1 hunks)
🔇 Additional comments (20)
schemas/gcp/swagger.yaml (7)
19-19: LGTM: API version bumped to 1.0.1.The version bump aligns with the addition of the
generationfield and security enhancements.
54-62: LGTM: Enhanced SearchParams documentation with TSL examples.The expanded description with Tree Search Language examples improves API usability.
74-75: LGTM: BearerAuth security applied to cluster endpoints.Authentication is properly configured for cluster operations.
Also applies to: 102-103, 146-147
766-772: LGTM: Generation field properly added to Cluster schema.The
generationfield is correctly defined as a required integer (int32, minimum 1) with appropriate documentation. The field is server-managed (not in ClusterCreateRequest) which is the correct pattern.Also applies to: 786-787
1096-1096: LGTM: Generation field properly added to NodePool schema.The
generationfield is correctly defined as required with matching constraints (int32, minimum 1) and included in the example payload.Also applies to: 1159-1165, 1181-1181
1284-1290: LGTM: Generation field added to NodePoolCreateResponse.The
generationfield is properly included in the create response, maintaining consistency with the server-managed generation pattern.Also applies to: 1333-1333
1542-1546: LGTM: BearerAuth security scheme properly defined.The security definition follows OpenAPI 2.0 (Swagger) conventions for API key authentication in the Authorization header.
schemas/gcp/openapi.yaml (6)
4-4: LGTM: API version bumped to 1.0.1.Version bump is consistent across all specification files.
333-338: LGTM: SearchParams enhanced with TSL documentation.The Tree Search Language examples improve developer experience.
43-44: LGTM: BearerAuth applied to cluster endpoints.Security is properly configured for cluster operations using OpenAPI 3.0 security syntax.
Also applies to: 76-77, 104-105
970-970: LGTM: Generation field added to NodePool schema.The field is properly defined with correct constraints and included in the example.
Also applies to: 986-990, 1005-1005
1148-1148: LGTM: Generation field added to NodePoolCreateResponse.Correctly included as a required field in the create response.
Also applies to: 1166-1170
1386-1389: LGTM: BearerAuth security scheme properly defined.The OpenAPI 3.0 format (
type: http, scheme: bearer) is correct for HTTP Bearer authentication.schemas/core/openapi.yaml (6)
4-4: LGTM: API version bumped to 1.0.1.Version bump is consistent across all API specifications.
408-411: LGTM: SearchParams documentation enhanced with TSL syntax.The expanded description with examples improves API discoverability.
43-44: LGTM: BearerAuth applied to cluster endpoints.Security requirements properly configured for cluster operations.
Also applies to: 76-77, 104-105
916-916: LGTM: Generation field added to NodePool schema.The field is properly defined with int32 format and minimum constraint of 1. The example correctly includes
generation: 1.Also applies to: 932-936, 952-952
1056-1056: LGTM: Generation field added to NodePoolCreateResponse.Correctly marked as required in the create response, following the server-managed generation pattern.
Also applies to: 1074-1078
1221-1224: LGTM: BearerAuth security scheme defined.The OpenAPI 3.0 security scheme is correctly configured for HTTP Bearer token authentication.
services/clusters.tsp (1)
15-15: LGTM: Authentication added to Clusters interface.The
@useAuth(HyperFleet.BearerAuth)decorator properly secures all cluster operations and aligns with the BearerAuth security requirements in the OpenAPI specifications. The NodePools interface already includes the same authentication treatment, with both services properly generating BearerAuth security definitions in the OpenAPI specs.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
schemas/gcp/swagger.yaml (1)
274-334: Remove incorrect references to POST operations on NodePool statuses endpoint.The GET operation (
getNodePoolsStatuses) for/api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}/statuseslacks a security definition, while the corresponding cluster-level GET operation (getClusterStatuses) includesBearerAuthsecurity. No POST operations exist on either endpoint. Align the NodePool GET operation with the cluster security pattern.schemas/gcp/openapi.yaml (1)
209-245: AddBearerAuthsecurity togetNodePoolsStatusesoperation.The GET operation
getNodePoolsStatuses(lines 209-245) lacks a security block, while the cluster-levelgetClusterStatusesoperation includesBearerAuthsecurity (lines 274-275). Add the missing security requirement to maintain consistency:security: - BearerAuth: []
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
schemas/core/openapi.yaml(17 hunks)schemas/gcp/openapi.yaml(16 hunks)schemas/gcp/swagger.yaml(22 hunks)services/nodepools.tsp(1 hunks)services/statuses-internal.tsp(1 hunks)services/statuses.tsp(1 hunks)
🔇 Additional comments (16)
services/statuses-internal.tsp (1)
14-14: LGTM!The addition of bearer authentication to the ClusterStatusesInternal interface aligns with the broader security enhancements across the API surface.
services/statuses.tsp (1)
14-14: LGTM!Bearer authentication properly added to the ClusterStatuses interface, consistent with the security enhancements throughout the API.
services/nodepools.tsp (1)
14-14: LGTM!Bearer authentication correctly applied to the NodePools interface, maintaining consistency with other API interfaces.
schemas/core/openapi.yaml (5)
4-4: LGTM!Version bump to 1.0.1 appropriately reflects the addition of the generation field and security enhancements.
420-422: LGTM!Enhanced documentation for the search parameter with TSL (Tree Search Language) examples improves API usability.
656-675: LGTM!The generation field additions to Cluster, NodePool, and NodePoolCreateResponse schemas are consistent and well-defined with appropriate constraints (int32, minimum: 1).
Also applies to: 928-948, 964-964, 1068-1090
1233-1236: LGTM!The BearerAuth security scheme is properly defined and consistently applied across most operations.
209-249: Verify missing security requirement on postNodePoolStatuses.The
postNodePoolStatusesoperation (lines 209-249) does not have asecurityblock, while similar POST operations likepostCluster(lines 76-77) andpostClusterStatuses(lines 322-323) includeBearerAuthsecurity. This inconsistency may be intentional if this is an internal adapter endpoint that uses a different authentication mechanism, but it should be verified.Run the following script to check if other internal status endpoints also lack security blocks:
#!/bin/bash # Check for security blocks on status-related POST operations across OpenAPI schemas echo "=== Checking postNodePoolStatuses security in core/openapi.yaml ===" rg -A 5 'operationId: postNodePoolStatuses' schemas/core/openapi.yaml | rg -E '(operationId|security:)' echo -e "\n=== Checking postClusterStatuses security in core/openapi.yaml ===" rg -A 5 'operationId: postClusterStatuses' schemas/core/openapi.yaml | rg -E '(operationId|security:)' echo -e "\n=== Checking internal status services in typespec ===" rg -B 2 -A 2 'postNodePoolStatuses|postClusterStatuses' services/statuses-internal.tspschemas/gcp/swagger.yaml (4)
19-19: LGTM!Version bump to 1.0.1 is consistent with the changes in this PR.
54-58: LGTM!Enhanced SearchParams documentation with TSL query syntax examples improves developer experience and API discoverability.
Also applies to: 122-126, 184-188, 312-316, 369-373, 421-425, 1589-1593
717-782: LGTM!Generation field additions to Cluster, NodePool, and NodePoolCreateResponse definitions are consistent with the core OpenAPI schema and properly constrained (int32, minimum: 1).
Also applies to: 796-796, 1106-1175, 1191-1191, 1294-1300, 1343-1343
1552-1556: LGTM!BearerAuth security definition is properly configured as an apiKey in the Authorization header.
schemas/gcp/openapi.yaml (4)
4-4: LGTM!Version bump to 1.0.1 appropriately reflects the API changes in this PR.
343-345: LGTM!Enhanced SearchParams documentation with TSL examples improves API usability and developer experience.
613-632: LGTM!Generation field additions to Cluster, NodePool, and NodePoolCreateResponse schemas are consistent and properly constrained across all schema files.
Also applies to: 667-667, 980-1000, 1015-1015, 1158-1180
1396-1399: LGTM!BearerAuth security scheme is properly defined and aligns with the security enhancements across the API.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
schemas/gcp/swagger.yaml (1)
274-334: Missing BearerAuth security ongetNodePoolsStatusesendpoint.The
getNodePoolsStatusesendpoint (lines 274-334) lacks thesecurity: - BearerAuth: []block that's present on similar endpoints likegetClusterStatuses. This inconsistency could allow unauthenticated access.Verify this is intentional, or add:
schema: $ref: '#/definitions/Error' + security: + - BearerAuth: [] description: Returns adapter status reports for this nodepool operationId: getNodePoolsStatusesschemas/core/openapi.yaml (2)
250-285: Missing BearerAuth security ongetNodePoolsStatusesendpoint.Same issue as in
swagger.yaml- this GET endpoint lacks authentication while similar status endpoints have it. Add security if this should be protected:schema: $ref: '#/components/schemas/Error' + security: + - BearerAuth: []
209-249: Missing BearerAuth security onpostNodePoolStatusesendpoint.The
postNodePoolStatusesendpoint lacks authentication, while the similarpostClusterStatusesendpoint (lines 322-323) hasBearerAuth. If both should be protected, add:requestBody: required: true content: application/json: schema: $ref: '#/components/schemas/AdapterStatusCreateRequest' + security: + - BearerAuth: []schemas/gcp/openapi.yaml (1)
209-245: Missing BearerAuth security ongetNodePoolsStatusesendpoint.Same issue as the other schema files - this endpoint lacks authentication. For consistency, add:
schema: $ref: '#/components/schemas/Error' + security: + - BearerAuth: []
🧹 Nitpick comments (1)
schemas/gcp/swagger.yaml (1)
697-710: Note: BearerAuth defined both as schema and security definition.The
BearerAuthappears in bothdefinitions(as a data schema) andsecurityDefinitions(as the actual security scheme). This is likely auto-generated from TypeSpec. The schema definition is unused for authentication purposes but doesn't cause harm.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
models/clusters/model.tsp(2 hunks)models/nodepools/model.tsp(2 hunks)schemas/core/openapi.yaml(21 hunks)schemas/gcp/openapi.yaml(20 hunks)schemas/gcp/swagger.yaml(26 hunks)
🔇 Additional comments (12)
models/clusters/model.tsp (2)
11-14: Breaking change: name validation constraints tightened.The
minLengthincreased from 1 to 3, and the pattern now requires names to start and end with alphanumeric characters (no leading/trailing hyphens). Existing clusters with 1-2 character names or names like-fooorfoo-will fail validation on update/creation.Ensure this breaking change is intentional and documented in migration notes. If existing resources need to be grandfathered, consider applying validation only to new resources.
64-74: LGTM!The
Clustermodel correctly extendsClusterBase, spreadsAPICreatedResource, and includes the newgenerationfield with proper validation (@minValue(1)). The documentation clearly explains the purpose of the generation field.models/nodepools/model.tsp (3)
11-14: Breaking change: name validation constraints added.Same concern as
ClusterBase-minLength(3)and the stricter pattern will reject previously valid names. Ensure this is documented as a breaking change.
61-72: LGTM!The
NodePoolmodel correctly extendsNodePoolBase, includesAPICreatedResource, and adds thegenerationfield with proper validation. Theowner_referencesfield as a singularObjectReferencealigns with the single-owner relationship to a parent Cluster.
74-81: LGTM!Correct design:
NodePoolCreateRequestexcludesgeneration(server-managed), whileNodePoolCreateResponseincludes the fullNodePoolwith the server-assigned generation.schemas/gcp/swagger.yaml (2)
19-19: LGTM!Version bump to 1.0.1 is appropriate given the API surface changes (new
generationfield, stricter name validation, BearerAuth security).
1169-1194: LGTM!The
generationfield is correctly added toNodePoolwith proper constraints (format: int32,minimum: 1) and included in therequiredlist. The field description clearly explains its purpose.schemas/core/openapi.yaml (2)
1242-1245: LGTM!The
BearerAuthsecurity scheme is correctly defined using OpenAPI 3.0 format (type: http,scheme: bearer).
671-675: LGTM!The
generationfield is correctly defined withtype: integer,format: int32,minimum: 1, and appropriate documentation. Consistent with the TypeSpec model definition.schemas/gcp/openapi.yaml (3)
4-4: LGTM!Version bump to 1.0.1 is consistent with other schema files.
996-1000: LGTM!The
generationfield is correctly added with proper constraints and documentation, consistent across all schema files.
1405-1408: LGTM!The
BearerAuthsecurity scheme is correctly defined using OpenAPI 3.0 format.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit f793d93
into
openshift-hyperfleet:main
2 participants
generationfield to NodePool objectssecuritySchematoopenapi.yamlthat was removed when the/compatibilityendpoint was removedSummary by CodeRabbit
New Features
Chores
Documentation
Validation
Style
✏️ Tip: You can customize this high-level summary in your review settings.