airbyte-ci: multi arch build #32816
airbyte-ci: multi arch build #32816
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. |
alafanechere
force-pushed
the
augustin/11-24-airbyte-ci_multi_arch_build
branch
from
a388429
to
8e625c4
Compare
octavia-squidington-iii
added
area/connectors
| @@ -17,10 +20,19 @@ | |||
| default=False, | |||
| type=bool, | |||
| ) | |||
| @click.option( | |||
| "-a", | |||
| "--architecture", | |||
| @@ -53,6 +65,7 @@ async def build(ctx: click.Context, use_host_gradle_dist_tar: bool) -> bool: | |||
| ctx.obj["concurrency"], | |||
| ctx.obj["dagger_logs_path"], | |||
| ctx.obj["execute_timeout"], | |||
| build_platforms, | |||
There was a problem hiding this comment.
What if we added this to the context instead? Lets test and publish reuse it
There was a problem hiding this comment.
I don't think architecture should be customized for test and publish.
If we'd want to run multiarch tests I think it should be handled at the test level and not with a customizable input.
Same idea for publish, I think we should always build and publish for all our supported architectures.
There was a problem hiding this comment.
Fair opinion on this not being something we want customizable for publish/test
But I was thinking of moving this to the context so that the entry point for the information of "which platforms are we building for?"
So that if you learn in build that the archicture target comes from the context you can take that knowledge with you to publish and test
Its not too far away from how other configurations like docker hub crednetials, cdk_version, connector_acceptance_test_image and others are in the context.
There was a problem hiding this comment.
@bnchrch I'm not sure which context you mean exactly.
Are we talking about the ConnectorContext, the ctx.obj, or the ClickPipelineContext?
I assume it's the ConnectorContext and I'll add it there. I can change it in a follow up PR if I'm wrong about your suggestion 😄
| @@ -18,6 +18,10 @@ on: | |||
| type: string | |||
| default: ci-runner-connector-publish-large-dagger-0-6-4 | |||
| required: true | |||
| airbyte-ci-binary-url: | |||
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/build_image/commands.py
Outdated
Show resolved
Hide resolved
|
Seems really close! Just wanted to table the discussion on if we should hoist build_platforms to the context level |
alafanechere
force-pushed
the
augustin/11-24-airbyte-ci_multi_arch_build
branch
2 times, most recently
from
b9866b0
to
619d58a
Compare
|
@bnchrch I added a couple of tests
I'd prefer to not propagate |
There was a problem hiding this comment.
Going to preapprove since your on long before I am!
But I left some follow up on the context idea.
The context object being a configuration object is something we already do and would likely go a long way in establishing common behaviours devs can take with them around our codebase.
We can still hardcode the build platforms for test/publish. It would just happen when we instantiate the context class.
alafanechere
force-pushed
the
augustin/11-24-airbyte-ci_multi_arch_build
branch
from
6ddff0c
to
bac3e2f
Compare
octavia-squidington-iii
added
area/connectors
alafanechere
force-pushed
the
augustin/11-24-airbyte-ci_multi_arch_build
branch
from
7054179
to
f64a96a
Compare
alafanechere
force-pushed
the
augustin/11-24-airbyte-ci_multi_arch_build
branch
from
f64a96a
to
7c8a6af
Compare
What
We want to add a
--architectureoption to theconnectors buildcommand.This will help users decide if they want to build single arch image are multi arch image locally, which has a significant execution time difference.
How
--architectureoption to `connectors build commandStepsto handle container variants if needed.🚨 User Impact 🚨
Manual testing:
* Pre-release publish still works