-
Notifications
You must be signed in to change notification settings - Fork 11
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
Builds and pushes lh-standalone image for x86 and ARM platforms and fixes dashboard #615
Conversation
This reverts commit 2585a8b.
@@ -72,8 +72,12 @@ jobs: | |||
npm install pnpm --global | |||
pnpm install | |||
pnpm build | |||
- name: Tests | |||
run: ./gradlew server:test | |||
- name: Build & Test Server |
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.
Building the jar as part of the pipeline drastically reduces the amount of time the docker build
takes for the linux/arm64
platform. Also, this same jar could be leverage for the lh-sever
image instead of building it there too.
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 seems like a good idea, I like it.
run: ./gradlew server:test | ||
- name: Build & Test Server | ||
run: ./gradlew server:test server:shadowJar | ||
- name: Set up QEMU |
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.
QEMU is needed to emulate an ARM architecture inside x86
run: ./gradlew server:test server:shadowJar | ||
- name: Set up QEMU | ||
uses: docker/setup-qemu-action@v1 | ||
- name: Set up Docker Buildx |
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.
BuildX (BuildKit) is needed to be able to build multiplatform images
docker tag $ECR_REGISTRY/$ECR_REGISTRY_ALIAS/$ECR_REPOSITORY:$IMAGE_TAG $ECR_REGISTRY/$ECR_REGISTRY_ALIAS/$ECR_REPOSITORY:latest | ||
docker push $ECR_REGISTRY/$ECR_REGISTRY_ALIAS/$ECR_REPOSITORY:$IMAGE_TAG | ||
docker push $ECR_REGISTRY/$ECR_REGISTRY_ALIAS/$ECR_REPOSITORY:latest | ||
uses: docker/build-push-action@v5 |
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.
Leveraging an already existing and official action to build and push the image. This seems to be better than doing the commands one by one. Maybe we should do this with all the other docker build
.
@@ -1,8 +1,3 @@ | |||
FROM gradle:8 as builder |
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.
The jar is compiled on the pipeline at a previous step. This ensures two things:
- Drastically reduces build time for linux/arm64 images. It was taking around 3 to 5 mins just to compile the jar
- By compiling it in the pipeline the jar can also be used by the
lh-server
image
COPY --from=builder /lh/server/build/libs/server-*-all.jar /lh/server.jar | ||
COPY ./server/build/libs/server-*-all.jar /lh/server.jar | ||
|
||
ENV LHD_API_HOST=localhost |
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.
These environment variables where missing for the dashboard to work inside lh-standalone
- name: Tests | ||
run: ./gradlew server:test | ||
- name: Build & Test Server | ||
run: ./gradlew server:test server:shadowJar |
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 wonder if is it a good idea to include the task server:e2e
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.
Oh great question, what do you think @sauljabin?
TBH, I don't really think we should be running any test at this point. I might be wrong, but my understanding is that we should only reach this publish
workflow once our other pipeline/workflows have successfully validated that everything is correct.
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 agree with that; I would even say don't put server:test
here.
@@ -40,7 +40,7 @@ else | |||
export AUTH_SECRET=$(uuidgen) | |||
fi | |||
|
|||
export HOSTNAME=localhost | |||
export HOSTNAME=0.0.0.0 |
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.
For some reason I don't seem to understand, localhost
was not working and thus we were not able to stablish a connection to the dashboard on the lh-standalone
image
All our images are built for the linux/amd64 platform because that's the platform of the Github Agent. This means that our images do not work on ARM-based Mac computers (e.g. M1, M2, etc.). That is a big problem considering that a lot of developers use Macs for their day to day work.
This PR introduces the
docker/build-push-action
action to help us build a multiplatform image (namely linux/amd64 and linux/arm64) for thelh-standalone
image. We should probably consider doing all our other images multiplatform, the major drawback is that the image size seems to become quite bigger.Also, the dashboard was not working on
lh-standalone
. That's also fixed as part of this PR.