-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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-ci: upgrade to dagger 0.9.5 #33582
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
282778e
to
40df54a
Compare
2015dcb
to
bfdd6de
Compare
Warning Soft code freeze is in effect until 2024-01-02. Please avoid merging to master. #freedom-and-responsibility |
402811b
to
4c8a6a4
Compare
08c193c
to
cc757ef
Compare
cc757ef
to
8de621f
Compare
4e586dc
to
83c228e
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.
Does this mean we can follow this up with a PR which removes the dirty hack I put in that installs some secrets as regular env vars with --is-local
is set so that the gradle integration test output wouldn't be all messed up?
@@ -10,7 +10,7 @@ authors = ["Airbyte <contact@airbyte.io>"] | |||
|
|||
[tool.poetry.dependencies] | |||
python = "~3.10" | |||
dagger-io = "^0.6.4" | |||
dagger-io = "==0.9.5" |
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.
Shouldn't you bump the major version counter of airbyte-ci as well?
if docker_hub_username_secret and docker_hub_password_secret: | ||
# Docker login happens late because there's a cache buster in the docker login command. | ||
dockerd_container = docker_login(dockerd_container, docker_hub_username_secret, docker_hub_password_secret) | ||
return dockerd_container.with_exec( | ||
["dockerd", "--log-level=error", f"--host=tcp://0.0.0.0:{DOCKER_HOST_PORT}", "--tls=false"], insecure_root_capabilities=True | ||
) | ||
).as_service() |
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.
neat
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.
A few very small things but LGTM!
@@ -142,14 +145,14 @@ runs: | |||
export _EXPERIMENTAL_DAGGER_RUNNER_HOST="unix:///var/run/buildkit/buildkitd.sock" | |||
airbyte-ci --disable-dagger-run --is-ci --gha-workflow-run-id=${{ github.run_id }} ${{ inputs.subcommand }} ${{ inputs.options }} | |||
env: | |||
_EXPERIMENTAL_DAGGER_CLOUD_TOKEN: "p.eyJ1IjogIjFiZjEwMmRjLWYyZmQtNDVhNi1iNzM1LTgxNzI1NGFkZDU2ZiIsICJpZCI6ICJlNjk3YzZiYy0yMDhiLTRlMTktODBjZC0yNjIyNGI3ZDBjMDEifQ.hT6eMOYt3KZgNoVGNYI3_v4CC-s19z8uQsBkGrBhU3k" |
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 to see this go to an input!
@@ -94,11 +94,6 @@ async def run_connectors_pipelines( | |||
default_connectors_semaphore = anyio.Semaphore(concurrency) | |||
dagger_logs_output = sys.stderr if not dagger_logs_path else create_and_open_file(dagger_logs_path) | |||
async with dagger.Connection(Config(log_output=dagger_logs_output, execute_timeout=execute_timeout)) as dagger_client: | |||
|
|||
# HACK: This is to get a long running dockerd service to be shared across all the connectors pipelines | |||
# Using the "normal" service binding leads to restart of dockerd during pipeline run that can cause corrupted docker state |
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.
😍
) | ||
# When the connectors pipelines are done, we can stop the dockerd service | ||
tg_main.cancel_scope.cancel() | ||
await dockerd_service.start() |
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.
💅 nit: Should/could we turn this to an async with
async with dockerd_service.start():
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'd be nice but I don't think we can as it's not returning a context manager
from pipelines.airbyte_ci.connectors.context import ConnectorContext | ||
|
||
|
||
def get_sdk_version() -> str: |
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.
Not immediately clear what SDK its refering to when you call it.
def get_sdk_version() -> str: | |
def get_dagger_sdk_version() -> str: |
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.
Done
c62d4fe
to
d0a4b5c
Compare
@postamar I think it's worth a try, if my not mistaken the issues you opened on Dagger repo are closed now . |
What
Closes #33429
How
airbyte-ci
,base-images
,CAT
Manual testing
(manual testing should be done using the dev airbyte ci binary - this is why I used [skip ci] on all commits)
connectors --name=source-postgres --name=source-faker test
connectors --name=source-pokeapi publish --pre-release
format check all
format fix all
(local)connectors --name=source-faker bump_version
(local)airbyte-ci test
🚨 User Impact 🚨
None, if all manual tests succeed
Dependencies:
magicache
set up is complete on the infra side.Update: Distributed caching is currently enabled on our runners: waiting for Dagger team to confirm it's working correctly.
2 This should not be merged until CI can run on current branche's binary
Depends on #33519
Related infra PRs