Update dockerfile to start supporting multi-arch builds #89
Conversation
|
Warning Rate limit exceeded@AjayJagan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe Dockerfile was refactored to use a single-stage build based on the Red Hat UBI 9 go-toolset 1.23 image, replacing the previous multi-stage approach. Manual installation of build dependencies and Go was removed, and build arguments were simplified. The build command now utilizes Changes
Suggested labels
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi Team, Can we also port these changes to the upstream project ? cc: @grdryn |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Dockerfile (2)
4-4:TARGETOSbuild arg is declared but never used
You only consumeTARGETARCH, soTARGETOSadds noise and invites confusion.-ARG TARGETOS TARGETARCH +ARG TARGETARCH
2-22: Minor: reuse Go module cache between builds to shrink CI times
You can split dependency download into its own layer to leverage Docker cache:FROM registry.access.redhat.com/ubi9/go-toolset:1.23 AS builder ARG TARGETARCH WORKDIR /workspace # -- deps layer ------------------------------------------------- COPY go.mod go.sum ./ RUN go mod download # -- source layer ---------------------------------------------- COPY . . RUN CGO_ENABLED=0 GOOS=linux GOARCH=${TARGETARCH:-amd64} make go-build-for-imageNice-to-have, but keeps incremental builds < 10 s.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: precommit
🔇 Additional comments (1)
Dockerfile (1)
17-17: Cross-compile + CGO: verify tool-chain availability for non-amd64 arches
CGO_ENABLED=1requires cross libc + gcc support for every$TARGETARCHyou intend to build (e.g.aarch64,ppc64le). The UBI 9 tool-set image ships only native gcc; cross builds will fail unlessgcc-$ARCH-linux-gnupackages are added.
Either disable CGO (CGO_ENABLED=0) or extend the image with the needed cross tool-chains.
AjayJagan
force-pushed
the
konflux-dockerfile-update
branch
3 times, most recently
from
3741763 to
1efc90a
Compare
AjayJagan
force-pushed
the
konflux-dockerfile-update
branch
from
1efc90a to
f533436
Compare
|
/test e2e-odh-cfo |
|
@Fiona-Waters @tedhtchang , can some help me review this please :) |
|
@AjayJagan: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
chipspeak
requested review from
laurafitzgerald,
pawelpaszki,
kryanbeane and
chipspeak
|
Hey Ajay, this looks good to me. We run our CI and e2e in the upstream moreso so I can't speak to the prow failure there. Are there some example multi-arch images built during the testing of this that we could see? |
|
@chipspeak , here is an index image that was built in konflux: quay.io/redhat-user-workloads/open-data-hub-tenant/codeflare-operator-multi-arch-poc:3adac4d93c0c4deedef5fdb484b6bdbccdfccb14 |
I think we'd need to have a chat about this as a team because that would involve onboarding the upstream project to Konflux. cc @laurafitzgerald |
Has this image been put through any testing within the context of RHOAI or is it purely the Konflux ones and then whatever CI it passes? |
Nope, no testing has been done in context of RHOAI.
yes |
There was a problem hiding this comment.
Hi @AjayJagan Apologies for the delay in reviewing here. it would be great if we can submit these changes upstream as you suggested. We have a sync which will bring them here. Depending on the timeframe you'd like this work to be completed by. If there isn't time, can you add the appropriate CARRY or PATCH prefix to your commit message.
Also you can ignore the failing test for now. Ref https://issues.redhat.com/browse/RHOAIENG-29260
I can override that for you if you want to get this pr merged.
|
|
||
| FROM registry.access.redhat.com/ubi8/ubi-minimal:8.8 | ||
| FROM registry.access.redhat.com/ubi9/ubi-minimal:latest |
There was a problem hiding this comment.
Is it usual to use the latest tag here for ubi9?
There was a problem hiding this comment.
I have it ready now : project-codeflare#690 |
|
@laurafitzgerald , should I close this in favor of the one opened in the upstream? |
|
closing this PR as the change is merged upstream and will be synced here. |
Description
As a part of an effort to enable multi arch builds in konflux, we need to update the Dockerfile.
JIRA: https://issues.redhat.com/browse/RHOAIENG-28666
How Has This Been Tested?
Image built from konflux: quay.io/redhat-user-workloads/open-data-hub-tenant/codeflare-operator-multi-arch-poc:3adac4d93c0c4deedef5fdb484b6bdbccdfccb14
Merge criteria:
Summary by CodeRabbit