-
Notifications
You must be signed in to change notification settings - Fork 6
feat: standardize TypeSpec schema definitions with enums and validation enhancements #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR standardizes status enums and normalizes the HTTP bearer auth scheme across schemas and examples. It adds public enums ResourcePhase (NotReady, Ready, Failed) and ConditionStatus (True, False, Unknown); replaces inline string literals with these types in ClusterStatus, NodePoolStatus, ConditionBase, and ConditionRequest; adds Error.details for field-level validation errors; removes the Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
models/common/model.tsp (1)
60-66: Consider tightening theError.detailselement shape
fieldanderrorare both optional; if your validation pipeline always produces an error message (and usually a field path), making at leasterrorrequired would strengthen the schema and improve client expectations. If you need flexibility for non-field-scoped errors, the current shape is fine.models/statuses/model.tsp (1)
4-11: Enum-ifying condition status is aligned; consider makingUnknownexplicitUsing
ConditionStatusinstead of raw string unions is a nice improvement. For readability and future‑proofing, you might optionally declareUnknown: "Unknown"as well so all members have explicit string values, mirroring the previous wire contract.schemas/core/openapi.yaml (2)
612-625: Clarify the purpose of the BearerAuth object schema.This
BearerAuthobject schema is separate from thesecuritySchemes.BearerAuthat line 1198. Consider adding a description to clarify its purpose (e.g., for validation, documentation, or internal use), as having both a schema and a security scheme with the same name could cause confusion for API consumers.
863-874: Consider makingfieldanderrorrequired within detail items.The
detailsarray items have bothfieldanderroras optional properties. For better API consistency and client usability, consider making them required — an error detail without specifying which field failed or what the error message is provides limited value.details: type: array items: type: object + required: + - field + - error properties: field: type: string description: Field path that failed validation error: type: string description: Validation error message for this field description: Field-level validation errors (optional)schemas/gcp/openapi.yaml (2)
646-659: Same observation as core schema: clarify BearerAuth object purpose.This
BearerAuthobject schema mirrors the one inschemas/core/openapi.yaml. Same suggestion applies — consider adding a description to distinguish it from the security scheme.
941-952: Same recommendation: consider requiringfieldanderrorin detail items.For consistency with the core schema recommendation, consider making
fieldanderrorrequired within each error detail item.details: type: array items: type: object + required: + - field + - error properties: field: type: string description: Field path that failed validation error: type: string description: Validation error message for this field description: Field-level validation errors (optional)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
main.tsp(1 hunks)models-core/cluster/example_cluster.tsp(2 hunks)models-core/nodepool/example_nodepool.tsp(2 hunks)models-gcp/cluster/example_cluster.tsp(2 hunks)models-gcp/nodepool/example_nodepool.tsp(2 hunks)models/clusters/model.tsp(1 hunks)models/common/model.tsp(3 hunks)models/nodepools/model.tsp(1 hunks)models/statuses/example_adapter_status.tsp(4 hunks)models/statuses/model.tsp(3 hunks)schemas/core/openapi.yaml(9 hunks)schemas/gcp/openapi.yaml(10 hunks)services/compatibility.tsp(1 hunks)
🔇 Additional comments (24)
models/common/model.tsp (2)
13-20: CentralizingResourcePhaseas an enum looks goodThe enum cleanly captures the three lifecycle phases and removes scattered string literals; this should make downstream status models and examples safer and more consistent.
114-114: Double‑check behavior changes from removing theorderdefaultDropping the
OrderDirection.descdefault means generated clients and documentation will no longer advertise/supply a default sort order. If the backend still assumesdescwhenorderis omitted, that’s OK, but please confirm consumers aren’t relying on the explicit default in the schema.models/statuses/model.tsp (1)
25-25: SharedConditionStatustype for condition/status requests looks consistentWiring both
ConditionBase.statusandConditionRequest.statusthrough the sameConditionStatusenum keeps adapters, status computation, and schemas in sync and reduces the chance of drift between request and stored status values.Also applies to: 84-84
models-core/cluster/example_cluster.tsp (1)
15-15: Examples now correctly use the new enumsUsing
ResourcePhase.ReadyforphaseandConditionStatus.Truefor condition statuses keeps this example in lockstep with the updated model typings and OpenAPI enums.Also applies to: 22-22, 32-32
models-core/nodepool/example_nodepool.tsp (1)
21-21: Enum usage in nodepool example matches the updated status models
ResourcePhase.ReadyandConditionStatus.Trueare correctly referenced here, so the example will validate against the new NodePoolStatus and ConditionStatus definitions.Also applies to: 28-28, 38-38
services/compatibility.tsp (1)
20-20: Explicitly targetingHyperFleet.BearerAuthis appropriate; confirm all callers are migratedQualifying
@useAuthwithHyperFleet.BearerAuthcorrectly ties this operation to your custom lowercase‑bearerscheme instead of the library’s default. Please also confirm that any other operations meant to use the same scheme have been updated to referenceHyperFleet.BearerAuthso security is consistent across the API surface and matches the regenerated OpenAPI.models-gcp/cluster/example_cluster.tsp (1)
37-37: GCP cluster example correctly adoptsResourcePhaseandConditionStatusUsing
ResourcePhase.ReadyandConditionStatus.Truehere keeps the GCP example in sync with the shared core status abstractions and regenerated OpenAPI schemas.Also applies to: 44-44, 54-54
main.tsp (1)
26-30: CustomBearerAuthoverride is shaped correctly; validate emitter outputDefining
HyperFleet.BearerAuthwithtype: AuthType.httpandscheme: "bearer"matches the expected custom HTTP auth scheme shape and should give kin-openapi the lowercase scheme it needs. Please confirm:
AuthTyperesolves from the HTTP library in this file (viausing Http;), and- The generated OpenAPI
components.securitySchemesnow emitscheme: bearereverywhere you expect.models/nodepools/model.tsp (1)
30-30: UsingResourcePhaseforNodePoolStatus.phaseimproves consistencySwitching
phaseto the sharedResourcePhaseenum de‑duplicates the phase definition and keeps nodepool and cluster statuses aligned. Just make sure any persistence/serialization logic still expects the same underlying values when reading/writing this column.models/clusters/model.tsp (1)
37-37: Good use of shared enum type for phase.Using
ResourcePhaseinstead of inline string literals improves type safety and ensures consistency acrossClusterStatusandNodePoolStatusdefinitions.models/statuses/example_adapter_status.tsp (3)
10-10: Consistent enum usage across all example adapter statuses.All occurrences of
status: "True"are correctly replaced withConditionStatus.True. The updated Health condition messages ("All adapter operations completed successfully") are more generic and appropriate for a reusable example.Also applies to: 17-17, 24-27
57-57: Consistent with the first example object.The
exampleAdapterStatusCreateRequestcorrectly mirrors the enum usage pattern.Also applies to: 63-63, 69-71
104-104: List example correctly updated.The
exampleAdapterStatusListitems useConditionStatus.Trueconsistently.Also applies to: 123-123
schemas/core/openapi.yaml (7)
756-761: Correct use of allOf for enum reference with description.Using
allOfwith$reftoResourcePhasewhile preserving the field description is the idiomatic OpenAPI 3.0 pattern for this use case.
839-845: ConditionStatus enum follows Kubernetes conventions.The quoted string values
'True','False', andUnknowncorrectly match the Kubernetes condition status convention.
1190-1196: ResourcePhase enum properly defined.The enum values
NotReady,Ready, andFailedwith the description provide clear semantics for resource lifecycle phases.
806-808: Consistent enum references for condition status fields.Both
ConditionBase.statusandConditionRequest.statuscorrectly reference the sharedConditionStatusenum.Also applies to: 831-831
1197-1200: Correct: lowercasebearerfor OpenAPI validator compatibility.The change to lowercase
beareraligns with kin-openapi requirements. While RFC 7235 treats scheme names as case-insensitive, using lowercase ensures broader compatibility with OpenAPI tooling.
469-470: Example messages updated consistently.The Health condition messages in examples are now more generic and align with the TypeSpec example updates.
Also applies to: 557-558
1110-1115: Consistent with ClusterStatus.phase pattern.NodePoolStatus.phase correctly uses the same
allOfpattern to referenceResourcePhase.schemas/gcp/openapi.yaml (3)
829-830: ConditionStatus and ResourcePhase references correctly applied.All status/phase fields properly reference the shared enum types, consistent with the core schema.
Also applies to: 879-880, 904-904, 912-918
1315-1316: GCP schema changes are consistent with core schema.The
ResourcePhaseenum,NodePoolStatus.phasereference, and lowercasebearersecurity scheme are all correctly synchronized withschemas/core/openapi.yaml.Also applies to: 1414-1420, 1437-1440
480-481: Example message updates consistent with core schema.Health condition messages updated to match the core schema and TypeSpec examples.
Also applies to: 568-569
models-gcp/nodepool/example_nodepool.tsp (1)
43-43: EnsureResourcePhaseandConditionStatusenums are accessible through imports.The changes correctly use
ResourcePhase.ReadyandConditionStatus.Trueenums instead of string literals. Verify that bothResourcePhaseandConditionStatusare properly exported from their respective definition modules and accessible through the import chain frommodels/nodepools/model.tsp.Applies to lines: 43, 50, 60
|
/lgtm |
Summary
This PR standardizes the TypeSpec schema definitions to improve type safety and align with OpenAPI validation
requirements.
Changes
Type Safety Improvements
ResourcePhaseenum: Defines cluster/nodepool phases (NotReady, Ready, Failed)ConditionStatusenum: Defines condition status values (True, False, Unknown)Validation Enhancements
detailsarray field for field-level validation errors with field path and error messageOpenAPI Compatibility
library requirements
Schema Cleanup
kindfield from AdapterStatusList to follow List patternGenerated Schemas
schemas/core/openapi.yamlandschemas/gcp/openapi.yamlwith all updatesImpact
Summary by CodeRabbit
New Features
Refactor
API Changes
✏️ Tip: You can customize this high-level summary in your review settings.