-
Notifications
You must be signed in to change notification settings - Fork 93
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
build(docker): Build an image for sentry CI #1658
Conversation
ARG DOCKER_ARCH=amd64 | ||
|
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.
I deleted all the ARCH
stuff here to make the dockerfile easier to build -- it now depend on the --platform
argument to docker build
instead of specifying the architecture as an --arg
60c6fc9
to
d14c031
Compare
@olksdr or I will take a closer look and re-approve afterwards.
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 looks good.
@asottile-sentry could you, please, merge master
back and resolve the conflicts, so we could give another look and finally merge it in.
@@ -15,11 +15,11 @@ BUILD_IMAGE="us.gcr.io/sentryio/relay:deps" | |||
|
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.
We should change the BUILD_IMAGE
:
- Use the GHCR or DockerHub instead of GCR
- Use caches, but also build that image as part of this script
@@ -30,6 +30,17 @@ build-linux-release: setup-git ## build linux release of the relay | |||
objcopy --add-gnu-debuglink target/${TARGET}/release/relay{.debug,} | |||
.PHONY: build-linux-release | |||
|
|||
collect-source-bundle: setup-git ## copy the built relay binary to current folder and collects debug bundles |
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.
Let's move this to a script. This allows us to:
- Call this more naturally from other scripts
- Use it in gocd deployments
- Publish it to GH releases
.github/workflows/ci.yml
Outdated
- name: ghcr.io login | ||
run: docker login --username '${{ github.actor }}' --password '${{ secrets.GITHUB_TOKEN }}' ghcr.io | ||
|
||
- name: build-deps |
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.
Idea: These steps could be moved to a script that we can also use locally to:
- Build / update the builder image
- Build the binary to a local folder (should we introduce a
build/
directory?) - Create the release docker container w/ the binary inside
In a close follow-up, we can then replace docker-build-linux.sh
in the build_binary
workflow with our new script.
69c8220
to
b58f526
Compare
After merging, we need to
"checks": [
{
"type": "github-apps",
"config": {
"repo": "getsentry/relay",
"contexts": [
"Integration Tests",
"Test (macos-latest)",
"Test (windows-latest)",
"Test All Features (ubuntu-latest)",
"Push GCR Docker Image"
]
}
}
] |
scripts/build-docker-image.sh
Outdated
# Set the correct build target and update the arch if required. | ||
if [[ "$ARCH" = "amd64" ]]; then | ||
BUILD_TARGET="x86_64-unknown-linux-gnu" | ||
elif [[ "$ARCH" = "arm64" ]]; then | ||
BUILD_TARGET="aarch64-unknown-linux-gnu" | ||
elif [[ "$ARCH" = "aarch64" ]]; then | ||
BUILD_TARGET="aarch64-unknown-linux-gnu" | ||
ARCH="arm64" | ||
fi |
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.
Let's add an else
branch that errors if no supported architecture is set, for instance:
# Set the correct build target and update the arch if required. | |
if [[ "$ARCH" = "amd64" ]]; then | |
BUILD_TARGET="x86_64-unknown-linux-gnu" | |
elif [[ "$ARCH" = "arm64" ]]; then | |
BUILD_TARGET="aarch64-unknown-linux-gnu" | |
elif [[ "$ARCH" = "aarch64" ]]; then | |
BUILD_TARGET="aarch64-unknown-linux-gnu" | |
ARCH="arm64" | |
fi | |
if [[ "$ARCH" == "aarch64 ]]; then | |
ARCH="arm64" | |
fi | |
if [[ "$ARCH" = "amd64" ]]; then | |
BUILD_TARGET="x86_64-unknown-linux-gnu" | |
elif [[ "$ARCH" = "arm64" ]]; then | |
BUILD_TARGET="aarch64-unknown-linux-gnu" | |
else | |
echo "ERROR Unsupported architecture" | |
exit 1 | |
fi |
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.
@jan-auer changed to switch this one
IMG_DEPS: ghcr.io/getsentry/relay-deps:${{ matrix.arch }} | ||
# GITHUB_SHA in pull requests points to the merge commit | ||
IMG_VERSIONED: ghcr.io/getsentry/relay:${{ github.event.pull_request.head.sha || github.sha }} | ||
ARCH: ${{ matrix.arch }} |
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.
at a glance this looks unused, but it's actually used inside build-docker-image
-- I would probably make it an argument to the script instead
- name: Push to ghcr.io | ||
run: | | ||
set -euxo pipefail | ||
docker login --username '${{ github.actor }}' --password '${{ secrets.GITHUB_TOKEN }}' ghcr.io |
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 will fail for external contributors -- you'll probably want to gate this somehow on that
llvm-toolset-7.0-clang-devel \ | ||
&& yum clean all \ | ||
&& rm -rf /var/cache/yum \ | ||
&& ln -s /usr/bin/cmake3 /usr/bin/cmake |
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.
typically /usr/bin
is managed by the package manager, this should be a symlink to /usr/local/bin/cmake
probably
ENV \ | ||
RELAY_UID=10001 \ | ||
RELAY_GID=10001 | ||
|
||
# Create a new user and group with fixed uid/gid | ||
RUN groupadd --system relay --gid $RELAY_GID \ | ||
&& useradd --system --gid relay --uid $RELAY_UID relay | ||
|
||
RUN mkdir /work /etc/relay \ | ||
&& chown relay:relay /work /etc/relay |
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 necessary for this PR, but this is much easier to do with USER
(and then not needing gosu
at all)
IMG_VERSIONED=${IMG_VERSIONED:-"relay:latest"} | ||
|
||
# Build a builder image with all the depdendencies. | ||
args=(--progress auto) |
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 be necessary?
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 fails locally for me, when the args
is empty
./scripts/build-docker-image.sh: line 32: args[@]: unbound variable
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.
ah, ancient macos bash -- you might be able to use declare -a args
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.
or put the real args into the array
scripts/build-docker-image.sh
Outdated
|
||
# Build the binary inside of the builder image. | ||
docker run \ | ||
-v "$(pwd):/work" \ |
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.
$(pwd)
=> $PWD
(var lookup is free compared to a subshell and subprocess)
I also like to be explicit and use --volume
and state whether it's :ro
or :rw
(I don't even remember the default)
scripts/build-docker-image.sh
Outdated
# Fix permissions for shared directories | ||
USER_ID=$(id -u) | ||
GROUP_ID=$(id -g) | ||
sudo chown -R "${USER_ID}:${GROUP_ID}" target/ |
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.
better: run the container with --user
and then you don't need to do this. or utilize docker cp
… into asottile-relay-multiarch
we're moving sentry's CI from depending on both gcr.io and ghcr.io to only ghcr.io
#skip-changelog