Skip to content
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

Merged
merged 4 commits into from
Mar 31, 2022

Conversation

jrhizor
Copy link
Contributor

@jrhizor jrhizor commented Mar 26, 2022

resolves #10255

I removed my local env variables related to M1 builds from my .zshrc except for SUB_BUILD and ran ./gradlew build -x and also ./gradlew :airbyte-integrations:connectors:destination-e2e-test:build -x test on my M1 machine successfully.

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Mar 26, 2022
@@ -1,74 +0,0 @@
# This file is exactly the same as docker-compose.build.yaml, except
Copy link
Contributor Author

@jrhizor jrhizor Mar 26, 2022

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.

Copy link
Contributor

@git-phu git-phu left a 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'
Copy link
Contributor

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.

Copy link
Contributor Author

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'
Copy link
Contributor

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

@jrhizor jrhizor temporarily deployed to more-secrets March 31, 2022 20:26 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets March 31, 2022 20:26 Inactive
@jrhizor
Copy link
Contributor Author

jrhizor commented Mar 31, 2022

Only the image checker build is failing and it's also failing on master, so I'm going to merge this.

@jrhizor jrhizor merged commit 526c5a0 into master Mar 31, 2022
@jrhizor jrhizor deleted the jrhizor/automatically-figure-out-arm-vars branch March 31, 2022 20:58
@sherifnada
Copy link
Contributor

great work @jrhizor !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

automatically detect architecture for gradle builds
4 participants