-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add CICD test for orchestrator using Dagger #24816
Conversation
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.
Nice! My main concern is around file system access / mount, I suggest you use the get_repo_dir
method of the context object if you need to mount directories to containers.
tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py
Show resolved
Hide resolved
tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py
Outdated
Show resolved
Hide resolved
tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py
Outdated
Show resolved
Hide resolved
tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py
Outdated
Show resolved
Hide resolved
tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py
Outdated
Show resolved
Hide resolved
tools/ci_connector_ops/ci_connector_ops/pipelines/commands/groups/metadata.py
Outdated
Show resolved
Hide resolved
2c3210d
to
dfdde53
Compare
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.
LGTM.
Please consider my suggestion about the with_poetry_module
parameters.
I agree that we should favor the use of debian base images, but please keep also what've been on top of alpine so far. My work on this PR has a lot of moving pieces that I think I succeeded in keeping stable and I'd prefer to not change the base images I used so far. I'll create an issue to do it later.
@@ -33,7 +33,7 @@ runs: | |||
python-version: "3.10" | |||
- name: Install ci-connector-ops package | |||
shell: bash | |||
run: pip install ./tools/ci_connector_ops\[pipelines]\ | |||
run: pip install -e ./tools/ci_connector_ops\[pipelines]\ |
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.
Do we really need to enable the editable
mode while running in a CI context?
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.
Without -e
we were getting this failure.
https://github.com/airbytehq/airbyte/actions/runs/4632791811/jobs/8197275855?pr=24816
Seems like some weird caching behaviour with the hostedtools cache.
As the version 0.2.1
is most recent but its certainly trying to reference code from <0.2.0
When looking around the code base though it seems like in the majority of actions where we install tools/ci_*
we are using the -e
flag.
I imagine for this reason.
tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py
Show resolved
Hide resolved
tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py
Show resolved
Hide resolved
tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py
Outdated
Show resolved
Hide resolved
tools/ci_connector_ops/ci_connector_ops/pipelines/commands/groups/metadata.py
Outdated
Show resolved
Hide resolved
debefef
to
7d5b979
Compare
What
We need to run orchestrator unit tests in CICD
closes #24209
How
This adds that using a dagger pipeline
Notes for reviewer
Not using an alpine image
Unfortunately dagster using the grpcio library which currently doesnt have "wheel" binaries for alpine. So to avoid 10 minute plus recompile times I changed to use a debian base package
Interesting reading:
https://medium.com/swlh/alpine-slim-stretch-buster-jessie-bullseye-bookworm-what-are-the-differences-in-docker-62171ed4531d
grpc/grpc#22815
grpc/grpc#24722
Poetry sibling modules
I updated with_poetry to let you copy a parent working directory of the poetry module to allow for relative imports