Bring docker-build back #79
Conversation
WalkthroughThis pull request introduces a transition from using Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
Motivation: move us closer to the default kubebuilder scaffold, this would make future upgrades easier. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Makefile (1)
Remove unused ko-related variable
The
KO_VERSIONvariable on line 250 is no longer used and can be safely removed to maintain a clean configuration.🔗 Analysis chain
Line range hint
1-250: Verify complete removal of ko-related configurationsLet's ensure all ko-related configurations have been properly removed or migrated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining ko-related configurations echo "Searching for remaining ko-related configurations..." rg -i 'ko[_-]' --type makeLength of output: 174
🧹 Nitpick comments (2)
Makefile (2)
101-110: LGTM! Clean Docker build and push targetsThe targets are well-documented and properly use the CONTAINER_TOOL variable for flexibility.
Consider adding a check for required environment variables:
.PHONY: docker-push docker-push: ## Push docker image with the manager. + @if [ -z "${IMG}" ]; then echo "IMG is not set"; exit 1; fi $(CONTAINER_TOOL) push ${IMG}
112-130: Enhance error handling in docker-buildx targetThe cross-platform build support is well-implemented, but could benefit from improved error handling.
Consider these improvements:
.PHONY: docker-buildx docker-buildx: ## Build and push docker image for the manager for cross-platform support + @if ! $(CONTAINER_TOOL) buildx version > /dev/null 2>&1; then \ + echo "buildx not available. Please enable docker buildx."; \ + exit 1; \ + fi # copy existing Dockerfile and insert --platform=${BUILDPLATFORM} into Dockerfile.cross, and preserve the original Dockerfile sed -e '1 s/\(^FROM\)/FROM --platform=\$$\{BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\$$\{BUILDPLATFORM\}/' Dockerfile > Dockerfile.cross - - $(CONTAINER_TOOL) buildx create --name jumpstarter-controller-builder + $(CONTAINER_TOOL) buildx create --name jumpstarter-controller-builder 2>/dev/null || true $(CONTAINER_TOOL) buildx use jumpstarter-controller-builder - - $(CONTAINER_TOOL) buildx build --push --platform=$(PLATFORMS) \ + $(CONTAINER_TOOL) buildx build --push --platform=$(PLATFORMS) \ --tag ${DOCKER_REPO}:${DOCKER_TAG} \ --tag ${DOCKER_REPO}:latest \ -f Dockerfile.cross . - - $(CONTAINER_TOOL) buildx rm jumpstarter-controller-builder - rm Dockerfile.cross + $(CONTAINER_TOOL) buildx rm jumpstarter-controller-builder 2>/dev/null || true + rm -f Dockerfile.cross
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/build.yaml(1 hunks)Dockerfile(1 hunks)Makefile(4 hunks)README.md(1 hunks)test/e2e/e2e_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: deploy-kind
- GitHub Check: lint-go
- GitHub Check: e2e-tests
- GitHub Check: tests
🔇 Additional comments (9)
test/e2e/e2e_test.go (1)
66-66: LGTM! Command updated to use docker-build.The change aligns with the transition from ko to docker-based build system.
Dockerfile (4)
2-2: LGTM! Using UBI go-toolset with fixed version.Using
registry.access.redhat.com/ubi9/go-toolset:1.22.9aligns with the previous discussion about using a version that's available downstream.
7-11: LGTM! Proper layer caching implementation.Good practice to copy and download dependencies before source code to leverage Docker layer caching.
19-23: LGTM! Cross-platform build support.Good documentation explaining the GOARCH handling for cross-platform builds. The empty default allows the binary to match the build platform.
25-28: Consider workspace permissions in builder stage.Based on previous review comments, there might be permission issues when building on UBI due to workspace ownership.
Run this script to check workspace permissions in the builder image:
.github/workflows/build.yaml (1)
42-42: LGTM! Using docker-buildx for cross-platform builds.The change to
docker-buildxenables multi-architecture image builds, which is necessary for releasing to quay.io. As discussed in previous comments, whiledocker-buildworks with podman for single architecture,docker-buildxis preferred for releases.README.md (1)
18-18: LGTM! Documentation updated for docker-push.The README correctly reflects the transition from ko to docker-based build system.
Makefile (2)
5-6: LGTM! Well-structured variable declarationsThe variable declarations are clean and follow best practices:
- Proper extraction of repository and tag from IMG variable
- Well-documented CONTAINER_TOOL variable with flexibility for alternative tools like podman
Also applies to: 17-21
153-153: LGTM! Correct dependency updateThe deploy target correctly depends on docker-build, aligning with the transition from ko to Docker.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
101-111: Enhance target documentation and declarations.While the implementation is correct, consider:
- Adding more descriptive comments about expected outcomes and prerequisites
- Moving the .PHONY declarations to be closer to their targets for better maintainability
# If you wish to build the manager image targeting other platforms you can use the --platform flag. # (i.e. docker build --platform linux/arm64). However, you must enable docker buildKit for it. # More info: https://docs.docker.com/develop/develop-images/build_enhancements/ +# Prerequisites: +# - Docker or alternative container tool configured in CONTAINER_TOOL +# - Proper permissions to build containers .PHONY: docker-build docker-build: ## Build docker image with the manager. $(CONTAINER_TOOL) build -t ${IMG} . +# Prerequisites: +# - Completed docker-build +# - Authentication configured for the target registry .PHONY: docker-push docker-push: ## Push docker image with the manager. $(CONTAINER_TOOL) push ${IMG}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/build.yaml(1 hunks)Dockerfile(1 hunks)Makefile(4 hunks)README.md(1 hunks)test/e2e/e2e_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/build.yaml
- test/e2e/e2e_test.go
- README.md
- Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: deploy-kind
- GitHub Check: e2e-tests
🔇 Additional comments (4)
Makefile (4)
5-6: LGTM! Clean variable extraction from IMG.The shell commands correctly split the IMG variable into repository and tag components.
17-22: LGTM! Good practice allowing container tool flexibility.The CONTAINER_TOOL variable with clear documentation enables future flexibility while maintaining Docker as the well-tested default.
153-153: LGTM! Proper dependency chain for deployment.The deploy target correctly depends on docker-build, cluster, and grpcurl, maintaining a proper build and deployment sequence.
112-130: Verify buildx setup and enhance Dockerfile modification.The multi-platform build implementation is solid, but consider:
- The sed command for Dockerfile modification might be fragile if the Dockerfile format changes
- The buildx builder might conflict with existing instances
Run this script to verify the buildx setup and Dockerfile modification:
✅ Verification successful
The multi-platform build implementation is correct and robust
The current implementation:
- Correctly modifies only the builder stage's FROM instruction
- Properly handles multi-stage builds
- Uses appropriate platform-specific arguments
- Includes proper cleanup of temporary files and builder instances
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify buildx prerequisites and Dockerfile compatibility # Check if buildx is available docker buildx version || echo "buildx not available" # Verify that the Dockerfile modification works as expected if [ -f Dockerfile ]; then # Test the sed command sed -e '1 s/\(^FROM\)/FROM --platform=\${BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\${BUILDPLATFORM\}/' Dockerfile > test.dockerfile # Check if the modification was successful grep -q 'FROM --platform=${BUILDPLATFORM}' test.dockerfile || echo "Dockerfile modification failed" rm test.dockerfile else echo "Dockerfile not found" fi # Check for existing builder instances docker buildx ls | grep jumpstarter-controller-builder && echo "Warning: builder instance already exists"Length of output: 557
Script:
#!/bin/bash # Verify Dockerfile structure and modification logic # Check Dockerfile existence and content if [ -f Dockerfile ]; then echo "=== Original Dockerfile first line ===" head -n 1 Dockerfile echo -e "\n=== Testing sed command ===" # Test the sed modification sed -e '1 s/\(^FROM\)/FROM --platform=\${BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\${BUILDPLATFORM\}/' Dockerfile > test.dockerfile head -n 1 test.dockerfile rm test.dockerfile echo -e "\n=== Checking for existing builder instances ===" # Search for existing builder configuration in common locations find . -type f -name "*.yaml" -o -name "*.yml" | xargs grep -l "jumpstarter-controller-builder" || echo "No existing builder configurations found" else echo "Dockerfile not found in repository root" # Try to locate Dockerfile in subdirectories find . -name "Dockerfile" -type f fiLength of output: 730
Script:
#!/bin/bash # Analyze Dockerfile structure and FROM instructions echo "=== Dockerfile Content ===" cat Dockerfile echo -e "\n=== Locating FROM instructions ===" grep -n "^FROM" Dockerfile || echo "No FROM instructions found" echo -e "\n=== Testing modified sed command ===" # Modified sed to target first FROM instruction sed '/^FROM/ {s/\(^FROM\)/FROM --platform=\${BUILDPLATFORM\}/;:a;n;ba}' Dockerfile > test.dockerfile echo "=== Modified Dockerfile first FROM instruction ===" grep -n "^FROM" test.dockerfile rm test.dockerfileLength of output: 1952
| COPY go.sum go.sum | ||
| # cache deps before building and copying source so that we don't need to re-download as much | ||
| # and so that source changes don't invalidate our downloaded layer | ||
| RUN go mod download |
2 participants
Summary by CodeRabbit
Build Process
docker-buildxkotool to native Docker build methodsDocumentation
Infrastructure