-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add hypeman build command #21
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
Conversation
Add a new 'build' command that builds images from a Dockerfile and source context. Features: - Takes a folder path as the build context (default: current directory) - Optionally accepts a Dockerfile path via --file/-f flag - Creates a tar.gz archive of the source and uploads via multipart form - Streams build logs in real-time via SSE - Configurable build timeout via --timeout flag (default: 600s) Usage examples: hypeman build hypeman build ./myapp hypeman build -f Dockerfile.prod ./myapp hypeman build --timeout 1200 ./myapp
| fmt.Fprintf(os.Stderr, "Build complete!\n") | ||
| return nil | ||
| case "failed": | ||
| buildError = "build failed" |
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.
Client hangs indefinitely on failed build status
Medium Severity
The handling of the "failed" status in streamBuildEvents is inconsistent with "ready" and "cancelled". When "ready" is received, the function returns immediately with nil. When "cancelled" is received, it returns immediately with an error. However, when "failed" is received, the code only sets buildError and continues reading from the SSE stream, relying on the server to close the connection (trigger EOF). If the server keeps the connection alive after sending "failed" (e.g., continues sending heartbeat events or has networking issues), the client will hang indefinitely instead of returning the failure to the user.
🔬 Verification Test
Why verification test was not possible: This bug requires an external SSE server to demonstrate the hanging behavior. The issue is a logical inconsistency that can be verified by code inspection: the case "failed" block (line 379-380) only sets a variable and does not return, while case "ready" (line 376-378) and case "cancelled" (line 381-382) both include immediate return statements. This inconsistency is visible in the diff and represents a potential client hang if the server doesn't close the connection after sending a failed status.
| break | ||
| } | ||
| return err | ||
| } |
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.
SSE reader drops final event data at EOF
Medium Severity
When reader.ReadString('\n') encounters EOF before finding a newline, it returns both the partial data read and io.EOF as the error. The current code checks for EOF and immediately breaks without processing the line variable, which may contain valid event data. If the server closes the connection without sending a trailing newline after the final event (due to network issues or server behavior), that event is silently dropped. This could cause a final "ready" status to be missed, leading the client to incorrectly report the build failed or ended unexpectedly.
🔬 Verification Test
Why verification test was not possible: This bug requires mocking a network connection that returns data without a trailing newline followed by EOF. The issue is evident from Go's bufio.Reader.ReadString documentation which states: "If ReadString encounters an error before finding a delimiter, it returns the data read before the error and the error itself." The current code structure breaks on EOF without checking if line contains valid data to process first.
rgarcia
left a comment
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.
Nice clean implementation of the build command! This pairs well with the server-side build system in hypeman.
The UX mirrors docker build nicely with the -f flag and context path argument. SSE streaming for real-time logs is a good touch.
Left a few minor comments:
- Consider reading
.dockerignoreinstead of hardcoding skip patterns - File descriptor leak in the tar walk loop
- Show actual error message on build failure
Nothing blocking—ship it! 🚀
| // createSourceTarball creates a gzipped tar archive of the build context | ||
| func createSourceTarball(contextPath string) (*bytes.Buffer, error) { | ||
| buf := new(bytes.Buffer) | ||
| gzWriter := gzip.NewWriter(buf) |
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.
Should we read .dockerignore to determine what to skip, rather than having a hardcoded list? That would match Docker's behavior and give users control over what gets excluded.
| } | ||
|
|
||
| // Use forward slashes for tar paths | ||
| header.Name = filepath.ToSlash(relPath) |
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.
This defer file.Close() is inside the filepath.Walk callback loop—defers don't run until the function returns, so this could leak file descriptors if there are many files. Should close immediately after the copy instead.
| if err := json.Unmarshal([]byte(data), &event); err != nil { | ||
| // Skip malformed events | ||
| continue | ||
| } |
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.
When the build fails, buildError is just set to the static string "build failed"—but the actual error message from the API isn't captured. Would be helpful to show the real error (maybe fetch the build status to get the error field, or parse it from an event if available).
Summary
Add a new
buildcommand that builds images from a Dockerfile and source context.Features
--file/-fflagPOST /buildsGET /builds/{id}/events--timeoutflag (default: 600s).git,node_modules,__pycache__, etc.)Usage Examples
Changes
pkg/cmd/build.go- New file with build command implementationpkg/cmd/cmd.go- Register build command in CLINote
Introduces image-building workflow to the CLI.
buildcommand (pkg/cmd/build.go) to tar.gz the build context, optionally include a Dockerfile (--file), and upload via multipart toPOST /buildswithtimeout_secondsGET /builds/{id}/events(SSE), printing progress and handling completion/failure/cancellation.git,node_modules,__pycache__,.venv,target)HYPEMAN_API_KEY; base URL can be set via root flag--base-urlorHYPEMAN_BASE_URL(defaults tohttp://localhost:8080)buildin the main CLI (pkg/cmd/cmd.go)Written by Cursor Bugbot for commit be699f1. This will update automatically on new commits. Configure here.