-
Notifications
You must be signed in to change notification settings - Fork 30
chore: regenerate manifest-server openapi spec #750
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
👋 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/openapi-gen#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/openapi-gen Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
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.
Pull Request Overview
This PR regenerates the OpenAPI specification for the manifest server, adding context support and refining type definitions.
- Adds optional
RequestContext
schema with workspace and project IDs for tracing and observability - Integrates context field into various request schemas (CheckRequest, DiscoverRequest, ReadRequest, ResolveRequest, SlicesRequest)
- Refines slice_descriptor type definition to include object type alongside existing string and null types
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
📝 WalkthroughWalkthroughIntroduces a new RequestContext schema and adds an optional context field to several request schemas. Broadens StreamReadSlices.slice_descriptor to also accept objects. Updates the Slice Limit section to include the optional context. All changes are in the OpenAPI spec file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant MS as Manifest Server
participant W as Connector/Worker
rect rgba(230,245,255,0.6)
note over C,MS: Requests with optional context
C->>MS: Check/Discover/Resolve/FullResolve/StreamTestRead (context?)
alt context provided
MS->>W: Forward request + context
else no context
MS->>W: Forward request (no context)
end
W-->>MS: Response
MS-->>C: Response
end
sequenceDiagram
autonumber
participant C as Client
participant MS as Manifest Server
rect rgba(240,235,255,0.6)
note over C,MS: Stream read slices input
C->>MS: StreamReadSlices(slice_descriptor: string|object|null, context?)
MS-->>C: Slices or error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Would you like to add brief examples in the spec showing valid ✨ 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 (
|
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 (7)
airbyte_cdk/manifest_server/openapi.yaml (7)
299-303
: Add a short description/example to the new context field (via the generator), wdyt?Since this file is generated, could we tweak the generator to document the new field for clients? A tiny description + example would clarify usage.
context: anyOf: - $ref: '#/components/schemas/RequestContext' - type: 'null' + description: Optional request-scoped metadata for tracing/observability (not used for auth). + example: + workspace_id: "00000000-0000-0000-0000-000000000000" + project_id: null
337-341
: Mirror the context doc/example here too for consistency, wdyt?Keeping property docs consistent across requests will help SDK consumers.
context: anyOf: - $ref: '#/components/schemas/RequestContext' - type: 'null' + description: Optional request-scoped metadata for tracing/observability (not used for auth). + example: + workspace_id: "00000000-0000-0000-0000-000000000000" + project_id: null
368-372
: Document the new context on FullResolveRequest as well, wdyt?Same rationale as other requests; helps generated SDKs and API readers.
context: anyOf: - $ref: '#/components/schemas/RequestContext' - type: 'null' + description: Optional request-scoped metadata for tracing/observability (not used for auth). + example: + workspace_id: "00000000-0000-0000-0000-000000000000" + project_id: null
472-486
: Strengthen RequestContext typing with UUID format and examples, wdyt?If these are UUIDs in practice, adding
format: uuid
+ examples will improve validation and codegen types. If they are not always UUIDs, ignore.RequestContext: properties: workspace_id: anyOf: - - type: string + - type: string + format: uuid - type: 'null' title: Workspace Id + example: "00000000-0000-0000-0000-000000000000" project_id: anyOf: - - type: string + - type: string + format: uuid - type: 'null' title: Project Id + example: "11111111-1111-1111-1111-111111111111" type: object title: RequestContext description: Optional context information for tracing and observability.Could both IDs appear simultaneously, or should this be mutually exclusive via
oneOf
? If mutually exclusive, I can propose the schema tweak, wdyt?
491-495
: Add matching context doc/example on ResolveRequest, wdyt?For parity with other requests.
context: anyOf: - $ref: '#/components/schemas/RequestContext' - type: 'null' + description: Optional request-scoped metadata for tracing/observability (not used for auth). + example: + workspace_id: "00000000-0000-0000-0000-000000000000" + project_id: null
575-579
: Widening slice_descriptor to object may break strict clients; confirm downstreams and consider a named schema + description, wdyt?Clients previously expecting a string may now need to handle objects. Can we confirm consumer readiness and document the object shape (even if open)? Defining a component schema also improves reuse.
slice_descriptor: - anyOf: - - type: object - - type: string - - type: 'null' - title: Slice Descriptor + anyOf: + - $ref: '#/components/schemas/SliceDescriptor' + - type: 'null' + title: Slice Descriptor + description: Identifier for a slice. Historically a string; now may be an object for richer identifiers. + + SliceDescriptor: + anyOf: + - type: string + - type: object + title: SliceDescriptor + description: String or free-form object describing the slice. Object shape is implementation-defined.If introducing a new component isn’t desirable in the generator, at least adding the
description
on the property would help, wdyt?
636-640
: Add context doc/example to StreamTestReadRequest for clarity, wdyt?This request is often used manually; having an inline example helps.
context: anyOf: - $ref: '#/components/schemas/RequestContext' - type: 'null' + description: Optional request-scoped metadata for tracing/observability (not used for auth). + example: + workspace_id: "00000000-0000-0000-0000-000000000000" + project_id: null
📜 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 (1)
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). (5)
- GitHub Check: Check: source-shopify
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
PyTest Results (Full)3 745 tests 3 733 ✅ 11m 4s ⏱️ Results for commit d219d9f. |
Missed this when doing the last couple updates to manifest-server, so the platform doesn't know about new available options
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.