Repository navigation
chore: add CI check for openapi spec generation - #751
Pedro S. Lopez (pedroslopez) wants to merge 3 commits into
Conversation
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@pedro/check-openapi#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch pedro/check-openapiHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds a CI check to automatically validate that the OpenAPI specification is up-to-date whenever API models are modified. The goal is to prevent developers from forgetting to regenerate the OpenAPI spec after making changes to request/response types.
- Adds a new GitHub Actions job that generates the OpenAPI spec and fails if it differs from the committed version
- Updates the README with documentation about the automated validation process
- Includes updates to the OpenAPI spec itself (likely from running the generation command)
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/workflows/python_lint.yml |
Adds new openapi-check job to validate OpenAPI spec is current |
airbyte_cdk/manifest_server/README.md |
Documents the automated OpenAPI validation process and manual regeneration command |
airbyte_cdk/manifest_server/openapi.yaml |
Updates OpenAPI spec with new RequestContext schema and context fields |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
📝 WalkthroughWalkthroughAdds a CI job to verify OpenAPI spec generation in python_lint workflow. Updates README with instructions about automated OpenAPI validation. Extends OpenAPI spec by introducing RequestContext and adding optional context fields to multiple request schemas, and broadens slice_descriptor type to allow objects. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant GH as GitHub Actions
participant Job as openapi-check
Dev->>GH: Open PR / push
GH->>Job: Run workflow
Job->>Job: Checkout repo
Job->>Job: Setup Poetry 2.0.1 & Python 3.10
Job->>Job: Install deps (poetry install)
Job->>Job: Generate OpenAPI (poetry run manifest-server generate-openapi)
Job->>Job: git diff for changes
alt Spec changed
Job-->>GH: Mark job failed (spec outdated)
else No changes
Job-->>GH: Job succeeds
end
sequenceDiagram
participant Client
participant API as Manifest Server API
participant Handler as Request Handler
Client->>API: Request (optional context)
API->>Handler: Parse payload incl. context
Handler-->>API: Proceed with operation using context (if provided)
API-->>Client: Response
note over API,Handler: Context fields are optional (workspace_id, project_id)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
♻️ Duplicate comments (3)
.github/workflows/python_lint.yml (1)
2-3: Good security posture: minimal GITHUB_TOKEN perms now declared.This addresses the prior security bot suggestion to limit default permissions. 👍
airbyte_cdk/manifest_server/openapi.yaml (2)
337-341: Repeat of thecontextaddition—same doc suggestion applies.Mirroring the note above: add a brief description in the source model so clients see why/when to use
context, wdyt?
491-495: Samecontextfield here—carry over the docstring suggestion.Adding a short description in the model will surface better help in generated clients, wdyt?
🧹 Nitpick comments (5)
airbyte_cdk/manifest_server/README.md (1)
128-139: Nice addition: call out the CI guardrail; consider linking the job name and mentioning the existing poe task.Could we add the actual job name (“OpenAPI Spec Check”) and a brief note that there’s also a poe task to regenerate the spec, so contributors can choose either command, wdyt?
.github/workflows/python_lint.yml (1)
82-124: Make the diff detection robust to untracked files and scope it to openapi.yaml.If the spec file ever becomes untracked,
git diff --quietwon’t catch it. Would you switch togit status --porcelainscoped to the spec, and set the output accordingly, wdyt?- name: Check for changes id: git-diff - run: | - git diff --quiet && echo "No changes to commit" || echo "changes=true" >> $GITHUB_OUTPUT - shell: bash + shell: bash + run: | + set -eo pipefail + TARGET="airbyte_cdk/manifest_server/openapi.yaml" + git update-index -q --refresh + if git status --porcelain -- "$TARGET" | grep -q .; then + printf "changes=true\n" >> "$GITHUB_OUTPUT" + else + echo "No changes to OpenAPI spec" + fiOptional follow-ups:
- Would you prefer installing only what’s needed for generation to speed up CI (e.g.,
poetry install --extras manifest-server) instead of--all-extras, wdyt?- Poetry versions differ across jobs (this one uses 2.0.1 while others use 1.8.4). Standardizing across the workflow can reduce surprises; want to align them in a follow-up?
airbyte_cdk/manifest_server/openapi.yaml (3)
299-303: Additive context field looks good; consider documenting its intent.To improve generated docs/SDKs, would you add a short description to the
contextproperty in the source model (so it flows into the spec), e.g., “Optional request context for tracing/observability,” wdyt?
472-486: Tighten typing for IDs and enrich field docs.If
workspace_id/project_idare UUIDs, would you addformat: uuid(via PydanticField(..., json_schema_extra={"format": "uuid"})) and brief descriptions so SDKs validate/help users, wdyt? Example in the source model:from pydantic import BaseModel, Field class RequestContext(BaseModel): workspace_id: str | None = Field( default=None, description="Workspace identifier associated with the request.", json_schema_extra={"format": "uuid"}, ) project_id: str | None = Field( default=None, description="Project identifier associated with the request.", json_schema_extra={"format": "uuid"}, )
575-579: Broadenedslice_descriptor= object|string|null—can we define the object shape?For stronger client generation, would you consider modeling the object form (e.g., with a dedicated schema or
oneOfvariants) instead of baretype: object, or at least documenting expected keys in the source model so it flows into the spec, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/python_lint.yml(2 hunks)airbyte_cdk/manifest_server/README.md(1 hunks)airbyte_cdk/manifest_server/openapi.yaml(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-shopify
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-intercom
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: SDM Docker Image Build
🔇 Additional comments (2)
airbyte_cdk/manifest_server/openapi.yaml (2)
368-372: Context on full resolve—ensure server plumbs it through.Since this feeds dynamic generation, can we confirm the backend handlers accept and propagate
contextwhere needed (logging/metrics/tracing), wdyt?
636-640:contexton test_read—ensure it’s utilized or ignored predictably.Can we confirm test-read handlers either use this for tracing or explicitly ignore it without error, and maybe document that behavior, wdyt?
…nt carried, and the stages Ruling 1: components from the TypedDicts, the route join from the narrowed resolver, the spec committed and CI red on drift. Ruling 2: openapi-typescript as a frontend dev dependency. The prior art cited for ruling 1 could not be found and is replaced by airbytehq/airbyte-python-cdk#751; the body records that a supplied example was wrong, the same class as the entries wrong about their own subject. msgspec was tried on every TypedDict in a throwaway environment. As shipped it refuses the project's boolean Literal tags on 35 of 133 and any union of two TypedDicts, and on 3.14 under deferred annotations it would read every NotRequired field as required because __optional_keys__ is empty there. With a rebuild pass from get_type_hints all 133 emit in 30 ms, every optional field correct against the AST, and openapi-typescript consumes the result. Seven of the components are dataclasses reached through two internal session TypedDicts that never go on the wire, so the emitter must take the resolver's closure, never the inventory - a rule stage C's script has to carry in its own text. No TypedDict count is carried any more: 117 was stale three ways and the AST census misses the inheriting ones. The count is derived at emission. Stages A-E are listed by what each proves; A closes 4b. Claude-Session: https://claude.ai/code/session_019ptzxdRZkLsVAFJptStv9V
… on drift packages/truestill-app/openapi.json is committed. scripts/emit_openapi.py --write regenerates it; --check (the default) exits 1 on drift; and test_the_committed_spec_is_current.py asserts the same in make check, in Python, with no Node - the airbyte pattern (airbytehq/airbyte-python-cdk#751). The emitter is deterministic: sorted keys at dump time, nothing dated, no path outside the routes', two renders compared. The derivations moved out of the resolver test into scripts/payload_contract.py, because a script importing a test is backwards; both test files and the two living documents that cited the old home now point at it. Each arm carries its status code and the method that reaches it. Status: `status_code=` on the response call, both arms of an `a if c else b`. Method: three lexical facts about the six two-method handlers, which all branch on `request.method == "POST"` - a return inside the block belongs to that method; one outside belongs to the other methods, and to that method too when the block falls through or the return sits in an except that wraps it. Not branch narrowing of a value. Q1268, derived and pinned: no union in the document carries a string tag, so no OpenAPI discriminator is emitted anywhere. Thirteen two-member unions carry a boolean tag (`ok`, `valid`, `armed`, `created`), which TypeScript narrows on without a discriminator. Ten unions have no shared property at all - every job envelope among them, because JobStarted is {"job_id"} and nothing else. Those are bare oneOfs, and openapi-typescript renders them as a union of the exact shapes: narrowable with `in`, and strictly more than the cast at main.tsx:37. Giving JobStarted a tag would change a payload and is a separate decision. Q1271: every arm of a status is one oneOf, never anyOf, and the test loops the derived arms asserting each into the document under its status and method - 111 arms, 38 union operations on 200. The expired-session refusal is a 409 arm, the bake's missing word a 400, the catalog-busy refusal a 503 on every operation - none of them a collision on 200 any more. Q1270: /api/jobs/{job_id}/events is text/event-stream with the four frames as one oneOf under schema; openapi-typescript 7.13.0 read the committed spec into 3,793 lines in 147 ms, with that route as a union of the four frame types. Q1273: DoneFrame.summary is the oneOf of the sixteen summary types, derived from the factories, and the document has no empty schema left. The two contract test files emit once per module: the first gate ran the lane at 48 s against the 45 s ceiling because every test re-emitted the document, and the ceiling is the durable number. Proved by mutation, control green first: an arm dropped from a multi-type route, the summary union not filled, oneOf turned to anyOf, the event stream's content type lost, and status codes collapsed to 200 each turn the drift test red. Stage E does not start here. Claude-Session: https://claude.ai/code/session_019ptzxdRZkLsVAFJptStv9V
I forget to update the openapi spec since it's a manual thing.
This adds a CI check to make sure we don't forget. There's already a poe task to regenerate so you should be able to run the command if you forget - i prefer that over autocommitting fixes
Summary by CodeRabbit
New Features
Documentation
Chores