Enhance API Schema and Build Process #16
Conversation
WalkthroughThis PR separates public and internal API surfaces: POST adapter-status endpoints were removed from the public spec and added to a new internal interface (services/statuses-internal.tsp). The compatibility service and its models were deleted. Several GET endpoints had Bad Request (400) responses added. OpenAPI generation and build tooling were revised (default OpenAPI 3.0 output, optional OpenAPI 2.0/Swagger via api-spec-converter, new npm scripts, and build-schema.sh CLI changes). API metadata now includes an Apache 2.0 license and production server URL. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🔇 Additional comments (6)
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: 2
🧹 Nitpick comments (7)
build-schema.sh (2)
16-39: Clarify provider handling and clean up now‑unreachable validationThe new arg parsing +
PROVIDER="${PROVIDER:-core}"default means the “provider argument is required” check is no longer reachable. Also, any second positional arg is silently ignored.Consider tightening this to avoid surprising behavior:
- Either keep the “core default” behavior and remove the
if [ -z "$PROVIDER" ]; thenvalidation block, or- Drop the default and truly require an explicit provider.
Optionally, treat a second non‑option arg as an error to catch typos early:
- *) - if [ -z "$PROVIDER" ]; then - PROVIDER="$arg" - fi - ;; + *) + if [ -z "$PROVIDER" ]; then + PROVIDER="$arg" + else + echo -e "${RED}Error: Unexpected extra argument: $arg${NC}" + echo "Usage: $0 [provider] [--swagger|--openapi2]" + exit 1 + fi + ;;Also applies to: 45-52
84-91: Improve Swagger conversion failure diagnosticsThe api-spec-converter preflight check is helpful, and step 4 handles failures gracefully, but all converter stderr is discarded:
... > "${OUTPUT_DIR}/swagger.yaml" 2>/dev/nullFor easier debugging when conversion fails (e.g., due to unsupported OpenAPI 3 features), consider preserving or at least surfacing a summarized error, for example:
- "${OUTPUT_DIR}/openapi.yaml" > "${OUTPUT_DIR}/swagger.yaml" 2>/dev/null; then + "${OUTPUT_DIR}/openapi.yaml" > "${OUTPUT_DIR}/swagger.yaml"; thenor redirecting stderr to a side log file and pointing users to it in the failure message.
Also applies to: 148-166
schemas/gcp/openapi.yaml (1)
7-9: OpenAPI GCP schema updates match the new API surfaceThe additions of the Apache 2.0 license, the consistent 400 Bad Request entries across key endpoints, and the production server URL all line up with the TypeSpec/service changes and README documentation.
If you intend 400s to carry the same structured error payload as other failures, consider wiring them to the
Errorschema (or a dedicated bad‑request schema) instead of leaving them description‑only, so client generators can treat them consistently. Otherwise, as pure “syntax error” responses without body, this is coherent as‑is.Also applies to: 35-36, 60-61, 92-93, 124-125, 150-151, 189-190, 226-227, 280-281, 1344-1345
README.md (1)
83-101: Docs align with code; consider clarifying how to opt into the internal statuses APIThe README updates do a good job of:
- Explaining the public vs internal split for status endpoints and mapping it to
services/statuses.tsp(GET, public) vsservices/statuses-internal.tsp(POST, internal).- Documenting the new OpenAPI 3.0 / Swagger output formats, npm build scripts, and direct
build-schema.shusage.- Showing manual Swagger conversion commands and listing
api-spec-converteras a dependency.One small improvement: since the table marks
statuses-internal.tspas “❌ No (opt-in)” for inclusion in the build, it would help to add a short note or example of how to enable that internal contract (e.g., via an alternate entrypoint, config flag, or separate build command) so consumers know the intended workflow.Also applies to: 138-159, 161-185, 205-209, 224-228, 292-292
schemas/gcp/swagger.yaml (1)
58-68: Consider wiring 400/404/409 responses to theErrorschema for consistencyAcross multiple endpoints, 400 (and some 404/409) responses only have a description, while the
defaultresponse correctly references#/definitions/Error. If the implementation returns the standard error payload for these statuses, it would be clearer for clients and codegen to also attach theErrorschema to those 4xx responses (mirroring howdefaultis modeled).Also applies to: 82-94, 121-131, 172-183, 203-214, 234-243, 290-299, 342-350, 387-397
schemas/core/openapi.yaml (2)
29-37: Align 400/404 responses with theErrorschema and ensure status GET has a default errorFor the cluster/nodepool list and detail endpoints, newly added 400 responses only carry a description, while
defaultcorrectly references#/components/schemas/Error. To keep the contract consistent (and friendlier to codegen), consider adding anapplication/jsoncontent block with theErrorschema to these 400 responses, and similarly wiring 404/409 where appropriate. Also,/api/hyperfleet/v1/clusters/{cluster_id}/statusesGET defines only 200/400/404, with no default error response; you may want to add adefaultresponse with the standardErrorpayload there as you do on other paths.Also applies to: 54-62, 86-94, 118-126, 144-152, 183-191, 260-273, 327-337, 349-362
363-365: Confirm that global BearerAuth and production server apply to all operationsThe new global
security: - BearerAuth: []pluscomponents.securitySchemes.BearerAuthand thehttps://hyperfleet.redhat.comserver make the core public surface authenticated and production‑targeted by default, which matches the PR summary. Please confirm that every path in this spec is meant to require Bearer auth; if some endpoints (e.g., health/readiness or public metadata) should remain unauthenticated, you’ll likely want per‑operationsecurity: []overrides for those.Also applies to: 1180-1183, 1185-1187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
README.md(5 hunks)build-schema.sh(6 hunks)main.tsp(2 hunks)package.json(1 hunks)schemas/core/openapi.yaml(13 hunks)schemas/gcp/openapi.yaml(10 hunks)schemas/gcp/swagger.yaml(1 hunks)services/clusters.tsp(3 hunks)services/compatibility.tsp(0 hunks)services/nodepools.tsp(4 hunks)services/statuses-internal.tsp(1 hunks)services/statuses.tsp(2 hunks)tspconfig.yaml(0 hunks)
💤 Files with no reviewable changes (2)
- tspconfig.yaml
- services/compatibility.tsp
🧰 Additional context used
🪛 Checkov (3.2.334)
schemas/gcp/swagger.yaml
[high] 1-1505: Ensure that security requirement defined in securityDefinitions - version 2.0 files
(CKV_OPENAPI_6)
[high] 1-1505: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-1505: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 477-484: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
[high] 1-1505: Ensure that securityDefinitions is defined and not empty - version 2.0 files
(CKV_OPENAPI_1)
🔇 Additional comments (7)
build-schema.sh (1)
93-99: Overall build flow extension to Swagger looks solidThe added output‑format messaging, explicit “OpenAPI 3.0 schema” wording, optional Swagger step, and final “Build complete!” banner make the build behavior clear without changing the default path.
Also applies to: 134-135, 168-169
package.json (1)
6-23: npm scripts and tooling wiring match the build scriptThe new build scripts (core, gcp, swagger variants, all) line up with
build-schema.sh’s CLI and should make local and CI builds straightforward, with devDependencies covering all referenced TypeSpec tooling and the converter.Please run the new scripts (
npm run build:core,npm run build:gcp, and at least one*:swagger) in your target environment to confirm there are no path or shell‑compatibility surprises.services/clusters.tsp (1)
20-22: BadRequestResponse additions are consistent with the 400 error surfaceExtending the response unions for list/get/post cluster operations to include
BadRequestResponsecleanly models 400s without altering success shapes, and lines up with the documented OpenAPI responses.Also applies to: 31-33, 47-52
main.tsp (1)
8-10: Metadata and provider wiring updates look correctImporting
./aliases.tspfor provider‑specific security plus adding the Apache 2.0 license and production server URL are consistent with the rest of the PR and keepmain.tspas a clean, provider‑agnostic entry point.Also applies to: 23-24
services/statuses.tsp (1)
23-31: Expanded error unions align with public read‑only status APIAdding
BadRequestResponseto the GET status endpoints’ unions captures invalid‑request cases without re‑introducing write operations on the public surface, which matches the new public/internal split.Also applies to: 46-55
services/nodepools.tsp (1)
22-24: BadRequestResponse union additions look consistent; just confirm 400 wiringExtending these four NodePools operations to include
BadRequestResponsealongside the success type andErrorkeeps the TypeSpec surface aligned with the new 400 responses in the generated OpenAPI/Swagger. Please just double‑check thatBadRequestResponseis defined (likely in../models/common/model.tsp) with the correct@error(400)(or equivalent) decoration so that emitters consistently map it to an HTTP 400 response body.Also applies to: 38-40, 55-57, 71-73
schemas/core/openapi.yaml (1)
7-9: License metadata addition looks goodAdding the Apache 2.0 license block under
infois correct and aligns with how other tooling expects license metadata in OpenAPI 3.
| swagger: '2.0' | ||
| info: | ||
| contact: | ||
| name: HyperFleet Team | ||
| description: >- | ||
| HyperFleet API provides simple CRUD operations for managing cluster | ||
| resources and their status history. | ||
|
|
||
|
|
||
| **Architecture**: Simple CRUD only, no business logic, no event creation. | ||
|
|
||
| Sentinel operator handles all orchestration logic. | ||
|
|
||
| Adapters handle the specifics of managing spec | ||
| license: | ||
| name: Apache 2.0 | ||
| url: 'https://www.apache.org/licenses/LICENSE-2.0' | ||
| title: HyperFleet API | ||
| version: 1.0.0 | ||
| host: hyperfleet.redhat.com | ||
| basePath: / | ||
| schemes: | ||
| - https | ||
| paths: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and examine the swagger.yaml file
fd -name "swagger.yaml" --type fRepository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 224
🌐 Web query:
Checkov CKV_OPENAPI_1 CKV_OPENAPI_4 CKV_OPENAPI_5 CKV_OPENAPI_6 security definitions
💡 Result:
Summary of those Checkov OpenAPI checks:
- CKV_OPENAPI_1 — Ensure that securityDefinitions is defined and not empty (OpenAPI v2). [1]
- CKV_OPENAPI_4 — Ensure that the global security field has rules defined (global "security" must be present/non-empty). [1][2]
- CKV_OPENAPI_5 — Ensure that security operations is not empty (operation-level "security" must be present/non-empty). [1][2]
- CKV_OPENAPI_6 — Ensure that security requirement is defined in securityDefinitions (operation/global security requirements must reference an entry from securityDefinitions for v2). [1]
Sources: Checkov OpenAPI policy index and OpenAPI scanning docs. [1][2]
References:
[1] Checkov — OpenAPI resource scans (policy index).
[2] Checkov — OpenAPI configuration scanning / examples.
🌐 Web query:
GCP API Gateway IAP authentication OpenAPI security best practices
💡 Result:
Short best-practices checklist for using IAP with GCP API Gateway + OpenAPI
-
Secure the backend with IAP (don’t rely on API Gateway alone). Enable IAP on the App Engine / Cloud Run service and use IAP to gate user access. (cloud.google.com)
-
Let API Gateway authenticate to the backend using an OIDC ID token signed by the gateway service account (configured via --backend-auth-service-account / x-google-backend). Grant only the minimal invoker/IAP role to that service account. (cloud.google.com)
-
Grant the gateway’s service account the exact IAM roles required:
- Cloud Run / Cloud Functions: roles/run.invoker or cloudfunctions.invoker.
- App Engine + IAP: IAP‑secured Web App User + the App Engine clientauth roles described in docs. Use least privilege. (cloud.google.com)
-
OpenAPI / OpenAPI-security specifics:
- Use x-google-backend and include the IAP OAuth client_id where required for App Engine examples; set backend-auth-service-account when creating the api-config so the gateway signs OIDC tokens. (cloud.google.com)
- Do NOT rely on OpenAPI-defined scopes for enforcement — API Gateway/Endpoints ignore/enforce scopes differently (scopes may be ignored). Enforce authorization at the backend (or via IAM/IAP). (docs.cloud.google.com)
-
Validate tokens and enforce authorization in your backend:
- Verify ID token audience (IAP client ID or gateway SA), issuer, expiry and signature server-side before granting access.
- Map validated identity to application authorization checks (roles/claims) rather than trusting OpenAPI doc scopes. (docs.cloud.google.com)
-
Operational security controls
- Use a dedicated gateway service account per environment, give least privilege, rotate keys/credentials, and audit access (Cloud Audit Logs).
- Use private backends or VPC connector where appropriate to reduce exposure. (cloud.google.com)
-
Tests & validation
- Test flows: unauthenticated request → IAP challenge; gateway-to-backend signed OIDC token accepted; backend rejects invalid audiences/expired tokens.
- Deploy a staging setup and verify IAM and IAP roles before production rollout. (cloud.google.com)
If you want, I can:
- produce a minimal example OpenAPI snippet (x-google-backend + backend-auth-service-account + IAP client_id) for App Engine or Cloud Run, or
- give gcloud commands to set the IAM bindings and create the api-config. Which would you prefer?
Citations:
- 1: https://cloud.google.com/api-gateway/docs/get-started-app-engine?utm_source=openai
- 2: https://cloud.google.com/api-gateway/docs/configure-dev-env?utm_source=openai
- 3: https://cloud.google.com/api-gateway/docs/securing-backend-services?utm_source=openai
- 4: https://cloud.google.com/api-gateway/docs/get-started-app-engine?utm_source=openai
- 5: https://docs.cloud.google.com/api-gateway/docs/openapi-limitations?utm_source=openai
- 6: https://docs.cloud.google.com/iap/docs/authentication-howto?utm_source=openai
- 7: https://cloud.google.com/api-gateway/docs/configure-dev-env?utm_source=openai
- 8: https://cloud.google.com/api-gateway/docs/get-started-app-engine?utm_source=openai
Based on my research, I have enough information to rewrite the review comment. The Checkov rules are legitimate, but the context of GCP API Gateway and IAP authentication is crucial. The review comment's recommendation is partially sound but needs refinement for the GCP use case.
Swagger spec lacks securityDefinitions/security — expected for GCP IAP-protected APIs, but needs documentation or Checkov override
The Swagger 2.0 document has no securityDefinitions and no global or per-operation security entries, which correctly triggers Checkov findings CKV_OPENAPI_1/4/5/6. However, this is a documented pattern for GCP APIs using API Gateway + Identity-Aware Proxy (IAP), where authentication is enforced out-of-band via x-google-backend extensions, IAM roles, and backend token validation—not via OpenAPI securityDefinitions.
Recommended actions:
- If this API is intentionally protected by GCP IAP/backend IAM: add a comment in the OpenAPI describing the authentication model (e.g., "Authentication enforced via GCP IAP and backend service account validation") and suppress or override CKV_OPENAPI_1/4/5/6 in your Checkov configuration.
- If the API should be documented with standard OpenAPI security, add
securityDefinitions(e.g., BearerAuth) plus a globalsecurityblock or per-operation overrides.
Also applies to: 342-350, 400-405, 1468-1503
🧰 Tools
🪛 Checkov (3.2.334)
[high] 1-1505: Ensure that security requirement defined in securityDefinitions - version 2.0 files
(CKV_OPENAPI_6)
[high] 1-1505: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-1505: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[high] 1-1505: Ensure that securityDefinitions is defined and not empty - version 2.0 files
(CKV_OPENAPI_1)
🤖 Prompt for AI Agents
In schemas/gcp/swagger.yaml around lines 1-24 (also apply same guidance to
ranges 342-350, 400-405, 1468-1503), the OpenAPI document lacks
securityDefinitions and security entries which triggers Checkov
CKV_OPENAPI_1/4/5/6; if the API is intentionally protected by GCP API Gateway +
IAP, add a clear top-level description/annotation stating "Authentication
enforced via GCP IAP and backend IAM; security handled out-of-band" and then
suppress or override the specific Checkov rules in your Checkov config for this
API; otherwise, add proper OpenAPI securityDefinitions (e.g., BearerAuth) and a
global security block or per-operation security objects to document
authentication and satisfy Checkov.
| @route("") | ||
| @post | ||
| @summary("Create or update adapter status") | ||
| @operationId("postClusterStatuses") | ||
| postClusterStatuses( | ||
| /** | ||
| * Cluster ID | ||
| */ | ||
| @path cluster_id: string, | ||
|
|
||
| @body body: AdapterStatusCreateRequest, | ||
| ): | ||
| | (CreatedResponse & AdapterStatus) | ||
| | BadRequestResponse | ||
| | NotFoundResponse | ||
| | ConflictResponse | ||
| | BadRequestResponse; | ||
|
|
There was a problem hiding this comment.
Remove duplicate BadRequestResponse and tighten internal status response unions
The new internal POST endpoints and their upsert semantics look good, but each union currently includes BadRequestResponse twice:
):
| (CreatedResponse & AdapterStatus)
| BadRequestResponse
| NotFoundResponse
| ConflictResponse
| BadRequestResponse;Recommend deduplicating and simplifying for both interfaces:
- ):
- | (CreatedResponse & AdapterStatus)
- | BadRequestResponse
- | NotFoundResponse
- | ConflictResponse
- | BadRequestResponse;
+ ):
+ | (CreatedResponse & AdapterStatus)
+ | BadRequestResponse
+ | NotFoundResponse
+ | ConflictResponse;Optionally, you might also add a short doc comment for nodepool_id to match the documented cluster_id param.
Also applies to: 57-75
🤖 Prompt for AI Agents
In services/statuses-internal.tsp around lines 25-42 (and similarly 57-75), the
response union for postClusterStatuses contains a duplicated BadRequestResponse
and should be tightened: remove the duplicate BadRequestResponse so the union
reads CreatedResponse & AdapterStatus | BadRequestResponse | NotFoundResponse |
ConflictResponse (or the equivalent deduplicated ordering used in the file), and
add a short doc comment for the nodepool_id parameter to mirror the cluster_id
doc comment; update both interface occurrences accordingly.
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)
schemas/core/openapi.yaml (1)
234-260: Add missing default error response togetClusterStatusesendpoint.The
getClusterStatusesendpoint (lines 234–260) defines 400 and 404 responses but lacks adefaulterror response. This is inconsistent with all other endpoints in the schema, which include adefaultresponse referencing theErrorschema. Clients expect consistent error handling across the API.Apply this diff to add the missing default response:
'404': description: The server cannot find the requested resource. + default: + description: An unexpected error response. + content: + application/json: + schema: + $ref: '#/components/schemas/Error'
♻️ Duplicate comments (2)
services/statuses-internal.tsp (1)
25-41: DeduplicateBadRequestResponsein internal status unions and optionally documentnodepool_idBoth internal POST methods currently have
BadRequestResponsetwice in the return union:): | (CreatedResponse & AdapterStatus) | BadRequestResponse | NotFoundResponse | ConflictResponse | BadRequestResponse;Recommend tightening this to a single
BadRequestResponsefor clarity and to avoid any oddities in emitted schemas:- ): - | (CreatedResponse & AdapterStatus) - | BadRequestResponse - | NotFoundResponse - | ConflictResponse - | BadRequestResponse; + ): + | (CreatedResponse & AdapterStatus) + | BadRequestResponse + | NotFoundResponse + | ConflictResponse;You might also consider adding a short doc comment for
nodepool_id(to mirrorcluster_id) for consistency.Also applies to: 57-74
schemas/gcp/swagger.yaml (1)
1-24: Swagger 2.0 spec matches the OpenAPI surface; document/authenticate strategy vs Checkov’s security findingsThe generated Swagger 2.0 spec accurately reflects the OpenAPI 3.0 surface (paths, models, 400/default errors, production host, license), so the conversion flow looks correct.
However, as Checkov reports, the document has:
- no
securityDefinitions, and- no global or per‑operation
securityentries,which will continue to trigger CKV_OPENAPI_1/4/5/6.
If authentication is intentionally enforced out‑of‑band (e.g., via GCP API Gateway + IAP and backend IAM/token validation), consider:
- adding a short note in
info.description(or a top‑level vendor extension) that explains the auth model, and- suppressing those specific Checkov rules for this spec in your Checkov configuration.
If instead you want tooling to understand and validate auth here, you’ll need to introduce an appropriate
securityDefinitionsblock (e.g., bearer token) plus a globalsecuritysection or operation‑levelsecurityentries.For CKV_OPENAPI_21 (arrays without
maxItems), you can either:
- define reasonable
maxItemsfor high‑risk list fields (e.g., top‑level*List.itemscollections), or- explicitly suppress that rule if unbounded arrays are acceptable for your use case.
Also applies to: 25-399, 400-1450, 1468-1503
🧹 Nitpick comments (3)
build-schema.sh (1)
4-7: Robust CLI parsing; consider surfacing converter errors instead of discarding stderrThe new provider/flag parsing, early validation (aliases/main.tsp,
tsppresence), and conditional swagger conversion logic look solid and match the documented usage. This should make misuse of the script much harder.For the swagger conversion step, you currently redirect
api-spec-converter’s stderr to/dev/null:npx api-spec-converter ... "${OUTPUT_DIR}/openapi.yaml" > "${OUTPUT_DIR}/swagger.yaml" 2>/dev/nullOn failure, it may be easier to debug if you either keep stderr visible or echo it only in the error path instead of fully discarding it.
Also applies to: 16-25, 31-55, 57-60, 91-99, 100-105, 124-143, 155-173, 175-176
services/statuses.tsp (1)
23-32: Asymmetric error unions between cluster and nodepool statuses—confirm this is intentional
getClusterStatusesreturnsBody<AdapterStatusList> | NotFoundResponse | BadRequestResponse, whilegetNodePoolsStatusesreturnsBody<AdapterStatusList> | Error | BadRequestResponse. That means cluster status exposes 404 explicitly, but nodepool status folds non‑400 failures into a genericError.If clients are expected to handle these two resources similarly, consider either:
- exposing
NotFoundResponseexplicitly for nodepool statuses, or- documenting that
Errorfor nodepool statuses may include 404 semantics.If the difference is deliberate (e.g., backing store behavior), it’s fine as‑is—just worth confirming.
Also applies to: 46-55
README.md (1)
83-97: Docs are in sync with tooling; consider clarifying how internal statuses build is enabledThe README changes accurately describe:
- the public vs internal split for status endpoints,
- the new npm scripts (
build:*andbuild:all),build-schema.shusage and--swagger/--openapi2flags, and- optional manual Swagger conversion via
npx api-spec-converter.One small potential improvement: in the “Public vs Internal API Split” section, you mark
statuses-internal.tspas “❌ No (opt-in)” under “Included in Build”. It might help future readers to add a short note or example showing how an internal API build is enabled (e.g., alternate entrypoint or config) so the “opt‑in” path is concrete.Also applies to: 138-159, 161-185, 205-209, 224-228, 286-293
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
README.md(5 hunks)build-schema.sh(5 hunks)main.tsp(2 hunks)package.json(1 hunks)schemas/core/openapi.yaml(10 hunks)schemas/gcp/openapi.yaml(10 hunks)schemas/gcp/swagger.yaml(1 hunks)services/clusters.tsp(3 hunks)services/compatibility.tsp(0 hunks)services/nodepools.tsp(4 hunks)services/statuses-internal.tsp(1 hunks)services/statuses.tsp(2 hunks)tspconfig.yaml(0 hunks)
💤 Files with no reviewable changes (2)
- tspconfig.yaml
- services/compatibility.tsp
🚧 Files skipped from review as they are similar to previous changes (2)
- services/clusters.tsp
- main.tsp
🧰 Additional context used
🪛 Checkov (3.2.334)
schemas/gcp/swagger.yaml
[high] 1-1505: Ensure that security requirement defined in securityDefinitions - version 2.0 files
(CKV_OPENAPI_6)
[high] 1-1505: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-1505: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 477-484: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
[high] 1-1505: Ensure that securityDefinitions is defined and not empty - version 2.0 files
(CKV_OPENAPI_1)
🔇 Additional comments (5)
package.json (1)
6-23: Scripts and devDependencies align with the build workflow; verify TypeSpec version alignmentThe npm scripts and devDependencies (TypeSpec packages +
api-spec-converter) are consistent withbuild-schema.shand the updated README. This should give a clean, reproducible build pipeline for both OpenAPI 3.0 and optional Swagger output.One thing to double‑check: ensure the globally installed
tspversion matches the@typespec/compilerversion range here to avoid subtle emitter/runtime mismatches.services/nodepools.tsp (1)
18-24: Consistent 400 modeling for NodePools endpointsAdding
BadRequestResponseto the nodepools read/create operations makes the 400 behavior explicit and lines up with the OpenAPI responses. The unions are consistent across the four methods.Just confirm that
Errordoes not itself already encompass a 400 shape; if it does, you may want to narrowErrorto 5xx/other cases to avoid ambiguous modeling for clients.Also applies to: 33-40, 49-57, 66-73
schemas/gcp/openapi.yaml (1)
7-9: OpenAPI 3 metadata and 400 responses look consistent with the TypeSpec changesAdding the Apache 2.0 license and updating the server URL to the production host look good and align with the new Swagger spec.
The newly documented 400 responses on the core list/get/create and nodepool/statuses endpoints correctly mirror the
BadRequestResponseadditions in the TypeSpec interfaces, so clients now have explicit 400 semantics in both OpenAPI 3.0 and Swagger.Also applies to: 35-37, 60-61, 92-93, 124-125, 150-151, 189-190, 226-227, 258-259, 280-281, 1344-1345
schemas/core/openapi.yaml (2)
7-9: License metadata added appropriately.The Apache 2.0 license information is now properly documented in the API specification, improving API discoverability and compliance clarity.
1104-1105: Server URL updated to production.The server URL has been correctly updated to the production endpoint (
https://hyperfleet.redhat.com). This change aligns with the API's maturity and deployment readiness.
| '400': | ||
| description: The server could not understand the request due to invalid syntax. |
There was a problem hiding this comment.
Add error schemas to 400 Bad Request responses.
All 400 error responses currently lack response schemas or content definitions. They only include descriptions. For consistency and API clarity, each 400 response should include the error response body schema (similar to the default responses that reference #/components/schemas/Error).
Apply this diff to add schemas to the 400 responses:
'400':
description: The server could not understand the request due to invalid syntax.
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/Error'Also applies to: 60-61, 92-93, 124-125, 150-151, 189-190, 226-227, 280-281
🤖 Prompt for AI Agents
In schemas/core/openapi.yaml around lines 35-36 (and also apply same change at
60-61, 92-93, 124-125, 150-151, 189-190, 226-227, 280-281), the 400 responses
only include a description and must be updated to include a response body
schema; replace each bare 400 description with a 400 response that adds a
content entry (application/json) referencing the existing Error schema
(#/components/schemas/Error) so the 400 responses mirror the default error
response structure.
| * | ||
| */ | ||
| @route("") | ||
| @post |
There was a problem hiding this comment.
Was it deleted by mistake? So is post nodepoolStatus.
There was a problem hiding this comment.
I moved the post operations to a statuses-internal.tsp so we don't expose POST operations externally in the GCP contract since these should be internal to our system
There was a problem hiding this comment.
For the specific user GCP, we may already do this for them, so they might not need this part, and we can choose not to expose it to them. However, I believe this modification should be reflected in gcp_openapi.yaml, not our general core openapi.yaml. I think that all our components can be provided to customers, meaning they will use POST requests when customizing their adapters. We should not hide this in the generic openapi.
There was a problem hiding this comment.
You caught an error that may have slipped when rebasing.
The statuses-internal.tsp should have been imported in the aliases-core.tsp, so it is generated for our internal components as you describe.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
services/statuses-internal.tsp (2)
25-42: Remove duplicate BadRequestResponse from the return type union.The return type union contains
BadRequestResponsetwice (lines 38 and 41). This should be deduplicated.Apply this diff:
): | (CreatedResponse & AdapterStatus) | BadRequestResponse | NotFoundResponse | ConflictResponse - | BadRequestResponse; + ;
57-75: Remove duplicate BadRequestResponse and add nodepool_id documentation.Two issues:
BadRequestResponseis duplicated in the return type union (lines 71 and 74)- The
nodepool_idparameter lacks a doc comment whilecluster_idhas one (line 63-64)Apply this diff:
/** * Cluster ID */ @path cluster_id: string, + /** + * NodePool ID + */ @path nodepool_id: string, @body body: AdapterStatusCreateRequest, ): | (CreatedResponse & AdapterStatus) | BadRequestResponse | NotFoundResponse | ConflictResponse - | BadRequestResponse; + ;
🧹 Nitpick comments (2)
services/statuses.tsp (1)
53-55: Consider using NotFoundResponse instead of Error for consistency.The
getNodePoolsStatusesoperation usesErrorin its return type union, whilegetClusterStatuses(lines 29-31) usesNotFoundResponse. Since both operations accept path parameters (cluster_idandnodepool_id) and can return 404 when resources aren't found, usingNotFoundResponsewould be more consistent and semantically clearer.Apply this diff for consistency:
- ): Body<AdapterStatusList> - | Error - | BadRequestResponse; + ): Body<AdapterStatusList> + | NotFoundResponse + | BadRequestResponse;schemas/gcp/openapi.yaml (1)
35-36: Add error response schemas to 400 Bad Request responses.All the 400 Bad Request responses currently only include a description without a response body schema. For consistency with the
defaulterror responses and to improve API client generation, consider adding the error schema reference to each 400 response.Example for one endpoint:
'400': description: The server could not understand the request due to invalid syntax. + content: + application/json: + schema: + $ref: '#/components/schemas/Error'This pattern should be applied to all 400 responses throughout the file.
Note: This same recommendation applies to
schemas/core/openapi.yamland was mentioned in past review comments.Also applies to: 60-61, 92-93, 124-125, 150-151, 189-190, 226-227, 280-281
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
README.md(5 hunks)build-schema.sh(5 hunks)main.tsp(2 hunks)package.json(1 hunks)schemas/core/openapi.yaml(10 hunks)schemas/gcp/openapi.yaml(10 hunks)schemas/gcp/swagger.yaml(1 hunks)services/clusters.tsp(3 hunks)services/compatibility.tsp(0 hunks)services/nodepools.tsp(4 hunks)services/statuses-internal.tsp(1 hunks)services/statuses.tsp(2 hunks)tspconfig.yaml(0 hunks)
💤 Files with no reviewable changes (2)
- services/compatibility.tsp
- tspconfig.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- services/nodepools.tsp
- services/clusters.tsp
🧰 Additional context used
🪛 Checkov (3.2.334)
schemas/gcp/swagger.yaml
[high] 1-1527: Ensure that security requirement defined in securityDefinitions - version 2.0 files
(CKV_OPENAPI_6)
[high] 1-1527: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-1527: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 477-484: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
[high] 1-1527: Ensure that securityDefinitions is defined and not empty - version 2.0 files
(CKV_OPENAPI_1)
🔇 Additional comments (14)
main.tsp (3)
8-9: LGTM! Import restructuring aligns with compatibility endpoint removal.The import change from
compatibility.tsptoaliases.tspis consistent with the removal of the compatibility API surface described in the PR objectives. The comment provides helpful context about provider-specific security configuration.
23-23: LGTM! License metadata added correctly.The Apache 2.0 license information has been properly added to the service metadata with the correct license URL.
24-24: Verify production server URL change impacts all generated schemas.The server URL has been changed from
http://localhost:8000(Development) tohttps://hyperfleet.redhat.com(Production). Since this is inmain.tsp, this change affects all generated OpenAPI schemas (core, GCP, etc.).Ensure that:
- The production URL
https://hyperfleet.redhat.comis correct and operational- Developers are aware they'll need to override this URL for local development
- This aligns with your deployment strategy across all providers
build-schema.sh (4)
16-55: LGTM! Argument parsing is well-structured with good error handling.The argument parsing logic correctly:
- Enforces provider as the first required argument
- Validates that provider doesn't start with
-(preventing common mistakes)- Supports both
--swaggerand--openapi2flag aliases- Rejects unknown options with clear error messages
91-98: LGTM! Pre-flight check prevents late failures.The conditional pre-flight check for
api-spec-converteravailability is well-designed:
- Only runs when swagger generation is requested
- Fails early before the TypeSpec compilation step
- Provides clear installation instructions
100-106: LGTM! Clear messaging about output formats.The conditional output format messaging helps users understand what will be generated before the build starts.
155-173: LGTM! Swagger conversion step is well-implemented.The OpenAPI 2.0 conversion step includes:
- Correct conversion parameters
- Helpful error messaging explaining that some OpenAPI 3.0 features aren't supported in 2.0
- Cleanup of partial output on failure
- Appropriate error handling
services/statuses.tsp (1)
29-31: LGTM! BadRequestResponse added for query parameter validation.The addition of
BadRequestResponseto the return type union allows the API to properly handle invalid query parameters (pagination, search, etc.) while maintaining the existingNotFoundResponsefor when the cluster is not found.schemas/gcp/openapi.yaml (1)
7-9: LGTM! License metadata added consistently.The Apache 2.0 license information matches the license added to
main.tspand follows the standard format.schemas/gcp/swagger.yaml (1)
1-24: LGTM! Swagger 2.0 metadata consistent with OpenAPI 3.0 version.The license, server URL, and API description match the OpenAPI 3.0 specification. The absence of
securityDefinitionstriggering Checkov warnings (CKV_OPENAPI_1/4/5/6) is expected if authentication is handled via GCP Identity-Aware Proxy (IAP) as noted in past review comments.Note: If security is indeed handled by GCP IAP, consider adding a comment in the spec description explaining the authentication model to suppress static analysis warnings and clarify the security approach for API consumers.
schemas/core/openapi.yaml (1)
7-9: LGTM! License metadata added consistently.The Apache 2.0 license information is consistent across all OpenAPI schema files.
README.md (3)
100-117: Excellent documentation of the public/internal API split.The added section clearly explains:
- Which operations are in which file
- The intended audience for each API surface
- The rationale for the split
The table format makes it easy to understand the differences at a glance.
154-245: LGTM! Comprehensive build documentation with clear examples.The updated build instructions:
- Clearly explain both OpenAPI 3.0 and 2.0 output formats
- Provide three levels of usage (npm scripts, direct script, manual)
- Include helpful examples for each approach
- Document the conversion step for Swagger generation
The progressive structure (recommended → direct → manual) helps users choose the right approach for their needs.
308-308: LGTM! Dependency documentation updated.The
api-spec-converterdependency is properly documented, consistent with its usage in the build process for generating OpenAPI 2.0 (Swagger) output.
|
/lgtm |
2 participants
This PR is to convert from OpenAPI v3 to Swagger for GCP
I addressed also some issues when validating the schemas with the openapi tool (https://github.com/speakeasy-api/openapi)
build-schema.shto support additional command-line options for generating OpenAPI 2.0 (Swagger)README.mdto reflect new build options and output formats.BadRequestResponsehandling to various service interfaces for improved error management.statuses-internal.tspfor internal status write endpoints, separating public and internal API functionalities.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.