Skip to content
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

Merged
merged 23 commits into from
Dec 17, 2024

Conversation

aaronsteers
Copy link
Contributor

@aaronsteers aaronsteers commented Dec 16, 2024

Summary by CodeRabbit

  • New Features

    • Added new constants for API roots to enhance API functionality.
    • Introduced methods for obtaining bearer tokens and checking connector statuses.
    • Added new methods in the CloudWorkspace class for retrieving connections, sources, and destinations.
    • Added a new data class to encapsulate connector check results.
  • Bug Fixes

    • Improved error handling in connector check functionality.
  • Tests

    • Added new integration tests for bearer token retrieval and connector checks, enhancing test coverage and error reporting.

…er-image-for-connectors
@aaronsteers aaronsteers changed the title Feat(cloud): Add connector check Feat(cloud): Add check feature for Cloud connectors Dec 16, 2024
Copy link

coderabbitai bot commented Dec 16, 2024

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 7378bf4 and db63aca.

📒 Files selected for processing (3)
  • airbyte/_util/api_util.py (9 hunks)
  • airbyte/cloud/__init__.py (2 hunks)
  • airbyte/cloud/connectors.py (2 hunks)
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces enhancements to the Airbyte API utility functions, focusing on cloud configuration and connector management. New constants and methods are added to api_util.py to support bearer token retrieval, configuration API interactions, and connector checking. A corresponding CheckResult data class is created in connectors.py to provide structured feedback on connector checks. The changes are complemented by new integration tests that validate the newly added functionality, ensuring robust error handling and API interaction capabilities.

Changes

File Changes
airbyte/_util/api_util.py - Added constants CLOUD_CONFIG_API_ROOT and CLOUD_CONFIG_API_TEST_ROOT
- Added methods: get_config_api_root(), get_bearer_token(), _make_config_api_request(), check_connector()
airbyte/cloud/connectors.py - Added CheckResult data class
- Added check() method to CloudConnector class
tests/integration_tests/cloud/test_cloud_api_util.py - Added test_get_bearer_token() integration test
- Added test_check_connector() parameterized integration test

Sequence Diagram

Loading
sequenceDiagram
    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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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 standard dataclasses module for the @dataclass decorator

Currently, dataclass is imported from attr, but the standard library's dataclasses module provides a built-in @dataclass decorator. Would it be better to import from dataclasses for consistency and to avoid external dependencies? Wdyt?

airbyte/_util/api_util.py (1)

17-17: Remove unused import dataclasses.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 unused

Remove unused import: dataclasses.dataclass

(F401)

tests/integration_tests/cloud/test_cloud_api_util.py (1)

265-265: Reconsider the commented-out test case in test_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

📥 Commits

Reviewing files that changed from the base of the PR and between 7e65ab3 and 337e641.

📒 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)

airbyte/cloud/connectors.py Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a 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 🌟

  1. 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.
"""
  1. 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)
  1. For consistency, should we align the parameter names? connection_id vs connector_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 and internal_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 include internal_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:

  1. 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}")
  1. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 337e641 and 7378bf4.

📒 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.

aaronsteers and others added 2 commits December 16, 2024 15:46
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@aaronsteers aaronsteers marked this pull request as ready for review December 16, 2024 23:49
@aaronsteers aaronsteers enabled auto-merge (squash) December 16, 2024 23:50
@aaronsteers aaronsteers merged commit ef7eb8f into main Dec 17, 2024
18 checks passed
@aaronsteers aaronsteers deleted the aj/feat/add-cloud-connector-check branch December 17, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant