feat(low-code): add check dynamic stream #223
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a Changes
Possibly related PRs
Suggested Reviewers
Hey there! 👋 I noticed you've added a really cool dynamic stream checking feature. Would you be open to discussing how this might impact existing declarative source configurations? Wdyt? The new Finishing Touches
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 (
|
![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 (2)
airbyte_cdk/sources/declarative/checks/check_dynamic_stream.py (2)
21-22: Minor correction in docstringWould you consider updating the docstring to change "numbers of streams to check" to "number of streams to check"? Wdyt?
37-50: InstantiateHttpAvailabilityStrategyoutside the loopTo improve efficiency, how about instantiating the
HttpAvailabilityStrategybefore the loop? This way, we avoid creating a new instance in each iteration. Wdyt?You could apply this diff:
def check_connection( self, source: AbstractSource, logger: logging.Logger, config: Mapping[str, Any] ) -> Tuple[bool, Any]: streams = source.streams(config=config) if len(streams) == 0: return False, f"No streams to connect to from source {source}" + availability_strategy = HttpAvailabilityStrategy() for stream_index in range(min(self.stream_count, len(streams))): stream = streams[stream_index] - availability_strategy = HttpAvailabilityStrategy() try: stream_is_available, reason = availability_strategy.check_availability( stream, logger ) if not stream_is_available: return False, reason except Exception as error: logger.error( f"Encountered an error trying to connect to stream {stream.name}. Error: \n {traceback.format_exc()}" ) return False, f"Unable to connect to stream {stream.name} - {error}" return True, None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
airbyte_cdk/sources/declarative/checks/__init__.py(1 hunks)airbyte_cdk/sources/declarative/checks/check_dynamic_stream.py(1 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(3 hunks)airbyte_cdk/sources/declarative/manifest_declarative_source.py(2 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(3 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)airbyte_cdk/sources/declarative/requesters/README.md(2 hunks)unit_tests/sources/declarative/checks/test_check_dynamic_stream.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- airbyte_cdk/sources/declarative/requesters/README.md
👮 Files not reviewed due to content moderation or server errors (4)
- airbyte_cdk/sources/declarative/manifest_declarative_source.py
- airbyte_cdk/sources/declarative/models/declarative_component_schema.py
- airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
- airbyte_cdk/sources/declarative/declarative_component_schema.yaml
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
[warning] 5-455: Import block is un-sorted or un-formatted
airbyte_cdk/sources/declarative/manifest_declarative_source.py
[warning] 5-56: Import block is un-sorted or un-formatted
unit_tests/sources/declarative/checks/test_check_dynamic_stream.py
[warning] 5-14: Import block is un-sorted or un-formatted
airbyte_cdk/sources/declarative/checks/__init__.py
[warning] 5-21: Import block is un-sorted or un-formatted
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/checks/__init__.py (1)
5-21: Address linter warning on import formattingThere's a linter warning indicating that the import block is un-sorted or un-formatted. Maybe we can sort and format the imports to comply with the code style guidelines? Wdyt?
🧰 Tools
🪛 GitHub Actions: Linters
[warning] 5-21: Import block is un-sorted or un-formatted
unit_tests/sources/declarative/checks/test_check_dynamic_stream.py (1)
5-14: Address linter warning on import formattingThere's a linter warning about the imports being un-sorted or un-formatted. Perhaps we could sort and format the import statements to fix this? Wdyt?
🧰 Tools
🪛 GitHub Actions: Linters
[warning] 5-14: Import block is un-sorted or un-formatted
|
/autofix
|
lazebnyi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
847-852: LGTM! Would you consider adding docstring for better maintainability?The implementation looks good and follows the established patterns. For consistency with other create methods, would you consider adding a docstring to document the purpose and parameters? wdyt?
Here's a suggested docstring:
@staticmethod def create_check_dynamic_stream( model: CheckDynamicStreamModel, config: Config, **kwargs: Any ) -> CheckDynamicStream: + """Creates a CheckDynamicStream component for checking dynamic streams. + + Args: + model: The model containing the configuration for the dynamic stream checker + config: The connector configuration + **kwargs: Additional keyword arguments + + Returns: + A CheckDynamicStream instance configured with the specified stream count + """ return CheckDynamicStream(stream_count=model.stream_count, parameters={})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/checks/__init__.py(1 hunks)airbyte_cdk/sources/declarative/manifest_declarative_source.py(2 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)unit_tests/sources/declarative/checks/test_check_dynamic_stream.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- airbyte_cdk/sources/declarative/manifest_declarative_source.py
- unit_tests/sources/declarative/checks/test_check_dynamic_stream.py
- airbyte_cdk/sources/declarative/checks/init.py
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
57-57: LGTM!The import statement is correctly placed in the imports section and follows the alphabetical order.
124-126: LGTM!The import statement for the model is correctly placed and follows the same pattern as other model imports.
lazebnyi
changed the title
…tehq/airbyte-python-cdk into lazebnyi/add-check-dynamic-streams
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
308-322: The schema looks good! Would you consider enhancing the documentation?The implementation is solid, but the documentation could be more helpful for users. What do you think about:
- Adding examples of when to use
CheckDynamicStreamvsCheckStream?- Clarifying in the
stream_countdescription how this number is used (e.g., "Specifies how many dynamic streams to sample during the check operation")?- Adding example values for
stream_countto guide users?airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
586-588: Consider adding more diverse examples for scopes, wdyt?The current example shows only CRM-related scopes. Consider adding examples from different domains to better illustrate the variety of possible scope combinations.
examples=[ - ["crm.list.read", "crm.objects.contacts.read", "crm.schema.contacts.read"] + ["crm.list.read", "crm.objects.contacts.read"], + ["analytics.read", "users.write", "files.all"], + ["read:users", "write:posts", "admin"] ],🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting. The code does not conform to the project's formatting standards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(12 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/models/declarative_component_schema.py
[warning] File requires formatting. The code does not conform to the project's formatting standards.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (7)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
21-23: LGTM! Clean implementation of multiple check types.The schema change elegantly supports both static and dynamic stream checks while maintaining backward compatibility.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (3)
55-62: LGTM! Clean implementation of CheckDynamicStream.The implementation is simple, focused, and well-documented. The
stream_countfield has clear description and title.🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting. The code does not conform to the project's formatting standards.
1670-1670: LGTM! Good enhancement to support dynamic stream checking.The update to both
DeclarativeSource1andDeclarativeSource2to supportCheckDynamicStreamis backward compatible and maintains consistency between the two versions.Also applies to: 1696-1696
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting. The code does not conform to the project's formatting standards.
992-1006: LGTM! Enhanced type safety with explicit type hints.The addition of type hints improves code maintainability and helps catch potential type-related issues early. The formatting is consistent across all updated sections.
Also applies to: 1615-1617, 2058-2062, 2105-2107, 2141-2145, 2209-2214
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting. The code does not conform to the project's formatting standards.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
57-57: LGTM! Clean import addition.The import of
CheckDynamicStreammaintains alphabetical order and follows the project's import style.
499-499: LGTM! Proper factory mapping.The addition of
CheckDynamicStreamModeltoPYDANTIC_MODEL_TO_CONSTRUCTORmaintains alphabetical order and follows the existing pattern.
853-858: LGTM! Well-structured implementation.The
create_check_dynamic_streammethod follows the same pattern ascreate_check_stream, is properly decorated as a static method, and has a clean implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
55-62: LGTM! Clean implementation of the newCheckDynamicStreamclass.The class is well-structured with proper field descriptions and types. The stream_count field is appropriately marked as required with clear documentation.
Quick question: Would it be helpful to add a default or example value for the stream_count field to guide users? For example:
examples=[1, 5]. wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
1662-1662: LGTM! Good update to support both static and dynamic stream checks.The change to make
checkfield support bothCheckStreamandCheckDynamicStreamaligns well with the PR objective of enabling connection checks for dynamic streams.
1688-1688: LGTM! Consistent implementation across both source types.The same change is correctly applied to
DeclarativeSource2, maintaining consistency in the schema.
What
The current implementation can only check connections for static streams. If a source implements only dynamic streams, this check is bypassed.
Fixed: https://github.com/airbytehq/airbyte-internal-issues/issues/11371
How
Added CheckDynamicStream to the CDK to natively check connections for cases where the source implements only dynamic streams.
Summary by CodeRabbit
New Features
CheckDynamicStreamto verify connectivity of dynamic streams.Improvements
Documentation
Tests
CheckDynamicStreamfunctionality, covering various HTTP response scenarios.