-
Notifications
You must be signed in to change notification settings - Fork 641
ci: OPS-980: Add operator build and push per-commit #3620
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Dillon Cullinan <[email protected]>
WalkthroughAdds an operator build-and-push job to the backend container validation workflow with a two-architecture matrix, and updates the operator Dockerfile to rely on externally provided TARGETARCH (no default). No other logic changes noted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer/PR
participant GH as GitHub Actions
participant BX as Docker Buildx
participant REG as Registries (AWS/Azure)
Dev->>GH: Push/PR triggers workflow
GH->>GH: Detect code changes
alt operator job matrix (amd64, arm64)
GH->>GH: Checkout repo
GH->>BX: Setup Buildx (platform = linux/${arch})
GH->>BX: docker build deploy/cloud/operator (TARGETOS/TARGETARCH set)
BX-->>GH: Operator image for platform
GH->>REG: Tag & push (ai-dynamo/dynamo:operator-${arch})
REG-->>GH: Push result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
Signed-off-by: Dillon Cullinan <[email protected]>
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/cloud/operator/Dockerfile (1)
44-48
: ARG scope bug: TARGETOS/TARGETARCH not defined in builder stage
RUN ... GOOS=${TARGETOS} GOARCH=${TARGETARCH} ...
expands to empty unless these ARGs are re-declared in this stage. Builds may target wrong arch or fail.Apply one of the fixes:
Option A (re-declare in builder):
FROM base AS builder +ARG TARGETOS +ARG TARGETARCH + # Build the binary RUN CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build -o manager ./cmd/main.goOption B (hoist ARGs global so all stages see them):
-# Base stage - Common setup for all stages -FROM golang:1.24 AS base - -# Docker buildx automatically provides these -ARG TARGETOS=linux -ARG TARGETARCH +# Docker buildx automatically provides these +ARG TARGETOS=linux +ARG TARGETARCH + +# Base stage - Common setup for all stages +FROM golang:1.24 AS base + +# Make them visible in this stage too (optional but explicit) +ARG TARGETOS +ARG TARGETARCH
🧹 Nitpick comments (4)
deploy/cloud/operator/Dockerfile (2)
47-47
: Improve local dev ergonomics with safe fallbacksIf someone builds without buildx, fall back to
go env
so GOOS/GOARCH resolve correctly.-RUN CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build -o manager ./cmd/main.go +RUN CGO_ENABLED=0 GOOS=${TARGETOS:-$(go env GOOS)} GOARCH=${TARGETARCH:-$(go env GOARCH)} go build -o manager ./cmd/main.go
8-12
: Also declare TARGETPLATFORM for clarity (optional)Expose TARGETPLATFORM and log it; helps diagnostics in multi-arch builds.
-# Docker buildx automatically provides these -ARG TARGETOS=linux -ARG TARGETARCH +# Docker buildx automatically provides these +ARG TARGETPLATFORM +ARG TARGETOS=linux +ARG TARGETARCH -RUN echo "Building for ${TARGETOS}/${TARGETARCH}" +RUN echo "Building for ${TARGETPLATFORM:-${TARGETOS}/${TARGETARCH}}".github/workflows/container-validation-backends.yml (2)
63-74
: Action metadata warning: docker-tag-push missing name (actionlint)Static analysis flagged the composite action metadata missing
name
. Fixing it avoids CI lint failures.Please update
.github/actions/docker-tag-push/action.yml
to include:name: Docker Tag and PushIf you want, I can scan the repo and open a PR to patch it.
40-51
: Optional: tighten change filter to operator pathIf
has_code_changes
is broad, the operator job may run unnecessarily. Consider using a path-specific filter (e.g.,deploy/cloud/operator/**
) to gate this job.Would you like a patch to extend
.github/filters.yaml
and wire it here?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/container-validation-backends.yml
(1 hunks)deploy/cloud/operator/Dockerfile
(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/container-validation-backends.yml
63-63: name is required in action metadata "/home/jailuser/git/.github/actions/docker-tag-push/action.yml"
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
Signed-off-by: Dillon Cullinan <[email protected]>
Overview:
TARGETARCH
in operatorDockerfile
, this variable is set automatically bydocker buildx
, hard-coding the variable made invocations of--platform linux/arm64
build withlinux/amd64
instead, resulting in incorrect images.Summary by CodeRabbit