-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
automatically figure out ARM-related variables #11450
Conversation
@@ -1,74 +0,0 @@ | |||
# This file is exactly the same as docker-compose.build.yaml, except |
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.
This is not used anywhere as far as I can tell with rg --hidden docker-compose.build-m1.yaml
.
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! I like that we don't have to treat arm like a special case anymore that requires manually set env vars.
build.gradle
Outdated
def isArm64 = arch == "aarch64" || arch == "arm64" | ||
|
||
def buildPlatform = System.getenv('DOCKER_BUILD_PLATFORM') ?: isArm64 ? 'linux/arm64' : 'linux/amd64' | ||
def alpineImage = System.getenv('ALPINE_IMAGE') ?: isArm64 ? 'arm64v8/alpine:3.14' : 'alpine:3.14' |
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.
there is also an amd64/alpine:3.14 image. What do you think about using that for the amd64
arch builds instead of just alpine:3.14
?
I bring this up because the alpine
tag is shared by multiple images built for different architectures, so there is an edge case where you could have built an image for the arm64 arch using the arm version of alpine:3.14
as a base image, but then when you try to build for the amd64 arch next, you will try to use the arm version of alpine:3.14
as the base image to build amd64 images (because that is the one in your local machine's docker image cache). Using amd64/alpine
for amd64 should avoid this issue.
I get that if our users are only building this airbyte project this case wouldn't come up, but I think it could be very possible that users are building other images that simply use alpine
as a base image, and I think users would normally expect the alpine
image tag to refer to the version that matches the arch of their local machine.
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.
Good idea, I added these.
build.gradle
Outdated
|
||
def buildPlatform = System.getenv('DOCKER_BUILD_PLATFORM') ?: isArm64 ? 'linux/arm64' : 'linux/amd64' | ||
def alpineImage = System.getenv('ALPINE_IMAGE') ?: isArm64 ? 'arm64v8/alpine:3.14' : 'alpine:3.14' | ||
def postgresImage = System.getenv('POSTGRES_IMAGE') ?: isArm64 ? 'arm64v8/postgres:13-alpine' : 'postgres:13-alpine' |
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.
similar to above, there is amd64/postgres:13-alpine
Only the image checker build is failing and it's also failing on |
great work @jrhizor ! |
resolves #10255
I removed my local env variables related to M1 builds from my
.zshrc
except forSUB_BUILD
and ran./gradlew build -x
and also./gradlew :airbyte-integrations:connectors:destination-e2e-test:build -x test
on my M1 machine successfully.