Skip to content

fix for docker build path#234

Merged
jbsmith7741 merged 2 commits intomainfrom
docker-fix
Sep 18, 2025
Merged

fix for docker build path#234
jbsmith7741 merged 2 commits intomainfrom
docker-fix

Conversation

@jbsmith7741
Copy link
Collaborator

@jbsmith7741 jbsmith7741 commented Aug 20, 2025

PR Type

Bug fix


Description

  • Fix Docker build to use Linux-specific binaries

  • Add dedicated linux_build target for cross-compilation

  • Ensure Docker container uses correct architecture binaries


Diagram Walkthrough

flowchart LR
  A["Makefile"] --> B["linux_build target"]
  B --> C["CGO_ENABLED=0 GOOS=linux"]
  C --> D["Linux binaries in build/linux/"]
  D --> E["Docker build"]
Loading

File Walkthrough

Relevant files
Bug fix
Makefile
Add Linux-specific build target for Docker                             

Makefile

  • Add new linux_build target with cross-compilation flags
  • Create separate Linux binary output directory
  • Update Docker target to use linux_build instead of all
  • Set CGO_ENABLED=0 and GOOS=linux for containerized builds
+5/-2     

@qodo-code-review
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Output Path Consistency

The new linux_build target outputs binaries under build/linux/, but the install rule and other targets still reference build/ without the linux/ subdir. Verify downstream targets (e.g., install, runtime scripts, Dockerfile COPY) use the correct path to avoid missing binaries.

linux_build:
	@mkdir -p ${BLDDIR}/linux
	CGO_ENABLED=0 GOOS=linux go build ${GOFLAGS} -o ${BLDDIR}/linux/ $(addprefix ./apps/utils/, $(TOOLS)) $(addprefix ./apps/workers/, $(APPS)) ./apps/flowlord
Target Dependencies

The docker target now depends on linux_build but not on cleaning or rebuilding when sources change. Ensure prerequisites or phony declarations keep artifacts up-to-date; consider whether all/clean should run first or if linux_build should depend on source patterns.

docker: linux_build
	docker build -t hydronica/task-tools:${version} .
GOFLAGS Propagation

Confirm GOFLAGS and environment for reproducible builds (e.g., -trimpath, -ldflags for versioning) are preserved in linux_build since previous all target change removed the direct go build invocation there.

@mkdir -p ${BLDDIR}/linux
CGO_ENABLED=0 GOOS=linux go build ${GOFLAGS} -o ${BLDDIR}/linux/ $(addprefix ./apps/utils/, $(TOOLS)) $(addprefix ./apps/workers/, $(APPS)) ./apps/flowlord

@qodo-code-review
Copy link

qodo-code-review bot commented Aug 20, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against empty image tag

Validate that 'version' is provided to prevent tagging 'hydronica/task-tools:'
with an empty tag. Add a guard to fail fast when 'version' is missing.

Makefile [46-48]

 docker: linux_build
+ifndef version
+	$(error version is undefined. Invoke as 'make docker version=<tag>')
+endif
 	docker build -t hydronica/task-tools:${version} .
 	docker push hydronica/task-tools:${version}
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies a bug where the docker target would fail if the version variable is not set, and provides a robust solution using a Makefile guard to prevent this.

Medium
General
Produce smaller static binaries

Ensure reproducible, statically linked binaries for scratch/distroless images by
disabling cgo at link time too. Add '-ldflags "-s -w"' to reduce binary size and
ensure no dynamic deps slip in.

Makefile [22-24]

 linux_build:
 	@mkdir -p ${BLDDIR}/linux
-	CGO_ENABLED=0 GOOS=linux go build ${GOFLAGS} -o ${BLDDIR}/linux/ $(addprefix ./apps/utils/, $(TOOLS)) $(addprefix ./apps/workers/, $(APPS)) ./apps/flowlord
+	CGO_ENABLED=0 GOOS=linux go build ${GOFLAGS} -ldflags "-s -w" -trimpath -o ${BLDDIR}/linux/ $(addprefix ./apps/utils/, $(TOOLS)) $(addprefix ./apps/workers/, $(APPS)) ./apps/flowlord
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This suggestion provides valuable optimizations for production Go builds by adding flags (-ldflags "-s -w", -trimpath) that reduce binary size and improve build reproducibility, which is highly relevant for creating smaller Docker images.

Medium
Stabilize module and build env

Ensure the Go toolchain respects module mode and repeat CGO/GOOS for all
commands in the recipe. Prefix the environment variables for the 'go build'
command only and set GOFLAGS to include -mod=mod to avoid vendoring surprises in
containers.

Makefile [22-24]

 linux_build:
 	@mkdir -p ${BLDDIR}/linux
-	CGO_ENABLED=0 GOOS=linux go build ${GOFLAGS} -o ${BLDDIR}/linux/ $(addprefix ./apps/utils/, $(TOOLS)) $(addprefix ./apps/workers/, $(APPS)) ./apps/flowlord
+	GOFLAGS="${GOFLAGS} -mod=mod" CGO_ENABLED=0 GOOS=linux go build -o ${BLDDIR}/linux/ $(addprefix ./apps/utils/, $(TOOLS)) $(addprefix ./apps/workers/, $(APPS)) ./apps/flowlord
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion to add -mod=mod to GOFLAGS is a good practice that enhances build reproducibility by ensuring Go modules are used consistently, which is beneficial in containerized environments.

Low
Possible issue
Correct per-binary output paths

Avoid polluting the source tree with binaries by outputting named binaries, not
a directory. Use -o with per-target filenames to ensure each binary is placed
under ${BLDDIR}/linux and not overwritten or misinterpreted by go build.

Makefile [22-24]

 linux_build:
 	@mkdir -p ${BLDDIR}/linux
-	CGO_ENABLED=0 GOOS=linux go build ${GOFLAGS} -o ${BLDDIR}/linux/ $(addprefix ./apps/utils/, $(TOOLS)) $(addprefix ./apps/workers/, $(APPS)) ./apps/flowlord
+	CGO_ENABLED=0 GOOS=linux go build ${GOFLAGS} -o ${BLDDIR}/linux/flowlord ./apps/flowlord
+	for tool in $(TOOLS); do CGO_ENABLED=0 GOOS=linux go build ${GOFLAGS} -o ${BLDDIR}/linux/$$tool ./apps/utils/$$tool; done
+	for app in $(APPS); do CGO_ENABLED=0 GOOS=linux go build ${GOFLAGS} -o ${BLDDIR}/linux/$$app ./apps/workers/$$app; done
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: This suggestion identifies a critical bug where the go build command would not produce the intended multiple binaries, and it provides a correct implementation using loops to build each binary individually.

High
General
Improve docker build/push safety

Prevent pushing a partially built or incorrect image by failing the push if the
build fails and by enabling BuildKit for consistent builds. Also tag the image
as latest atomically with the version to avoid drift between tags.

Makefile [46-48]

 docker: linux_build
-	docker build -t hydronica/task-tools:${version} .
+	DOCKER_BUILDKIT=1 docker build -t hydronica/task-tools:${version} -t hydronica/task-tools:latest .
 	docker push hydronica/task-tools:${version}
+	docker push hydronica/task-tools:latest
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This suggestion enhances the Docker workflow by adding a latest tag, a common and useful convention, and enabling BuildKit, which is a modern best practice for Docker builds.

Medium
Harden the build recipe

Ensure builds fail fast on compilation errors by enabling error propagation in
the recipe. Prefix the long build command with a line continuation and split
targets to improve readability and avoid accidental success when part of the
list fails. Also add set -euo pipefail to guard against unset variables and
partial failures.

Makefile [22-24]

 linux_build:
+	@set -euo pipefail
 	@mkdir -p ${BLDDIR}/linux
-	CGO_ENABLED=0 GOOS=linux go build ${GOFLAGS} -o ${BLDDIR}/linux/ $(addprefix ./apps/utils/, $(TOOLS)) $(addprefix ./apps/workers/, $(APPS)) ./apps/flowlord
+	CGO_ENABLED=0 GOOS=linux go build ${GOFLAGS} -o ${BLDDIR}/linux/ \
+		$(addprefix ./apps/utils/, $(TOOLS)) \
+		$(addprefix ./apps/workers/, $(APPS)) \
+		./apps/flowlord
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly proposes adding set -euo pipefail to make the build script more robust, which is a good practice for shell commands within a Makefile.

Low
  • Update

@jbsmith7741 jbsmith7741 merged commit 4298707 into main Sep 18, 2025
2 checks passed
@jbsmith7741 jbsmith7741 deleted the docker-fix branch September 18, 2025 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants