Conversation
| TASK_RUNNER_REPO ?= $(AX_IMAGE_REPO)/ax-task-runner | ||
| CONTAINER_CLI ?= $(shell which podman 2>/dev/null || which docker 2>/dev/null) | ||
| KO_PLATFORM ?= linux/amd64,linux/arm64 | ||
| TASK_RUNNER_PLATFORMS ?= $(KO_PLATFORM) |
There was a problem hiding this comment.
Could we keep TASK_RUNNER_PLATFORMS independent from KO_PLATFORM here?
These variables control two different build paths (ko for the Go services and the task-runner's Go/Docker build). Coupling them means changing KO_PLATFORM implicitly changes the task-runner build targets as well.
I think explicit defaults would make the interface clearer and avoid surprising behavior:
KO_PLATFORM ?= linux/amd64,linux/arm64
TASK_RUNNER_PLATFORMS ?= linux/amd64,linux/arm64If the intention is to deliberately share this configuration, could we document that relationship?
| GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -o bin/linux_amd64/ax-task-runner ./cmd/ax-task-runner | ||
| @echo "==> Cross-compiling ax-task-runner for $(TASK_RUNNER_PLATFORMS)..." | ||
| @for p in $$(echo $(TASK_RUNNER_PLATFORMS) | tr ',' ' '); do \ | ||
| arch=$$(basename $$p); \ |
There was a problem hiding this comment.
Could we avoid deriving GOARCH with basename from the OCI platform string?
This assumes every supported platform maps directly to a Go architecture and can break for valid platform specifications containing additional components, e.g. linux/arm/v7 or linux/arm64/v8.
Since this value is ultimately passed to GOARCH, I'd prefer either explicitly maintaining the supported Go architectures (amd64, arm64) or adding validation/mapping from platform → GOARCH before invoking go build.
| @if [ "$$(basename $(CONTAINER_CLI))" = "podman" ]; then \ | ||
| $(CONTAINER_CLI) manifest push $(TASK_RUNNER_REPO):latest $(TASK_RUNNER_REPO):latest 2>/dev/null || $(CONTAINER_CLI) push $(TASK_RUNNER_REPO):latest; \ | ||
| else \ | ||
| $(CONTAINER_CLI) buildx build --platform $(TASK_RUNNER_PLATFORMS) -t $(TASK_RUNNER_REPO):latest -f Dockerfile.task-runner --push .; \ |
There was a problem hiding this comment.
Could we clarify the intended output of this buildx build?
With multiple platforms, BuildKit cannot generally load the resulting multi-platform image into the classic local Docker image store as a normal image. This target also doesn't use --push or an explicit output.
push-task-runner subsequently performs another multi-platform build with --push, so make build-task-runner and make push-task-runner now have somewhat surprising semantics and the image may be built twice.
Could we make the build/publish flow explicit—for example, have the build target use an appropriate local/export output and have the push target be the single publishing path?
| @echo "==> Pushing task runner image to $(TASK_RUNNER_REPO):latest..." | ||
| $(CONTAINER_CLI) push $(TASK_RUNNER_REPO):latest | ||
| @if [ "$$(basename $(CONTAINER_CLI))" = "podman" ]; then \ | ||
| $(CONTAINER_CLI) manifest push $(TASK_RUNNER_REPO):latest $(TASK_RUNNER_REPO):latest 2>/dev/null || $(CONTAINER_CLI) push $(TASK_RUNNER_REPO):latest; \ |
There was a problem hiding this comment.
Could we avoid suppressing the manifest-push error here?
2>/dev/null || ... can hide the actual reason the multi-platform manifest push failed and then fall back to a different push operation. For a release/deployment target, that can make an architecture-related publishing failure difficult to diagnose.
I'd prefer failing with the original error, or using an explicit Podman/Docker-specific publishing path with clear failure semantics.
There was a problem hiding this comment.
@rakyll Overall, this is a good direction and it addresses the core problem in #358: the previous build/deploy path was hardcoded to linux/amd64, while this change adds amd64/arm64 support to the task runner and passes multiple platforms to ko.
The TARGETARCH change in Dockerfile.task-runner and the ko --platform changes are straightforward and aligned with the intended fix.
Before merging, I think the build/publish workflow needs some tightening:
TASK_RUNNER_PLATFORMSis coupled toKO_PLATFORMeven though they control separate build systems.basenameis being used to convert OCI platform strings intoGOARCH, which assumes a narrower platform format than the variable name suggests.- The Docker Buildx multi-platform build has no explicit
--pushor--load, whilepush-task-runnerbuilds the image again. This makes the target semantics unclear and can result in duplicate builds. - The manifest push suppresses the underlying error, which makes release failures harder to diagnose.
- There is currently no automated verification that both architectures are actually produced and published correctly.
I would address these points and add a small CI/build verification for linux/amd64 and linux/arm64. With those changes, the multi-architecture support would be much easier to reason about and maintain.
Fixes #358.