fix: combine ENTRYPOINT and CMD from wrapped Dockerfile if present

[seanmcgary]

Motivation:

In a Dockerfile, it is acceptable, and common, to specify an ENTRYPOINT that points to a cli-type binary that can accept multiple commands and then set CMD to a subcommand with args. Previously we were taking only the CMD if present, which would break if using the aforementioned pattern.

Modifications:

Combined ENTRYPOINT and CMD into a single command that the wrapped container will call.

Result:

It no longer breaks!

Testing:

All tests pass, tested functionality on a local build.

Open questions:

None

[claude]

Claude finished @seanmcgary's task —— View job

---

Code Review Complete ✅

Reviewing PR #24: fix: combine ENTRYPOINT and CMD from wrapped Dockerfile if present

Todo List:

• Read and analyze the modified file
• Review code changes for correctness
• Check for potential bugs and edge cases
• Evaluate code quality and Go best practices
• Assess security implications
• Consider test coverage needs
• Provide comprehensive feedback

---

Review Summary

This PR correctly fixes the Docker ENTRYPOINT + CMD combination issue in pkg/commands/utils/build_utils.go:155-171. The change is well-implemented and addresses the stated problem effectively.

✅ Positive Aspects

1. Correct Problem Identification & Solution

• The previous logic had a flaw: it only used CMD if present, falling back to ENTRYPOINT only when CMD was empty
• The new logic correctly combines both ENTRYPOINT and CMD, which matches Docker's standard behavior
• This fixes the common pattern where ENTRYPOINT points to a CLI binary and CMD provides default arguments

2. Code Quality

• Clean, readable implementation using standard Go patterns
• Good variable naming (cmd instead of originalCmd)
• Proper error handling preserved
• Comments clearly explain the intent: "Combine Entrypoint and Cmd to get full command"

3. Minimal, Focused Change

• Only 8 lines added, 4 removed - very surgical fix
• No unnecessary refactoring or scope creep
• Maintains existing function signature and return values

🔍 Technical Analysis

Docker Behavior Alignment
The fix correctly implements Docker's runtime behavior:

• When both ENTRYPOINT and CMD are present: exec ENTRYPOINT + CMD
• When only ENTRYPOINT exists: exec ENTRYPOINT
• When only CMD exists: exec CMD
• When neither exists: empty command array

Edge Cases Handled

• ✅ Empty ENTRYPOINT, populated CMD
• ✅ Populated ENTRYPOINT, empty CMD
• ✅ Both populated (the main fix)
• ✅ Both empty

⚠️ Areas for Consideration

1. Test Coverage Gap

• No unit tests found for extractImageConfig() function
• Consider adding tests covering:

  // Test cases to consider
  {entrypoint: ["/app"], cmd: ["start"], expected: ["/app", "start"]}
  {entrypoint: [], cmd: ["node", "server.js"], expected: ["node", "server.js"]}
  {entrypoint: ["/entrypoint.sh"], cmd: [], expected: ["/entrypoint.sh"]}

2. Backward Compatibility

• Change should be backward compatible since it's fixing incorrect behavior
• Existing deployments using only CMD or only ENTRYPOINT will continue working
• Only improves cases where both are present

🏁 Recommendation

APPROVE - This is a solid bug fix that:

• Correctly implements Docker's ENTRYPOINT + CMD combination semantics
• Uses clean, maintainable Go code
• Addresses a real user pain point
• Has minimal risk of regression

The lack of unit tests is noted but doesn't block this fix since the change is straightforward and the author confirmed manual testing.

📝 Future Suggestions

• Consider adding unit tests for extractImageConfig() in a follow-up PR
• The function could benefit from test coverage given its importance in the container wrapping process

[Copilot]

Pull Request Overview

This PR fixes the handling of Docker image commands by properly combining ENTRYPOINT and CMD directives from wrapped Dockerfiles. Previously, the code only used CMD when present, which broke the common Docker pattern where ENTRYPOINT specifies a CLI binary and CMD provides subcommands/arguments.

Key Changes:

• Modified command extraction logic to concatenate ENTRYPOINT and CMD arrays
• Removed conditional logic that prioritized CMD over ENTRYPOINT

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] Use var cmd []string instead of cmd := []string{} to avoid allocating an empty slice that will be immediately replaced by append operations. This is more idiomatic and slightly more efficient.

Suggested change

	cmd := []string{}
	var cmd []string

[shrimalmadhur]

can we add a test for this whole function?