-
Notifications
You must be signed in to change notification settings - Fork 44
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
Feat(cloud): Add check
feature for Cloud connectors
#562
Conversation
…er-image-for-connectors
check
feature for Cloud connectors
Warning Rate limit exceeded@aaronsteers has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 55 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 (3)
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Airbyte API utility functions, focusing on cloud configuration and connector management. New constants and methods are added to Changes
Sequence DiagramsequenceDiagram
participant Client
participant CloudConnector
participant APIUtil
participant ConfigAPI
Client->>CloudConnector: check()
CloudConnector->>APIUtil: check_connector()
APIUtil->>APIUtil: get_bearer_token()
APIUtil->>ConfigAPI: Make API request
ConfigAPI-->>APIUtil: Return check result
APIUtil-->>CloudConnector: Return (success, message)
CloudConnector-->>Client: Return CheckResult
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 1
🧹 Nitpick comments (3)
airbyte/cloud/connectors.py (1)
9-9
: Consider using the standarddataclasses
module for the@dataclass
decoratorCurrently,
dataclass
is imported fromattr
, but the standard library'sdataclasses
module provides a built-in@dataclass
decorator. Would it be better to import fromdataclasses
for consistency and to avoid external dependencies? Wdyt?airbyte/_util/api_util.py (1)
17-17
: Remove unused importdataclasses.dataclass
The import
dataclasses.dataclass
on line 17 is not used in the code. Shall we remove it to clean up the imports? Wdyt?Apply this diff to remove the unused import:
-from dataclasses import dataclass
🧰 Tools
🪛 Ruff (0.8.2)
17-17:
dataclasses.dataclass
imported but unusedRemove unused import:
dataclasses.dataclass
(F401)
tests/integration_tests/cloud/test_cloud_api_util.py (1)
265-265
: Reconsider the commented-out test case intest_check_connector
There's a commented-out test case in the parameter list for
test_check_connector
. Should we include this test case to enhance coverage, or remove it to keep the code clean? Wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte/_util/api_util.py
(9 hunks)airbyte/cloud/connectors.py
(2 hunks)tests/integration_tests/cloud/test_cloud_api_util.py
(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
airbyte/_util/api_util.py
17-17: dataclasses.dataclass
imported but unused
Remove unused import: dataclasses.dataclass
(F401)
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 (5)
airbyte/cloud/workspaces.py (2)
7-33
: Great documentation additions! Would you consider adding type hints to the examples? 🤔The usage examples are clear and practical. To maintain consistency with the rest of the codebase's typing practices, we could add type hints in the example code. wdyt?
-workspace = cloud.CloudWorkspace( +workspace: cloud.CloudWorkspace = cloud.CloudWorkspace( workspace_id="...", client_id="...", client_secret="...", ) -source = ab.get_source("source-faker", config={"count": 100}) +source: ab.Source = ab.get_source("source-faker", config={"count": 100}) -deployed_source = workspace.deploy_source( +deployed_source: CloudSource = workspace.deploy_source(
97-137
: The new methods look great! A few suggestions to make them even better 🌟
- Would you consider adding return type info to the docstrings? For example:
"""Get a connection by ID. This method does not fetch data from the API. It returns a `CloudConnection` object, which will be loaded lazily as needed. Returns: CloudConnection: A lazy-loaded connection object. """
- For better type safety, we could use NewType for IDs. wdyt?
from typing import NewType ConnectionId = NewType('ConnectionId', str) SourceId = NewType('SourceId', str) DestinationId = NewType('DestinationId', str)
- For consistency, should we align the parameter names?
connection_id
vsconnector_id
in different methods.airbyte/cloud/connectors.py (3)
2-38
: Consider enhancing the example's error handling?The documentation is comprehensive and well-structured! For the usage example, what do you think about making the error handling more specific by showing how to access both
error_message
andinternal_error
? This could help users better understand the difference between these fields. Wdyt?if not check_result: if check_result.error_message: print(f"Check failed: {check_result.error_message}") elif check_result.internal_error: print(f"Internal error: {check_result.internal_error}")
71-73
: Consider handling both error types in str?The current implementation only shows
error_message
. Should we also includeinternal_error
when present? This would make the string representation more informative. Something like:- return "Success" if self.success else f"Failed: {self.error_message}" + if self.success: + return "Success" + return f"Failed: {self.error_message or self.internal_error or 'Unknown error'}"
115-142
: Consider enhancing error handling and tuple unpacking?The implementation looks solid! A couple of suggestions to make it even better:
- What do you think about making the error message more specific by including both error types? Like:
- raise ValueError(f"Check failed: {check_result}") + msg = check_result.error_message or check_result.internal_error or "Unknown error" + raise ValueError(f"Connector check failed: {msg}")
- The tuple unpacking could be more explicit for better readability:
- result = api_util.check_connector( + success, error_message = api_util.check_connector( workspace_id=self.workspace.workspace_id, connector_type=self.connector_type, actor_id=self.connector_id, api_root=self.workspace.api_root, client_id=self.workspace.client_id, client_secret=self.workspace.client_secret, ) check_result = CheckResult( - success=result[0], - error_message=result[1], + success=success, + error_message=error_message, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte/cloud/connectors.py
(2 hunks)airbyte/cloud/workspaces.py
(2 hunks)
🔇 Additional comments (3)
airbyte/cloud/workspaces.py (1)
97-109
: Verify references to the old get_connection method
The AI summary mentioned that an old get_connection
method was removed. Let's ensure all references are updated.
✅ Verification successful
All references to get_connection look consistent with the new implementation
Looking at the search results, all usages of get_connection
are aligned with the current implementation. The method is used correctly in:
- Test files (
test_cloud_sync.py
,test_cloud_sql_reads.py
) - Documentation examples in
sync_results.py
and__init__.py
- API utilities in
api_util.py
No references to any old implementation were found, and all calls follow the current signature of accepting a connection_id parameter. Looking good! 🎯
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential references to the old get_connection method
rg -l "get_connection" --type python
# Look for method calls on CloudWorkspace instances
ast-grep --pattern 'workspace.get_connection($$$)'
Length of output: 120
Script:
#!/bin/bash
# Search for potential references to get_connection method
rg "get_connection" -A 2 -B 2
# Look for method calls on workspace instances using a more general pattern
ast-grep --pattern 'workspace.get_connection'
# Also check for any test files that might use this method
fd "test.*\.py$" --exec grep -l "get_connection" {} \;
Length of output: 6412
airbyte/cloud/connectors.py (2)
75-80
: Update class name in repr method
The class name in the string representation is still "CloudCheckResult" but should be "CheckResult".
115-142
: Verify test coverage and error handling patterns
Let's ensure the new check
method has proper test coverage and follows existing error handling patterns.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
CloudWorkspace
class for retrieving connections, sources, and destinations.Bug Fixes
Tests