-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
airbyte-lib: Refactor connectors #34552
airbyte-lib: Refactor connectors #34552
Conversation
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
For my part, this looks great. Thanks, @flash1293 !
I have reviewed the migration scripts and scanned the updated connector code changes. This looks 'safe' to me, and I appreciate the broader approach to including package_data. We might find something is still missed, but this should catch everything that is foreseeable. And if something is missed, it'll fail the validation tests later when we next attempt to publish.
I noted this on other threads, but I'll note here too:
- One nice advantage of having all the connectors' CLI entrypoints registered, is that we gain new test capabilities via git-based or local path-based installs. This will help us find bugs and test functionality in a variety of use cases before needing to formally publish. (Without a CLI entrypoint, this is just a lot harder to do.)
/approve-and-merge reason="We collectively decided that CI can be bypassed for this global change. No publish should be triggered" |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This PR refactors most connectors so they are compatible with airbyte-lib:
run
functionPublish plan
This PR is intended to merged without CI checks, as a lot of the touched connectors currently have broken tests. As the docker image tag won't be bumped, no new version will be published, reducing the risk of this change.
The next developer working on the connector will implicitly publish these changes, which shouldn't have any effect on functionality.
How to test
airbyte-lib comes with a helper to validate that a connector works in general:
ci_credentials
as explained here: https://github.com/airbytehq/airbyte/blob/master/airbyte-ci/connectors/ci_credentials/README.mdairbyte-lib
and runpoetry shell
VERSION=dev ci_credentials source-<connector under test> write-to-storage
airbyte-lib-validate-source --connector-dir ./airbyte-integrations/connectors/source-<connector under test>/ --sample-config ./airbyte-integrations/connectors/source-<connector under test>/secrets/config.json
Converted connectors:
Click to expand list
Skipped connectors (already converted):
Click to expand list
Skipped connectors (non-python):
Click to expand list
Skipped directories (main.py not found, language:python/low-code):
source-zendesk-chat/
Script used to do the conversion
convert.sh