feat: Add interfaces for editing cloud source, destination, and connection configs#841
Conversation
…configs - Add patch_source(), patch_destination(), and patch_connection() API utility functions - Add update_config() methods to CloudSource, CloudDestination, and CloudConnection classes - Add MCP tools for updating cloud sources, destinations, and connections - Mark all update operations as destructive in MCP tool annotations Co-Authored-By: AJ Steers <aj@airbyte.io>
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/add-cloud-config-update-interfaces' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/add-cloud-config-update-interfaces'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
devin-ai-integration
Bot
changed the title
|
Warning Rate limit exceededdevin-ai-integration[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds API helpers to PATCH sources, destinations, and connections; exposes rename and config/connection-modifying methods on CloudSource, CloudDestination, and CloudConnection (including set_table_prefix and set_selected_streams); and registers new MCP tools to invoke those operations. Duplicate tool definitions were introduced in the same file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant MCP as MCP Tool
participant Cloud as Cloud object\n(CloudSource/CloudDestination/CloudConnection)
participant API as api_util\n(patch_*)
participant Server as Airbyte API
MCP->>Cloud: call rename/update_config/set_table_prefix/set_selected_streams
activate Cloud
Cloud->>API: patch_source/patch_destination/patch_connection(...)
activate API
API->>Server: PATCH /sources|/destinations|/connections (Patch*Request)
Server-->>API: 200 OK + updated resource
deactivate API
Cloud->>Cloud: update cached _*_info with response
Cloud-->>MCP: return self or resource URL
deactivate Cloud
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 (3)
690-744: Consider validating that at least one parameter is provided, wdyt?The function works correctly, but currently allows calls with all parameters set to
None, which would result in an API call that doesn't actually change anything. Should we add a check to ensure at least one ofname,config, orconfig_secret_nameis provided before making the API call? This would provide earlier feedback to the user and avoid unnecessary API calls.Example:
+ if name is None and config is None and config_secret_name is None: + return "No updates provided. Specify at least one of: name, config, or config_secret_name" + try: workspace: CloudWorkspace = _get_cloud_workspace()That said, the current implementation is still functionally correct—the API will handle no-op updates gracefully.
747-804: Consider the same parameter validation as suggested for sources, wdyt?This function has the same structure and considerations as
update_cloud_source_config. It might benefit from the same optional validation to ensure at least one parameter is provided before making an API call.
807-870: Consider parameter validation here too, wdyt?Similar to the source and destination update functions, this allows calls with all parameters as
None. Adding validation would provide clearer feedback:+ if name is None and prefix is None and status is None: + return "No updates provided. Specify at least one of: name, prefix, or status" + try: workspace: CloudWorkspace = _get_cloud_workspace()The status enum conversion looks good—it will be caught by the try/except if an invalid status string is provided.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte/_util/api_util.py(3 hunks)airbyte/cloud/connections.py(2 hunks)airbyte/cloud/connectors.py(2 hunks)airbyte/mcp/cloud_ops.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
airbyte/cloud/connectors.py (2)
airbyte/cloud/connections.py (4)
update_config(262-298)name(87-95)source_id(98-106)destination_id(121-129)airbyte/_util/api_util.py (2)
patch_source(567-615)patch_destination(755-803)
airbyte/mcp/cloud_ops.py (4)
airbyte/cloud/connections.py (7)
source_id(98-106)name(87-95)source(109-118)update_config(262-298)destination_id(121-129)destination(132-141)connection_url(160-162)airbyte/cloud/connectors.py (7)
source_id(186-191)name(113-121)name(363-367)update_config(202-229)update_config(272-299)connector_url(129-131)destination_id(256-261)airbyte/cloud/workspaces.py (3)
get_source(119-131)get_destination(133-145)get_connection(105-117)airbyte/mcp/_util.py (1)
resolve_config(129-226)
airbyte/cloud/connections.py (2)
airbyte/cloud/connectors.py (4)
update_config(202-229)update_config(272-299)name(113-121)name(363-367)airbyte/_util/api_util.py (1)
patch_connection(920-977)
airbyte/_util/api_util.py (2)
airbyte/secrets/base.py (1)
SecretString(38-143)airbyte/exceptions.py (1)
AirbyteError(432-447)
⏰ 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, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (9)
airbyte/_util/api_util.py (3)
567-615: LGTM! Clean implementation following established patterns.The
patch_sourcefunction follows the same structure as other API utility functions in this file (likecreate_sourceandget_source). The error handling is appropriate, withsource_idincluded in the context for debugging. The docstring clearly warns about the destructive nature of this operation.
755-803: LGTM! Consistent with patch_source and existing patterns.The
patch_destinationfunction mirrors the structure ofpatch_sourceand follows the same patterns as other destination-related functions in this file. Error handling and documentation are appropriate.
920-977: LGTM! Comprehensive connection update support.The
patch_connectionfunction appropriately handles multiple optional parameters (name, configurations, schedule, prefix, status) to support partial updates. Thenoqafor too many arguments is justified given the inherent complexity of connection configuration. The function follows established patterns and includes proper error handling.airbyte/cloud/connectors.py (2)
202-229: LGTM! Clean delegation and state management.The
update_configmethod properly delegates toapi_util.patch_sourceand updates the cached connector info with the API response. Returningselfenables method chaining, which is a nice ergonomic touch. The destructive operation warning is appropriately documented.
272-299: LGTM! Consistent with CloudSource.update_config.The
update_configmethod for destinations follows the same pattern as the source implementation. Proper delegation, state management, and documentation are all in place.airbyte/cloud/connections.py (2)
14-14: LGTM! Proper TYPE_CHECKING import for type annotations.The import of
api_modelsis correctly placed under theTYPE_CHECKINGblock to avoid runtime overhead while providing type hints for the newupdate_configmethod.
262-298: LGTM! Comprehensive connection update support.The
update_configmethod provides a clean interface for updating connection configuration with multiple optional parameters. It follows the same pattern as the source and destination update methods, properly delegates toapi_util.patch_connection, and updates the cached state appropriately. The documentation clearly warns about the destructive nature of the operation.airbyte/mcp/cloud_ops.py (2)
7-7: LGTM! Runtime import for enum conversion.The
api_modelsimport is correctly added at the module level (not underTYPE_CHECKING) since it's needed at runtime for converting the status string toConnectionStatusEnuminupdate_cloud_connection_config.
991-1010: LGTM! Proper tool registration with destructive annotations.All three update tools are correctly registered with
DESTRUCTIVE_HINT: True, which appropriately reflects their nature. This is consistent with other destructive operations in the tool registry.
- Split CloudSource.update_config() into rename() and update_config() - Split CloudDestination.update_config() into rename() and update_config() - Split CloudConnection.update_config() into rename(), update_schedule(), and update_config() - Removed status parameter from CloudConnection.update_config() (no use cases) - Created separate MCP tools: rename_cloud_source, rename_cloud_destination, rename_cloud_connection - Removed blanket try/except from MCP tools to allow organic error propagation - Mark rename operations as non-destructive (DESTRUCTIVE_HINT: False) Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
airbyte/mcp/cloud_ops.py (1)
689-880: Consider refactoring to reduce duplication across similar operations, wdyt?The rename functions for source/destination/connection and the update_config functions follow nearly identical patterns. You might consider extracting a generic helper, something like:
def _rename_cloud_resource( resource_type: Literal["source", "destination", "connection"], resource_id: str, name: str, ) -> str: workspace = _get_cloud_workspace() resource = getattr(workspace, f"get_{resource_type}")( **{f"{resource_type}_id": resource_id} ) resource.rename(name=name) url = getattr(resource, "connector_url" if resource_type != "connection" else "connection_url") return f"Successfully renamed {resource_type} '{resource_id}' to '{name}'. URL: {url}"This would reduce the 200+ lines of similar code to ~50 lines. However, the explicit functions might be clearer for API consumers, so this is a tradeoff between DRY and explicitness. What do you think?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte/cloud/connections.py(2 hunks)airbyte/cloud/connectors.py(2 hunks)airbyte/mcp/cloud_ops.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
airbyte/cloud/connections.py (2)
airbyte/cloud/connectors.py (7)
rename(202-219)rename(284-301)rename(591-619)name(113-121)name(387-391)update_config(221-241)update_config(303-323)airbyte/_util/api_util.py (1)
patch_connection(920-977)
airbyte/cloud/connectors.py (2)
airbyte/cloud/connections.py (5)
rename(260-277)name(87-95)source_id(98-106)update_config(301-328)destination_id(121-129)airbyte/_util/api_util.py (2)
patch_source(567-615)patch_destination(755-803)
airbyte/mcp/cloud_ops.py (4)
airbyte/cloud/connections.py (8)
source_id(98-106)name(87-95)source(109-118)rename(260-277)update_config(301-328)destination_id(121-129)destination(132-141)connection_url(160-162)airbyte/cloud/connectors.py (10)
source_id(186-191)name(113-121)name(387-391)rename(202-219)rename(284-301)rename(591-619)connector_url(129-131)update_config(221-241)update_config(303-323)destination_id(268-273)airbyte/cloud/workspaces.py (3)
get_source(119-131)get_destination(133-145)get_connection(105-117)airbyte/mcp/_util.py (1)
resolve_config(129-226)
⏰ 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, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (6)
airbyte/cloud/connectors.py (2)
202-241: LGTM! Clean implementation of rename and update operations.Both methods follow a consistent pattern and correctly update internal state only when the API call succeeds. The fluent interface (returning
self) is a nice touch for method chaining. Docstrings appropriately warn users about the destructive nature of config updates.
284-323: LGTM! Consistent implementation across destination operations.These methods mirror the CloudSource implementations perfectly, maintaining consistency across the codebase. Well done!
airbyte/cloud/connections.py (2)
14-14: LGTM! Proper use of TYPE_CHECKING.Correctly importing
api_modelsonly for type annotations avoids unnecessary runtime dependencies.
260-328: LGTM! Operations successfully decoupled per previous feedback.The operations are now properly separated into
rename(),update_schedule(), andupdate_config()methods. Each has a clear, focused responsibility and the destructive operations are well-documented. The implementation is clean and consistent.airbyte/mcp/cloud_ops.py (2)
689-708: LGTM! Clean implementation without blanket exception handling.Good to see the function lets exceptions propagate naturally as requested in previous feedback. The implementation is straightforward and focused.
1001-1041: LGTM! Tool registrations use appropriate destructive hints.The annotations correctly distinguish between rename operations (non-destructive) and config updates (destructive). Well categorized!
- Removed update_schedule() method to avoid exposing internal API models - Created feature request issue #843 for proper scheduling interface - Proposed interface uses simple cron string instead of internal models Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte/cloud/connections.py (1)
279-306: Nice implementation! Aligns well with past feedback.The
update_configmethod correctly implements the past review suggestion: accepting configurations and prefix only, with status and schedule removed. The destructive warning is clearly documented, and the pattern is consistent with similar methods inCloudSourceandCloudDestination.One optional consideration: should we validate that at least one parameter is provided? Currently, calling
update_config()with no arguments would still hit the API. This might be fine if the API handles it gracefully, but explicit validation could provide clearer feedback to users. Wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/cloud/connections.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/cloud/connections.py (2)
airbyte/cloud/connectors.py (7)
rename(202-219)rename(284-301)rename(591-619)name(113-121)name(387-391)update_config(221-241)update_config(303-323)airbyte/_util/api_util.py (1)
patch_connection(920-977)
⏰ 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)
🔇 Additional comments (1)
airbyte/cloud/connections.py (1)
260-277: LGTM! Clean separation of rename operation.The
renamemethod is nicely decoupled and follows the same pattern asCloudSourceandCloudDestination. The implementation is straightforward: patch the name via API, refresh local state, and return self for chaining. This aligns well with the past review feedback about separating rename from other updates.
|
You're right - we should use |
- Changed configurations parameter type from api_models.StreamConfigurationsInput to dict - Avoids exposing internal API models in public interface - Matches MCP tool signature which already uses dict Co-Authored-By: AJ Steers <aj@airbyte.io>
|
Devin, update from |
- Merged latest main with decorator-based tool registration - Applied @mcp_tool decorator to all six new functions - Marked rename operations as non-destructive (open_world=True) - Marked update_config operations as destructive (destructive=True, open_world=True) - Removed manual tool registration in favor of register_tools() Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
airbyte/mcp/cloud_ops.py (3)
940-946: Verify type compatibility for the configurations parameter, wdyt?The parameter is typed as
dict | None, butCloudConnection.update_config()(and ultimatelyapi_util.patch_connection()) expectsmodels.StreamConfigurationsInput | None. This type mismatch could cause runtime validation errors. Should we update the type hint to match the downstream expectation?Based on past review feedback.
812-816: Pass source.config_spec to enable hardcoded secret detection.The comment "We don't have the spec here" is misleading—the source object fetched at line 810 has a config_spec property. Passing
Nonebypasses hardcoded secret detection inresolve_config(). Should we change line 815 toconfig_spec_jsonschema=source.config_specto match the pattern used indeploy_source_to_cloud()(line 85)?Based on past review feedback.
Apply this diff:
config_dict = resolve_config( config=config, config_secret_name=config_secret_name, - config_spec_jsonschema=None, # We don't have the spec here + config_spec_jsonschema=source.config_spec, )
887-891: Pass destination.config_spec to enable hardcoded secret detection.Same issue as in
update_cloud_source_config(): the destination object at line 885 has a config_spec property available. Should we change line 890 toconfig_spec_jsonschema=destination.config_specto match the pattern indeploy_destination_to_cloud()(line 155)?Based on past review feedback.
Apply this diff:
config_dict = resolve_config( config=config, config_secret_name=config_secret_name, - config_spec_jsonschema=None, # We don't have the spec here + config_spec_jsonschema=destination.config_spec, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte/cloud/connections.py(1 hunks)airbyte/mcp/cloud_ops.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
airbyte/cloud/connections.py (2)
airbyte/cloud/connectors.py (7)
rename(202-219)rename(284-301)rename(591-619)name(113-121)name(387-391)update_config(221-241)update_config(303-323)airbyte/_util/api_util.py (1)
patch_connection(920-977)
airbyte/mcp/cloud_ops.py (5)
airbyte/mcp/_tool_utils.py (1)
mcp_tool(83-129)airbyte/cloud/connections.py (8)
source_id(97-105)name(86-94)source(108-117)rename(259-276)update_config(278-305)destination_id(120-128)destination(131-140)connection_url(159-161)airbyte/cloud/connectors.py (10)
source_id(186-191)name(113-121)name(387-391)rename(202-219)rename(284-301)rename(591-619)connector_url(129-131)update_config(221-241)update_config(303-323)destination_id(268-273)airbyte/cloud/workspaces.py (3)
get_source(119-131)get_destination(133-145)get_connection(105-117)airbyte/mcp/_util.py (1)
resolve_config(129-226)
🪛 GitHub Actions: Run Linters
airbyte/mcp/cloud_ops.py
[error] 892-898: Ruff format requested changes. 1 file would be reformatted by 'ruff format --diff'.
[error] 964-970: Ruff format requested changes. 1 file would be reformatted by 'ruff format --diff'.
⏰ 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, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (4)
airbyte/cloud/connections.py (1)
259-276: LGTM! Clean implementation following the established pattern.The rename method correctly mirrors the implementation in CloudSource and CloudDestination, updates the cached connection info, and returns self for chaining.
airbyte/mcp/cloud_ops.py (3)
750-773: LGTM! Clean implementation of the rename operation.The function correctly retrieves the source, calls the rename method, and returns a descriptive success message. The lack of a destructive flag is appropriate since renaming is non-destructive.
822-848: LGTM! Consistent with the source rename implementation.The function follows the same clean pattern as
rename_cloud_source()and appropriately omits the destructive flag.
900-926: LGTM! Consistent rename implementation for connections.The function follows the established pattern from source and destination rename operations.
|
You're absolutely right - during creation we only expose I can see two options:
Which approach would you prefer? I'm leaning toward option 1 (remove entirely) since:
Let me know and I'll update accordingly! |
- Removed CloudConnection.update_config() method - Added CloudConnection.set_table_prefix() for setting table prefix - Added CloudConnection.set_selected_streams() for setting selected streams - Replaced update_cloud_connection_config MCP tool with two separate tools: - set_cloud_connection_table_prefix (non-destructive) - set_cloud_connection_selected_streams (destructive) - This matches the interface used during connection creation (selected_streams + table_prefix) Co-Authored-By: AJ Steers <aj@airbyte.io>
|
Done! I've replaced
This now matches the interface we use during connection creation ( The MCP tools have also been updated:
All linting and type checking passes. Changes pushed to the PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
airbyte/cloud/connections.py (1)
309-316: Type mismatch: configurations parameter expects StreamConfigurationsInput, not dict.At line 309, you're constructing a plain dict, but
api_util.patch_connectionexpectsmodels.StreamConfigurationsInput | Nonefor theconfigurationsparameter. While duck typing might make this work at runtime, it creates a type safety gap and could fail API validation.Should we update line 309 to properly construct a
models.StreamConfigurationsInputobject instead of a plain dict, wdyt?- configurations = {"streams": [{"name": stream_name} for stream_name in stream_names]} + from airbyte_api import models + configurations = models.StreamConfigurationsInput( + streams=[models.StreamConfiguration(name=stream_name) for stream_name in stream_names] + )airbyte/mcp/cloud_ops.py (2)
812-816: Misleading comment: config spec IS available on the source object.The comment at line 815 states "We don't have the spec here", but this is incorrect. The
sourceobject fetched at line 810 has aconfig_specproperty available. PassingNonebypasses hardcoded secret detection inresolve_config(), which could allow credentials to be accidentally committed.Should we update line 815 to pass
source.config_specinstead ofNone, wdyt?config_dict = resolve_config( config=config, config_secret_name=config_secret_name, - config_spec_jsonschema=None, # We don't have the spec here + config_spec_jsonschema=source.config_spec, )Based on learnings.
887-891: Misleading comment: config spec IS available on the destination object.The comment at line 890 states "We don't have the spec here", but the
destinationobject fetched at line 885 has aconfig_specproperty available. PassingNonebypasses hardcoded secret detection.Should we update line 890 to pass
destination.config_specinstead ofNone, wdyt?config_dict = resolve_config( config=config, config_secret_name=config_secret_name, - config_spec_jsonschema=None, # We don't have the spec here + config_spec_jsonschema=destination.config_spec, )Based on learnings.
🧹 Nitpick comments (1)
airbyte/mcp/cloud_ops.py (1)
928-954: Consider marking this tool as destructive.While less severe than changing stream configurations, updating the table prefix could break downstream systems or queries that expect specific table names. Should we add
destructive=Trueto the decorator to signal this risk to users, wdyt?@mcp_tool( domain="cloud", + destructive=True, open_world=True, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte/cloud/connections.py(1 hunks)airbyte/mcp/cloud_ops.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
airbyte/cloud/connections.py (2)
airbyte/cloud/connectors.py (5)
rename(202-219)rename(284-301)rename(591-619)name(113-121)name(387-391)airbyte/_util/api_util.py (1)
patch_connection(920-977)
airbyte/mcp/cloud_ops.py (5)
airbyte/mcp/_tool_utils.py (1)
mcp_tool(83-129)airbyte/cloud/connections.py (10)
source_id(97-105)name(86-94)source(108-117)rename(259-276)destination_id(120-128)destination(131-140)connection_url(159-161)set_table_prefix(278-295)stream_names(143-148)set_selected_streams(297-319)airbyte/cloud/connectors.py (10)
source_id(186-191)name(113-121)name(387-391)rename(202-219)rename(284-301)rename(591-619)connector_url(129-131)update_config(221-241)update_config(303-323)destination_id(268-273)airbyte/cloud/workspaces.py (4)
CloudWorkspace(64-612)get_source(119-131)get_destination(133-145)get_connection(105-117)airbyte/mcp/_util.py (1)
resolve_config(129-226)
⏰ 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, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (6)
airbyte/cloud/connections.py (2)
259-276: LGTM! Clean implementation of rename method.This follows the same pattern as the
renamemethods inCloudSourceandCloudDestinationclasses, maintaining consistency across the codebase. The method correctly updates the internal state and returns self for chaining.
278-295: LGTM! Table prefix setter looks good.The implementation is straightforward and correctly updates the connection configuration through the API.
airbyte/mcp/cloud_ops.py (4)
750-773: LGTM! Clean rename implementation for sources.The tool correctly omits the
destructiveflag since renaming is a safe operation. The implementation is straightforward and returns a helpful message.
822-848: LGTM! Destination rename implementation mirrors source rename.Consistent with the source rename tool and correctly omits the
destructiveflag.
899-925: LGTM! Connection rename tool looks good.Clean implementation that correctly omits the
destructiveflag for this safe operation.
957-995: LGTM! Selected streams setter is correctly marked as destructive.The implementation properly handles the stream_names parameter (supporting both string and list inputs) and correctly marks this as a destructive operation.
- Changed from dict to api_models.StreamConfigurationsInput - Create StreamConfiguration objects for each stream name - This matches the pattern used in create_connection() Co-Authored-By: AJ Steers <aj@airbyte.io>
|
High confidence! I've updated the implementation to use the proper API models instead of a dict. Confirmation of approach: stream_configurations: list[models.StreamConfiguration] = []
if selected_stream_names:
for stream_name in selected_stream_names:
stream_configuration = models.StreamConfiguration(
name=stream_name,
)
stream_configurations.append(stream_configuration)
stream_configurations_obj = models.StreamConfigurations(stream_configurations)My implementation creates Reference: See Changes pushed in commit 16994db. |
- Added _build_stream_configurations() static method - Improves code readability and reviewer confidence - Documents the pattern matching connection creation Co-Authored-By: AJ Steers <aj@airbyte.io>
- Moved build_stream_configurations() to api_util.py - Updated create_connection() to use the shared helper - Updated CloudConnection.set_selected_streams() to use the shared helper - Ensures consistency between connection creation and updates Co-Authored-By: AJ Steers <aj@airbyte.io>
|
Perfect! I've refactored to use a shared helper function for both operations: Changes made:
Benefits:
The helper creates Changes pushed in commit 20e5a08. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
airbyte/cloud/connections.py (2)
124-130: Critical: destination_id property assigns source_id by mistake.This returns the wrong ID and will cascade into incorrect behavior (e.g., repr, deletions). Fix the assignment. wdyt?
Apply this diff:
- self._destination_id = self._connection_info.source_id + self._destination_id = self._connection_info.destination_id
145-151: Guard None when configurations/streams are absent.
self._connection_info.configurationscan be None; accessing.streamswould raise. Add a safe fallback. wdyt?Apply this diff:
- return [stream.name for stream in self._connection_info.configurations.streams or []] + streams = getattr(getattr(self._connection_info, "configurations", None), "streams", None) or [] + return [s.name for s in streams]
♻️ Duplicate comments (1)
airbyte/cloud/connections.py (1)
299-325: StreamConfiguration missing required fields—code likely incompleteThe current
set_selected_streamsimplementation only passesnametoStreamConfiguration, but airbyte-api v0.53.0 requires at minimum:stream(AirbyteStream object),sync_mode, anddestination_sync_mode. This will likely fail at runtime.To fix:
- Retrieve current stream configurations (with existing sync/destination modes) before patching, so you can preserve or intelligently set these required fields.
- Alternatively, accept them as parameters to
set_selected_streams.Beyond the required fields, your original suggestions still apply and are worth considering:
- Accept
Iterable[str]instead oflist[str]and de-duplicate while preserving order—would that work better for your use case?- Early return if the selection is unchanged—helps avoid unnecessary API calls?
- Validate names against current
stream_namesto fail fast?
🧹 Nitpick comments (2)
airbyte/cloud/connections.py (2)
261-279: Idempotency: skip PATCH if the name is unchanged?Nice addition. To avoid unnecessary API calls, consider an early return when the name already matches. wdyt?
def rename(self, name: str) -> CloudConnection: """Rename the connection. @@ - updated_response = api_util.patch_connection( + if self._connection_info and self._connection_info.name == name: + return self + updated_response = api_util.patch_connection(
280-298: Prefix handling: allow clearing, normalize, and idempotency?Would you like to support clearing the prefix (None/empty), strip whitespace, and skip PATCH if unchanged, to make this safer/idempotent? wdyt?
- def set_table_prefix(self, prefix: str) -> CloudConnection: + def set_table_prefix(self, prefix: str | None) -> CloudConnection: @@ - updated_response = api_util.patch_connection( + # Normalize: treat empty string as None (clear) + norm = (prefix or "").strip() + norm = norm if norm != "" else None + if self._connection_info and (self._connection_info.prefix or None) == norm: + return self + updated_response = api_util.patch_connection( @@ - prefix=prefix, + prefix=norm,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/cloud/connections.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/cloud/connections.py (2)
airbyte/cloud/connectors.py (5)
rename(202-219)rename(284-301)rename(591-619)name(113-121)name(387-391)airbyte/_util/api_util.py (1)
patch_connection(920-977)
⏰ 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.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (1)
airbyte/cloud/connections.py (1)
8-9: Import alias looks good.Using
api_modelskeeps callsites concise and consistent withapi_utilusage, LGTM. wdyt?
Aaron ("AJ") Steers (aaronsteers)
deleted the
devin/add-cloud-config-update-interfaces
branch
1 participant
Add interfaces for editing cloud source, destination, and connection configs
Summary
This PR adds update/patch capabilities for PyAirbyte's Cloud resources (sources, destinations, and connections), along with corresponding MCP tools for AI assistant access. All update operations are marked as destructive since configuration changes can easily break existing connectors.
Changes:
patch_source(),patch_destination(), andpatch_connection()API utility functions inairbyte/_util/api_util.pyupdate_config()methods toCloudSource,CloudDestination, andCloudConnectionclassesupdate_cloud_source_config,update_cloud_destination_config,update_cloud_connection_configDESTRUCTIVE_HINT: TrueannotationNote on Connection Updates: The MCP tool for connections intentionally only supports updating
name,prefix, andstatus(not stream configurations or schedules, which require complex data structures and should be managed through the Airbyte UI).Review & Testing Checklist for Human
update_config(), confirm the object's internal state is properly refreshed and no fields are lostTest Plan Recommendation
source.update_config(name="New Name", config={...})update_cloud_source_configwith a source IDNotes
_connector_info,_connection_info) assume API responses are completeRequested by: AJ Steers (aj@airbyte.io, Aaron ("AJ") Steers (@aaronsteers))
Devin Session: https://app.devin.ai/sessions/f4a3290c001145d9b5fe33142727e2d1
Summary by CodeRabbit