-
Notifications
You must be signed in to change notification settings - Fork 23
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
Allow build on arm64 #3
base: main
Are you sure you want to change the base?
Conversation
* replace all `amd64` and `x86_64` references with $TARGETARCH (or computed values based off that) * re-enable certificate checking * unquiet wget invocations to allow debugging * unify download call usage
.devcontainer/Dockerfile
Outdated
ARG TERRAFORM_VERSION=1.3.6 | ||
RUN mkdir -p /tmp/download \ | ||
&& cd /tmp/download \ | ||
&& wget -O terraform.zip "https://releases.hashicorp.com/terraform/1.3.6/terraform_1.3.6_linux_${TARGETARCH}.zip" \ |
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.
Hi @DavidS-om , thank you for your PR.
The TERRAFORM_VERSION
is not being used here. Would be great to allow the user to pass it on as arguments.
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.
d'oh, yes!
@@ -7,6 +7,8 @@ | |||
ARG VARIANT="jammy" | |||
FROM mcr.microsoft.com/vscode/devcontainers/base:0-${VARIANT} | |||
|
|||
ARG TARGETARCH |
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.
If the user doesn't provide a value, how the build would respond?
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 set automatically by buildkit. I've added a comment to point this out
|
||
# AWS CLI | ||
SHELL ["/bin/zsh", "-c"] | ||
RUN mkdir -p /tmp/download \ | ||
&& cd /tmp/download \ | ||
&& curl "https://awscli.amazonaws.com/awscli-exe-linux-x86_64.zip" --silent -o "awscliv2.zip" \ | ||
&& ARCH=${TARGETARCH/amd64/x86_64} \ |
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.
Why provide ARCH
twice here?
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.
because of the awscli hosting URLs, this needs to replace amd64 => x86_64 and arm64 => aarch64. Since I didn't want to touch the TARGETARCH, this code is overwriting ARCH a second time to apply both replaces.
Thanks for your feedback! All good points. I hope I can provide an update early next week. |
Looks like I need to re-do the work on top of #6 to address the merge conflict |
🧠 Pull Request
Changes
amd64
andx86_64
references with $TARGETARCH (or computed values based off that)Type of change
Why
Fixes #2