-
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
Source Faker: Add support for PyPi and AirbyteLib entrypoints #34033
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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,
|
@@ -1,6 +1,6 @@ | |||
// For format details, see https://aka.ms/devcontainer.json. For config options, see the | |||
{ | |||
"name": "Connector Development DevContainer (Generic)", | |||
"name": "Java Development DevContainer (Generic)", |
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.
Updated the name to distinguish the generic Java devcontainer from the Python one.
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.
The source-faker
changes LGTM.
I raised a couple of concerns / suggestion about the devcontainer
configuration.
I'm not using devcontainer
myself so my suggestion might be unrealistic / not matching what you like to achieve.
Please engage in a more in-depth discussion if you'd like to broaden the adoption of devcontainer.json
among our connector developers. My main concern is to find a way to guarantee a certain level of consistency between the dev environment (managed by devcontainers.json
) and the build environment (managed by airbyte-ci
) so that developer do not face late CI failures / bad surprises on release.
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.
Can we use python connector base image as a development container for connectors?
The problem we might have is that:
- Python connectors base image is based on Python 3.9.18
- airbyte-ci (dagger) requires Python 3.10
So it's kinda hard to have a unified dev container which can match connector runtime with airbyte-ci
installed.
We should ideally have a devcontainer with multiple python version installed, 3.9 for connectors, 3.10 for airbyte-ci.
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.
@alafanechere - I tested successfully and was able to run airbyte-ci
without issue, thanks to the make
option.
Happy to keep iterating on this. For now, it gives a baseline experience that's much better than starting with a blank of generic container.
"initializeCommand": "git config --add safe.directory /workspaces/airbyte", | ||
|
||
// Setup airbyte-ci on the container: | ||
"postCreateCommand": "make tools.airbyte-ci-dev.install", |
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.
Is airbyte-ci
correctly working inside the container?
If it does I think it's thanks to the docker-in-docker
feature.
The problem with this approach is that the dagger engine in use will be using your devcontainer
docker / buildkit engine, and not your host one.
This can impact performance and caching.
To not use docker-in-docker you can try to:
- Mount the docker socket to the dev container so that it can interact with your host docker engine
- Set the
_EXPERIMENTAL_DAGGER_RUNNER_HOST
env var to tell the dagger engine to use this mounted socket. (example in CI here)
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.
It works fine as-is for now. In the cloud, I think docker-in-docker is correct, and probably the best we can get. There's also a 'docker-from-docker' option that might work better locally - but I don't have familiar with that as of yet, and so I'd like to keep as is for now.
Please engage in a more in-depth discussion if you'd like to broaden the adoption of devcontainer.json among our connector developers.
Agreed. No need to push this as of now. I think over time we will get to a good place where we have collective confidence that the containers give a solid and consistent experience.
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.
Approving for faker changes, @alafanechere s comments on dev container make sense to me
This adds compatibility with AirbyteLib.
Includes PyPi declaration as defined in:
This PR replaces the related changes in:
source-faker
#33878This PR also includes a generic Python DevContainer, which can be used for testing in Codespaces. I'm happy to remove it though and move to a different PR if anyone has an objection. I confirmed I was able to run pytest from this decontainer, and that all tests are passing.