Fix: Updated handling for Manifest-only connectors; Breaks: Remove Python 3.9 support#340
Conversation
WalkthroughWalkthroughThe changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeclarativeExecutor
participant ManifestValidator
participant SourceManifestRetriever
User->>DeclarativeExecutor: Instantiate with name and manifest
DeclarativeExecutor->>ManifestValidator: Validate manifest structure
alt Validation successful
ManifestValidator-->>DeclarativeExecutor: Validated
else Validation error
ManifestValidator-->>DeclarativeExecutor: Raise AirbyteConnectorInstallationError
end
DeclarativeExecutor->>SourceManifestRetriever: Retrieve source manifest
SourceManifestRetriever-->>DeclarativeExecutor: Return manifest
Would you like to include any specific examples in the summaries, or do you think the current level of detail is sufficient? wdyt? Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (1)
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 as PR comments)
Additionally, you can add 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
Outside diff range, codebase verification and nitpick comments (1)
airbyte/_executors/declarative.py (1)
12-12: Consider converting JSON manifests to YAMLIt seems there are JSON manifest files present in the repository, specifically in the
tests/integration_tests/fixturesdirectory. Since you've switched to using YAML for manifest configuration, have you considered converting these JSON files to YAML to ensure compatibility? Additionally, it might be worth checking if any scripts or tools need updates to handle YAML instead of JSON. Wdyt?
tests/integration_tests/fixtures/valid_config.jsontests/integration_tests/fixtures/invalid_config.jsontests/integration_tests/fixtures/registry.jsonAnalysis chain
Consider the implications of switching to YAML.
You've switched from JSON to YAML for manifest configuration, which enhances flexibility. Have you ensured that all existing manifests are compatible with YAML, and have you considered the potential need for migration or conversion scripts? Wdyt?
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if there are any JSON manifest files that might need conversion to YAML. # Test: Search for JSON manifest files. Expect: No JSON manifest files. fd --type f --extension json --exec echo "JSON manifest found: {}"Length of output: 288
Aaron ("AJ") Steers (aaronsteers)
changed the title
Aaron ("AJ") Steers (aaronsteers)
changed the title
Aaron ("AJ") Steers (aaronsteers)
deleted the
chore/manifest-only-connector-language
branch
Maxime Carbonneau-Leclerc (maxi297)
left a comment
Maxime Carbonneau-Leclerc (maxi297)
left a comment
There was a problem hiding this comment.
More a review for me to learn PyAirbyte than for confirming the change
| def _validate_manifest(self, manifest_dict: dict) -> None: | ||
| """Validate the manifest.""" | ||
| manifest_text = json.dumps(manifest_dict) | ||
| if "class_name:" in manifest_text: |
There was a problem hiding this comment.
My understanding is that this means we don't support any connector with custom components. I think this is fair but there were some talks a while back to support custom components in the Connector Builder. I don't know where those talks are at today but it could be interesting for the marketplace team to know that such change will have an impact on PyAirbyte
| # Source manifest is either a URL or a boolean (True) | ||
| source_manifest = _try_get_source_manifest( | ||
| source_name=name, | ||
| manifest_url=None if source_manifest is True else source_manifest, |
There was a problem hiding this comment.
I'm trying to figure out a bit more about PyAirbyte ; in which case get_connector_executor is called with source_manifest being a URL?
2 participants
This fixes the language check to include the new
'manifest-only'language type, and it updates PyAirbyte to locate manifest yaml in their new directory.This PR also requires dropping Python 3.9 support, as we've previously discussed in Slack. This is necessary in order to bump to the latest CDK version, which is necessary to install the latest manifest-only connectors.
Notes:
source_manifestconnectors, so no rename is needed. "Low-code" and "manifest-only" are both the same category from the PyAirbyte user perspective.manifest.ymlandpython_components.zipto GCS connector registry airbyte#42584source-fakerwas previously a dev dependency, which created conflicts when trying to bump the CDK. This PR removessource-fakeras a dev dependency and tests that rely on it will now auto-install the connector to complete their tests. This makes the diff much messier, but it is required in order to bump the CDK.Summary by CodeRabbit
New Features
Improvements
Updates