feat: Add support for publishing custom connector definitions #816
Conversation
- Add api_util functions for custom YAML/Docker source/destination definition CRUD operations - Add CloudWorkspace methods for publishing all 3 definition types with validation - Add 9 MCP tools (publish/list/update for YAML sources, Docker sources, Docker destinations) - Add integration tests for all definition types - Add client-side manifest validation for YAML with pre_validate option Supports all 3 custom definition types from Airbyte 1.6: - custom_yaml_source_definition (YAML manifests, no Docker build needed) - custom_docker_source_definition (custom Docker source images) - custom_docker_destination_definition (custom Docker destination images) Uses airbyte-api 0.53.0 SDK with declarative_source_definitions, source_definitions, destination_definitions Relates to Airbyte 1.6 release: https://docs.airbyte.com/release_notes/v-1.6 API docs: https://reference.airbyte.com/reference/createdeclarativesourcedefinition Requested by: @aaronsteers Devin session: https://app.devin.ai/sessions/7733e25275f44008ab6cb765d4ef5106 Co-Authored-By: AJ Steers <[email protected]>
Original prompt from AJ Steers |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1759443655-custom-connector-definitions' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1759443655-custom-connector-definitions'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
📝 WalkthroughWalkthroughAdds client-side YAML manifest validation and CRUD helpers for custom YAML and Docker connector definitions; extends CloudWorkspace, MCP cloud_ops handlers, and connector models to publish/list/get/update/rename/permanently-delete custom sources and destinations; adds integration tests for lifecycle and validation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CW as CloudWorkspace
participant AU as api_util
participant API as Airbyte API
rect rgb(245,248,255)
note right of User: Publish custom YAML source (optional pre-validate)
User->>CW: publish_custom_source_definition(name, manifest_yaml, pre_validate?)
alt pre_validate
CW->>AU: validate_yaml_manifest(manifest)
AU-->>CW: (is_valid, err_msg)
alt invalid
CW-->>User: raise PyAirbyteInputError
end
end
CW->>AU: create_custom_yaml_source_definition(..., manifest, workspace_id, creds)
AU->>API: POST /declarative_source_definitions
API-->>AU: DeclarativeSourceDefinitionResponse
AU-->>CW: response
CW-->>User: CloudCustomSourceDefinition (id, name, manifest, version)
end
rect rgb(245,255,245)
note right of User: Publish custom Docker destination
User->>CW: publish_custom_destination_definition(name, docker_image, docker_tag, documentation_url)
CW->>AU: create_custom_docker_destination_definition(..., docker_repository, docker_image_tag, workspace_id, creds)
AU->>API: POST /destination_definitions
API-->>AU: DefinitionResponse
AU-->>CW: response
CW-->>User: CloudCustomDestinationDefinition (id, name, repo, tag)
end
rect rgb(255,248,245)
note right of User: Permanently delete custom definition
User->>CW: permanently_delete_custom_source_definition(definition_id)
CW->>AU: get_custom_*_definition(..., definition_id)
AU->>API: GET /.../{definition_id}
API-->>AU: Response (indicates yaml/docker)
AU-->>CW: response
CW->>AU: delete_custom_*_definition(..., definition_id)
AU->>API: DELETE /.../{definition_id}
API-->>AU: 204
AU-->>CW: success
CW-->>User: None
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Would you like me to flag any of these as blockers to review first? wdyt? Suggested labels
Suggested reviewers
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (6)
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. 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: 0
🧹 Nitpick comments (3)
airbyte/mcp/cloud_ops.py (1)
552-555: Verify: Should list functions return raw data or formatted strings?This function returns
list[dict[str, Any]]directly, while publish/update functions return formatted strings. Looking at other list functions in this file (e.g.,list_deployed_cloud_source_connectorsat line 406), they also return raw data. So this is consistent with the existing pattern.However, it creates an inconsistency in return types within the custom definitions API surface. For MCP tools, should we consider wrapping these in try/except blocks and returning a formatted response (either the list as JSON string on success, or an error message string on failure)? This would make the tool interface more consistent. Wdyt?
airbyte/cloud/workspaces.py (1)
467-472: Verify: Should yaml.safe_load() errors be caught and wrapped?The code uses
yaml.safe_load()to parse manifests from Path or str, but doesn't explicitly handle YAML parsing errors. If a user provides malformed YAML, they'll get a genericyaml.YAMLErrorexception.Should we catch and wrap these in a
PyAirbyteInputErrorfor consistency with the rest of the API? This would provide a more user-friendly error message. Wdyt?Example refactor:
if isinstance(manifest, Path): - manifest_dict = yaml.safe_load(manifest.read_text()) + try: + manifest_dict = yaml.safe_load(manifest.read_text()) + except yaml.YAMLError as e: + raise exc.PyAirbyteInputError( + message="Failed to parse YAML manifest from file", + context={"path": str(manifest), "error": str(e)}, + ) from e elif isinstance(manifest, str): - manifest_dict = yaml.safe_load(manifest) + try: + manifest_dict = yaml.safe_load(manifest) + except yaml.YAMLError as e: + raise exc.PyAirbyteInputError( + message="Failed to parse YAML manifest from string", + context={"error": str(e)}, + ) from eairbyte/_util/api_util.py (1)
940-976: Consider: Could validation be more comprehensive?The current validation checks the bare minimum: dict type, presence of "version" and "type" fields, and type="DeclarativeSource". This is a good start for catching obvious errors early.
However, should we consider additional checks? For example:
- Version format (e.g., semantic version pattern)
- Presence of "streams" field
- Basic structure validation for streams
The trade-off is complexity vs. error-catching. The API will ultimately validate the full manifest, so maybe this minimal approach is fine for now. Wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte/_util/api_util.py(2 hunks)airbyte/cloud/workspaces.py(3 hunks)airbyte/mcp/cloud_ops.py(2 hunks)tests/integration_tests/cloud/test_custom_definitions.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/integration_tests/cloud/test_custom_definitions.py (4)
airbyte/cloud/workspaces.py (16)
CloudWorkspace(60-841)publish_custom_yaml_source(457-499)list_custom_yaml_sources(501-526)get_custom_yaml_source(528-545)update_custom_yaml_source(547-580)delete_custom_yaml_source(582-593)publish_custom_docker_source(595-630)list_custom_docker_sources(632-658)get_custom_docker_source(660-678)update_custom_docker_source(680-704)delete_custom_docker_source(706-717)publish_custom_docker_destination(719-754)list_custom_docker_destinations(756-782)get_custom_docker_destination(784-802)update_custom_docker_destination(804-828)delete_custom_docker_destination(830-841)tests/integration_tests/cloud/conftest.py (1)
cloud_workspace(91-102)airbyte/_util/text_util.py (1)
generate_random_suffix(14-22)airbyte/exceptions.py (1)
PyAirbyteInputError(201-210)
airbyte/mcp/cloud_ops.py (2)
airbyte/cloud/workspaces.py (9)
publish_custom_yaml_source(457-499)list_custom_yaml_sources(501-526)update_custom_yaml_source(547-580)publish_custom_docker_source(595-630)list_custom_docker_sources(632-658)update_custom_docker_source(680-704)publish_custom_docker_destination(719-754)list_custom_docker_destinations(756-782)update_custom_docker_destination(804-828)airbyte/_util/api_util.py (6)
list_custom_yaml_source_definitions(1014-1039)update_custom_yaml_source_definition(1072-1104)list_custom_docker_source_definitions(1166-1189)update_custom_docker_source_definition(1220-1252)list_custom_docker_destination_definitions(1314-1337)update_custom_docker_destination_definition(1368-1400)
airbyte/cloud/workspaces.py (3)
airbyte/_util/api_util.py (16)
validate_yaml_manifest(940-976)create_custom_yaml_source_definition(979-1011)list_custom_yaml_source_definitions(1014-1039)get_custom_yaml_source_definition(1042-1069)update_custom_yaml_source_definition(1072-1104)delete_custom_yaml_source_definition(1107-1126)create_custom_docker_source_definition(1129-1163)list_custom_docker_source_definitions(1166-1189)get_custom_docker_source_definition(1192-1217)update_custom_docker_source_definition(1220-1252)delete_custom_docker_source_definition(1255-1274)create_custom_docker_destination_definition(1277-1311)list_custom_docker_destination_definitions(1314-1337)get_custom_docker_destination_definition(1340-1365)update_custom_docker_destination_definition(1368-1400)delete_custom_docker_destination_definition(1403-1422)airbyte/exceptions.py (1)
AirbyteDuplicateResourcesError(513-517)airbyte/mcp/cloud_ops.py (6)
list_custom_yaml_source_definitions(552-555)update_custom_yaml_source_definition(558-592)list_custom_docker_source_definitions(643-646)update_custom_docker_source_definition(649-677)list_custom_docker_destination_definitions(728-731)update_custom_docker_destination_definition(734-762)
airbyte/_util/api_util.py (2)
airbyte/exceptions.py (2)
PyAirbyteInputError(201-210)AirbyteError(432-447)airbyte/secrets/base.py (1)
SecretString(38-143)
⏰ 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). (6)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (2)
tests/integration_tests/cloud/test_custom_definitions.py (1)
167-184: LGTM! Error validation test looks good.The test correctly verifies that an invalid manifest (missing the required
typefield) raises aPyAirbyteInputErrorand that the error message mentions "type". This aligns with the validation logic inapi_util.validate_yaml_manifest().airbyte/mcp/cloud_ops.py (1)
781-789: LGTM! Tool registration is complete.All 9 new custom definition functions are properly registered with the FastMCP app, following the established pattern in this file.
PyTest Results (Fast Tests Only, No Creds)304 tests ±0 304 ✅ ±0 4m 22s ⏱️ -3s Results for commit 7572ff8. ± Comparison against base commit a6b513e. This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
PyTest Results (Full)372 tests +5 354 ✅ +3 19m 56s ⏱️ -17s Results for commit 7572ff8. ± Comparison against base commit a6b513e. This pull request removes 2 and adds 7 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
- Replace 15 CloudWorkspace methods with 10 consolidated methods - Add CloudCustomSourceDefinition and CloudCustomDestinationDefinition dataclasses - Implement lazy-loading pattern for efficient data retrieval - Replace 9 MCP tools with 6 consolidated tools with shortened names - Update integration tests to use new dataclass returns - Fix all parameter passing to use api_root, client_id, client_secret All methods now return proper dataclasses following the lazy-loading pattern from CloudSource/CloudConnection. Public API is consolidated to accept either manifest_yaml or docker_image parameters. Co-Authored-By: AJ Steers <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte/cloud/connectors.py(2 hunks)airbyte/cloud/workspaces.py(3 hunks)airbyte/mcp/cloud_ops.py(2 hunks)tests/integration_tests/cloud/test_custom_definitions.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration_tests/cloud/test_custom_definitions.py
🧰 Additional context used
🧬 Code graph analysis (3)
airbyte/mcp/cloud_ops.py (2)
airbyte/cloud/workspaces.py (6)
publish_custom_source_definition(462-562)list_custom_source_definitions(564-607)update_custom_source_definition(643-711)publish_custom_destination_definition(741-782)list_custom_destination_definitions(784-808)update_custom_destination_definition(831-857)airbyte/cloud/connectors.py (11)
name(110-118)name(299-303)name(427-431)documentation_url(342-348)documentation_url(448-452)version(315-321)docker_repository(324-330)docker_repository(434-438)docker_image_tag(333-339)docker_image_tag(441-445)manifest(306-312)
airbyte/cloud/connectors.py (2)
airbyte/cloud/workspaces.py (4)
CloudWorkspace(65-874)workspace_url(83-85)permanently_delete_custom_source_definition(713-739)permanently_delete_custom_destination_definition(859-874)airbyte/_util/api_util.py (3)
get_custom_yaml_source_definition(1042-1069)get_custom_docker_source_definition(1192-1217)get_custom_docker_destination_definition(1340-1365)
airbyte/cloud/workspaces.py (3)
airbyte/_util/api_util.py (15)
validate_yaml_manifest(940-976)create_custom_docker_source_definition(1129-1163)list_custom_yaml_source_definitions(1014-1039)list_custom_docker_source_definitions(1166-1189)get_custom_yaml_source_definition(1042-1069)get_custom_docker_source_definition(1192-1217)update_custom_yaml_source_definition(1072-1104)update_custom_docker_source_definition(1220-1252)delete_custom_yaml_source_definition(1107-1126)delete_custom_docker_source_definition(1255-1274)create_custom_docker_destination_definition(1277-1311)list_custom_docker_destination_definitions(1314-1337)get_custom_docker_destination_definition(1340-1365)update_custom_docker_destination_definition(1368-1400)delete_custom_docker_destination_definition(1403-1422)airbyte/cloud/connectors.py (15)
CloudCustomDestinationDefinition(400-485)CloudCustomSourceDefinition(258-397)name(110-118)name(299-303)name(427-431)documentation_url(342-348)documentation_url(448-452)manifest(306-312)_from_yaml_response(370-382)docker_repository(324-330)docker_repository(434-438)docker_image_tag(333-339)docker_image_tag(441-445)_from_docker_response(385-397)_from_docker_response(474-485)airbyte/exceptions.py (2)
PyAirbyteInputError(201-210)AirbyteDuplicateResourcesError(513-517)
⏰ 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). (6)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
airbyte/_util/api_util.py
Outdated
| from airbyte_api import api as airbyte_api_api | ||
| from airbyte_api import models as airbyte_api_models |
There was a problem hiding this comment.
I don't think we need these extra imports. the submodules are already imported in the line above.
- Remove duplicate imports 'api as airbyte_api_api' and 'models as airbyte_api_models' - Update all references to use shorter import names (api, models) - Addresses PR feedback from @aaronsteers All references updated and verified with poe fix-and-check passing. Co-Authored-By: AJ Steers <[email protected]>
| def get_custom_source_definition( | ||
| self, | ||
| definition_id: str, | ||
| ) -> CloudCustomSourceDefinition: |
There was a problem hiding this comment.
Devin, since these exist as two separate domains, let's require the caller to specifically request 'yaml' or 'docker' as type in an input arg.
airbyte/cloud/workspaces.py
Outdated
| def update_custom_source_definition( | ||
| self, | ||
| definition_id: str, | ||
| *, | ||
| name: str | None = None, | ||
| manifest_yaml: dict[str, Any] | Path | str | None = None, | ||
| docker_tag: str | None = None, | ||
| pre_validate: bool = True, | ||
| ) -> CloudCustomSourceDefinition: |
There was a problem hiding this comment.
Let's decouple renaming from updating.
- Add a new "rename_custom_source_definition()" call that just does the rename.
- Determine the connector type based on whether docker_tag or manifest_yaml is specified. (Exactly one is required.)
- Based on what type of update is requested we can determine which type to send to our call to 'get_custom_source_definition()'. (See my prior comment about making that a required input arg.)
There was a problem hiding this comment.
Actually, let's move "update" and "rename" into the more narrow classes: specifically into 'CloudCustomSourceDefinition' and 'CloudCustomDestinationDefinition'. This will remove noise and simplify the CloudWorkspace model to not be so cluttered. Also, there our method names can be shorter - simply rename() and update_definition().
Keep "publish", "get" and "list" operations here as they are now, because Workspace is still the best entry point for those operations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
airbyte/_util/api_util.py (1)
977-1420: Consider extracting common CRUD patterns to reduce duplication?All three definition types (YAML sources, Docker sources, Docker destinations) share nearly identical CRUD logic: build request, call API, check response, raise on failure. The main differences are the request/response types and API endpoints. If the airbyte_api library supports it, could we extract a generic helper that takes the API method and request/response types as parameters? This would reduce duplication and make future additions easier. Wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/_util/api_util.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/_util/api_util.py (2)
airbyte/exceptions.py (2)
PyAirbyteInputError(201-210)AirbyteError(432-447)airbyte/secrets/base.py (1)
SecretString(38-143)
⏰ 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). (6)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (6)
airbyte/_util/api_util.py (6)
938-974: LGTM on the validation helper!The manifest validation logic is clear and follows a sensible pattern: type check, required fields, then value constraint. The dual mode (raise vs. return) gives callers flexibility. Nicely done.
977-1102: LGTM on YAML source create/list/get/update!These four functions follow a consistent pattern: build request, call API, check response, raise
AirbyteErroron failure. The error messages include helpful context (workspace_id, definition_id where applicable). Nice work.
1127-1250: LGTM on Docker source create/list/get/update!These functions mirror the YAML source pattern and correctly handle errors by raising
AirbyteErrorwith context. The optionaldocumentation_urlparameter in create is a nice touch.
1275-1398: LGTM on Docker destination create/list/get/update!These functions follow the same solid pattern as the source functions: build request, call API, check response, raise on failure. The consistency across all three definition types is great for maintainability.
1105-1124: Missing error handling for delete_custom_yaml_source_definition? The delete call doesn’t check the API response like the other CRUD methods; should we verify the result or rely on client exceptions for failure handling? wdyt?
1401-1420: Validate delete_custom_docker_destination_definition response
The function currently ignores the API response; captureresponse = airbyte_instance.destination_definitions.delete_destination_definition(request), checkstatus_ok(response.status_code), and raiseAirbyteErroron failure to align with other CRUD methods—wdyt?
| def delete_custom_docker_source_definition( | ||
| workspace_id: str, | ||
| definition_id: str, | ||
| *, | ||
| api_root: str, | ||
| client_id: SecretString, | ||
| client_secret: SecretString, | ||
| ) -> None: | ||
| """Delete a custom Docker source definition.""" | ||
| airbyte_instance = get_airbyte_server_instance( | ||
| api_root=api_root, | ||
| client_id=client_id, | ||
| client_secret=client_secret, | ||
| ) | ||
|
|
||
| request = api.DeleteSourceDefinitionRequest( | ||
| workspace_id=workspace_id, | ||
| definition_id=definition_id, | ||
| ) | ||
| airbyte_instance.source_definitions.delete_source_definition(request) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Same question on Docker source delete—should we check the response?
Just like the YAML source delete, this function doesn't verify the API response. Should we add a status check and raise AirbyteError on failure for consistency with the other CRUD operations? Wdyt?
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file=$(fd api_util.py | head -n1)
echo "File: $file"
echo "===== update_custom_docker_source_definition (1218-1250) ====="
sed -n '1218,1250p' "$file"
echo
echo "===== delete_custom_docker_source_definition (1253-1272) ====="
sed -n '1253,1272p' "$file"Length of output: 1868
Add error handling in delete_custom_docker_source_definition
Should we capture the result of airbyte_instance.source_definitions.delete_source_definition(request) and raise AirbyteError on failure for consistency with other CRUD methods? wdyt?
🤖 Prompt for AI Agents
In airbyte/_util/api_util.py around lines 1253 to 1272, the
delete_custom_docker_source_definition call ignores the API response and lacks
error handling; update it to capture the result of
airbyte_instance.source_definitions.delete_source_definition(request), check
whether the response indicates success (or contains an error/failed
flag/message), and on failure raise AirbyteError with a clear message and
included response/error details for consistency with other CRUD helpers; ensure
AirbyteError is imported from the appropriate module and that any sensitive
fields are redacted from the error text.
…date - Make custom_connector_type required (not optional) in list/get/delete methods - Add separate rename_custom_source_definition() method for Docker connectors - Refactor update_custom_source_definition() to determine type from parameters - Remove name parameter from update (use rename method instead) - Update all callers: CloudWorkspace methods, MCP tools, dataclass, tests - Add new rename_custom_source_definition MCP tool Addresses PR feedback from @aaronsteers: - Explicit type requirement prevents ambiguity between YAML/Docker domains - Separate rename method clarifies intent vs generic updates - Type determination from parameters simplifies update API API changes: - list_custom_source_definitions: custom_connector_type now required - get_custom_source_definition: custom_connector_type now required - permanently_delete_custom_source_definition: custom_connector_type now required - update_custom_source_definition: removed name parameter, determines type from manifest_yaml vs docker_tag - NEW: rename_custom_source_definition: Docker-only rename operation Breaking changes: All public methods now require explicit type parameter Co-Authored-By: AJ Steers <[email protected]>
- Move update_custom_source_definition to CloudCustomSourceDefinition.update_definition() - Move rename_custom_source_definition to CloudCustomSourceDefinition.rename() - Move update_custom_destination_definition to CloudCustomDestinationDefinition.update_definition() - Update all callers: MCP tools and integration tests - MCP tools now accept manifest_yaml as str | Path (not dict) - Keep publish/get/list/permanently_delete in CloudWorkspace This reduces clutter in CloudWorkspace by moving update/rename operations to the narrower dataclasses where they belong. Methods use shorter names since they're now in type-specific classes. Addresses PR feedback from @aaronsteers Co-Authored-By: AJ Steers <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte/cloud/connectors.py(2 hunks)airbyte/cloud/workspaces.py(3 hunks)airbyte/mcp/cloud_ops.py(3 hunks)tests/integration_tests/cloud/test_custom_definitions.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/integration_tests/cloud/test_custom_definitions.py
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte/cloud/connectors.py
🧰 Additional context used
🧬 Code graph analysis (2)
airbyte/mcp/cloud_ops.py (2)
airbyte/cloud/workspaces.py (6)
publish_custom_source_definition(462-562)list_custom_source_definitions(564-606)get_custom_source_definition(608-639)publish_custom_destination_definition(670-711)list_custom_destination_definitions(713-737)get_custom_destination_definition(739-758)airbyte/cloud/connectors.py (13)
name(113-121)name(302-306)name(549-553)documentation_url(345-351)documentation_url(570-574)docker_repository(327-333)docker_repository(556-560)docker_image_tag(336-342)docker_image_tag(563-567)manifest(309-315)update_definition(368-444)update_definition(588-612)rename(446-482)
airbyte/cloud/workspaces.py (3)
airbyte/_util/api_util.py (13)
validate_yaml_manifest(938-974)create_custom_yaml_source_definition(977-1009)create_custom_docker_source_definition(1127-1161)list_custom_yaml_source_definitions(1012-1037)list_custom_docker_source_definitions(1164-1187)get_custom_yaml_source_definition(1040-1067)get_custom_docker_source_definition(1190-1215)delete_custom_yaml_source_definition(1105-1124)delete_custom_docker_source_definition(1253-1272)create_custom_docker_destination_definition(1275-1309)list_custom_docker_destination_definitions(1312-1335)get_custom_docker_destination_definition(1338-1363)delete_custom_docker_destination_definition(1401-1420)airbyte/cloud/connectors.py (15)
CloudCustomDestinationDefinition(522-633)CloudCustomSourceDefinition(261-519)name(113-121)name(302-306)name(549-553)documentation_url(345-351)documentation_url(570-574)manifest(309-315)_from_yaml_response(492-504)docker_repository(327-333)docker_repository(556-560)docker_image_tag(336-342)docker_image_tag(563-567)_from_docker_response(507-519)_from_docker_response(622-633)airbyte/mcp/cloud_ops.py (4)
publish_custom_source_definition(505-592)list_custom_source_definitions(595-624)publish_custom_destination_definition(732-781)list_custom_destination_definitions(784-798)
⏰ 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). (6)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
| if custom_connector_type == "yaml": | ||
| result = api_util.get_custom_yaml_source_definition( | ||
| workspace_id=self.workspace_id, | ||
| definition_id=definition_id, | ||
| api_root=self.api_root, | ||
| client_id=self.client_id, | ||
| client_secret=self.client_secret, | ||
| ) | ||
| return CloudCustomSourceDefinition._from_yaml_response(self, result) # noqa: SLF001 | ||
| result = api_util.get_custom_docker_source_definition( | ||
| workspace_id=self.workspace_id, | ||
| definition_id=definition_id, | ||
| api_root=self.api_root, | ||
| client_id=self.client_id, | ||
| client_secret=self.client_secret, | ||
| ) | ||
| return CloudCustomSourceDefinition._from_docker_response(self, result) # noqa: SLF001 |
There was a problem hiding this comment.
Add an explicit guard for unsupported connector types
When custom_connector_type is anything other than the exact string "yaml", we fall through to the Docker path. A small typo (for example "YAML") ends up calling the Docker endpoint with a YAML definition ID, which surfaces a misleading API error and makes the MCP tools frustrating to use. Could we add an explicit else that raises exc.PyAirbyteInputError so we fail fast with a clear message (and reuse the same guard in permanently_delete_custom_source_definition just below), wdyt?
if custom_connector_type == "yaml":
result = api_util.get_custom_yaml_source_definition(
workspace_id=self.workspace_id,
definition_id=definition_id,
api_root=self.api_root,
client_id=self.client_id,
client_secret=self.client_secret,
)
return CloudCustomSourceDefinition._from_yaml_response(self, result) # noqa: SLF001
- result = api_util.get_custom_docker_source_definition(
- workspace_id=self.workspace_id,
- definition_id=definition_id,
- api_root=self.api_root,
- client_id=self.client_id,
- client_secret=self.client_secret,
- )
- return CloudCustomSourceDefinition._from_docker_response(self, result) # noqa: SLF001
+ if custom_connector_type == "docker":
+ result = api_util.get_custom_docker_source_definition(
+ workspace_id=self.workspace_id,
+ definition_id=definition_id,
+ api_root=self.api_root,
+ client_id=self.client_id,
+ client_secret=self.client_secret,
+ )
+ return CloudCustomSourceDefinition._from_docker_response(self, result) # noqa: SLF001
+ raise exc.PyAirbyteInputError(
+ message="custom_connector_type must be 'yaml' or 'docker'",
+ context={
+ "definition_id": definition_id,
+ "custom_connector_type": custom_connector_type,
+ },
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if custom_connector_type == "yaml": | |
| result = api_util.get_custom_yaml_source_definition( | |
| workspace_id=self.workspace_id, | |
| definition_id=definition_id, | |
| api_root=self.api_root, | |
| client_id=self.client_id, | |
| client_secret=self.client_secret, | |
| ) | |
| return CloudCustomSourceDefinition._from_yaml_response(self, result) # noqa: SLF001 | |
| result = api_util.get_custom_docker_source_definition( | |
| workspace_id=self.workspace_id, | |
| definition_id=definition_id, | |
| api_root=self.api_root, | |
| client_id=self.client_id, | |
| client_secret=self.client_secret, | |
| ) | |
| return CloudCustomSourceDefinition._from_docker_response(self, result) # noqa: SLF001 | |
| if custom_connector_type == "yaml": | |
| result = api_util.get_custom_yaml_source_definition( | |
| workspace_id=self.workspace_id, | |
| definition_id=definition_id, | |
| api_root=self.api_root, | |
| client_id=self.client_id, | |
| client_secret=self.client_secret, | |
| ) | |
| return CloudCustomSourceDefinition._from_yaml_response(self, result) # noqa: SLF001 | |
| if custom_connector_type == "docker": | |
| result = api_util.get_custom_docker_source_definition( | |
| workspace_id=self.workspace_id, | |
| definition_id=definition_id, | |
| api_root=self.api_root, | |
| client_id=self.client_id, | |
| client_secret=self.client_secret, | |
| ) | |
| return CloudCustomSourceDefinition._from_docker_response(self, result) # noqa: SLF001 | |
| raise exc.PyAirbyteInputError( | |
| message="custom_connector_type must be 'yaml' or 'docker'", | |
| context={ | |
| "definition_id": definition_id, | |
| "custom_connector_type": custom_connector_type, | |
| }, | |
| ) |
🤖 Prompt for AI Agents
In airbyte/cloud/workspaces.py around lines 623 to 639, add an explicit guard
for unsupported custom_connector_type values instead of implicitly falling
through to the Docker path: if custom_connector_type == "yaml" call
get_custom_yaml_source_definition and return the YAML response; elif
custom_connector_type == "docker" call get_custom_docker_source_definition and
return the Docker response; otherwise raise exc.PyAirbyteInputError with a clear
message (including the invalid value and the supported options like "yaml" and
"docker") so callers fail fast. Apply the same explicit branching and error
guard to permanently_delete_custom_source_definition just below to keep behavior
consistent.
- Replace TEST_YAML_MANIFEST with complete structure including spec section - Add definitions section following real manifest patterns - Skip Docker custom definition tests pending API support confirmation - The missing spec section was causing 500 error 'get(...) must not be null' Co-Authored-By: AJ Steers <[email protected]>
1 participant
feat: Add support for publishing custom connector definitions
Summary
Implements PyAirbyte support for publishing custom connector definitions using new Airbyte 1.6 API endpoints, enabling users to create YAML connectors without Docker builds and register custom Docker images.
Added Support for 3 Definition Types:
Implementation Details:
api_util.pyfor full CRUD operations (create, list, get, update, delete)pre_validate=Trueby default)declarative_source_definitions,source_definitions,destination_definitionsendpointsUsage Examples:
Review & Testing Checklist for Human
🔴 High Risk - 5 items to verify:
validate_yaml_manifest()catches common errors but isn't overly restrictive for valid manifestsNotes
@pytest.mark.requires_credsand only run with valid Airbyte Cloud credentialsRequested by: @aaronsteers
Devin session: https://app.devin.ai/sessions/7733e25275f44008ab6cb765d4ef5106
Summary by CodeRabbit
New Features
Integrations
Tests