HYPERFLEET-437 - refactor: changes to adopt oapi-codegen #21
Conversation
WalkthroughThe PR updates the API specification version from 1.0.1 to 1.0.2 and performs a systematic structural refactoring of data model definitions across TypeSpec and OpenAPI schemas. Model inheritance patterns are converted to composition using spread syntax throughout TypeSpec files. Common models gain generics (List), while OpenAPI schemas are flattened by replacing allOf-based inheritance with explicit field declarations. Pagination metadata is standardized across list schemas. Status and condition representations are reorganized with explicit fields. Changes affect Cluster, NodePool, and AdapterStatus resource definitions and their related request/response and list models. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🔇 Additional comments (53)
Comment |
ciaranRoche
left a comment
ciaranRoche
left a comment
There was a problem hiding this comment.
Overall looks good to me, just some minor comments
rh-amarin
force-pushed
the
avoid-extends
branch
from
9e2460a to
fb40c94
Compare
rh-amarin
force-pushed
the
avoid-extends
branch
from
fb40c94 to
46284c2
Compare
| additionalProperties: {} | ||
| description: Adapter-specific data (structure varies by adapter type) | ||
| description: Base fields shared by AdapterStatus and AdapterStatusCreateRequest | ||
| AdapterStatusCreateRequest: |
There was a problem hiding this comment.
The AdapterStatusCreateRequest and ConditionRequest schemas were removed from gcp/openapi.yaml when eliminating the allOf inheritance pattern, but their flattened versions weren't added back.
These schemas exist in core/openapi.yaml (lines 546-628) but are missing from the GCP spec. Since the GCP spec also has the POST /clusters/{cluster_id}/statuses endpoint, it needs these schemas.
Suggested fix: Copy the flattened AdapterStatusCreateRequest and ConditionRequest schemas from core/openapi.yaml to gcp/openapi.yaml.
There was a problem hiding this comment.
There should be no POST /clusters/{cluster_id}/statuses endpoint in the GCP contract, since to the final user of the GCP contract, they don't need to POST to statuses, only the adapters do, and for that, they use the core contract.
Or did I misunderstand your comment?
|
Several important documentation comments were removed during the refactoring:
These comments provide valuable context about system behavior. Consider preserving them even when changing from Example fix for /** Cluster specification - CLM stores and forwards to adapters without unmarshalling */
spec: ClusterSpec; |
| - adapter | ||
| - observed_generation | ||
| type: object | ||
| AdapterStatusCreateRequest: |
There was a problem hiding this comment.
Same issue as gcp/openapi.yaml - the gcp/swagger.yaml file is also missing the flattened versions of:
AdapterStatusCreateRequestConditionRequest
These schemas were removed when eliminating inheritance but not replaced with their flattened equivalents.
The core/openapi.yaml has these schemas properly flattened, so please ensure consistency across all spec files.
I commented to a similar ask from Ciaran, some descriptions introduced some change to the openapi.yaml generated that didn't go well for the generators. But, in either case, I think we will be revisiting the documentation of our API in later stages, to polish what goes really into the public OpenAPI contract. e.g. It makes little sense to document anything about a database column in an API spec, or unmarshalling details |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ciaranRoche 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 21cd118
into
openshift-hyperfleet:main
3 participants
This PR changes the openapi.yaml definition to avoid errors for go type generators different than the Java official one
e.g. using allOf+base properties for inheritance causes errors for different generators (oapi-codegen, ogen)
microsoft/typespec#826
These generators can read
allOfin the openapi.yaml, but not in the form that typespec.io produces it, so theopenapi.yamlhad to be post-processed. This makes for a worse experience using Typespec, so I opted to remove the use ofextendsand make the schema definitions more "plain"The comments above structs was also causing some issues with the generators, they can use them, but create more complex structures in go types code.
Updated version to v1.0.2
Summary by CodeRabbit
Release Notes - Version 1.0.2
Chores
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.