-
Notifications
You must be signed in to change notification settings - Fork 1
chore: improve Makefile with better organization and features #5
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
- Extract common variables (BUILD_DIR, MODULE, LDFLAGS, DOCKER_REGISTRY, COVER_FILE) - Fix coverage file naming from go-sim-lb to go-template - Fix Docker image tags with proper registry/name:version format - Add docker target to build all images - Improve lint target with proper error handling for gofmt/gofumpt - Add install target for installing binaries to GOPATH/bin - Add run-cli and run-httpserver targets for local development - Use rm -f instead of unlink for better portability - Add Docker section header for better organization Closes #4 Co-authored-by: Chris Hager <metachris@users.noreply.github.com>
|
Claude finished @claude[bot]'s task in 1m 9s —— View job PR Review: Makefile Improvements
Overall Assessment: ✅ Good PR with solid improvements This PR brings welcome organizational improvements to the Makefile. The changes are well-structured and follow Makefile best practices. What's Good
Suggestions (see inline comments)
Minor Notes
LGTM with minor suggestions. The improvements are meaningful and the code is clean. |
| lint: ## Run linters | ||
| gofmt -d -s . | ||
| gofumpt -d -extra . | ||
| @if [ -n "$$(gofmt -d -s . 2>&1)" ]; then gofmt -d -s .; exit 1; fi |
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.
gofmt -d -s . sends the diff to stdout, but the 2>&1 redirect captures stderr too. If gofmt finds formatting issues, it outputs them to stdout (not stderr), but the exit code is still 0. Your check works because you're testing if there's any output.
However, there's a potential edge case: if gofmt encounters an error (e.g., syntax error in a Go file), it writes to stderr and returns a non-zero exit code. With 2>&1, you're now capturing error messages too, which could mask the original error behavior.
Consider a cleaner approach that separates the concerns:
@gofmt -d -s . | grep . && exit 1 || trueOr to be more explicit about the intent:
@test -z "$$(gofmt -d -s .)" || (gofmt -d -s . && exit 1)The current implementation will work for the common case, but the error handling could be improved.
| LDFLAGS := -X $(MODULE)/common.Version=$(VERSION) | ||
|
|
||
| # Docker configuration | ||
| DOCKER_REGISTRY ?= your-registry |
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.
💡 Suggestion: The default registry value your-registry is a good placeholder, but consider adding a comment or validation to remind users to set this before using Docker targets.
Alternatively, you could add a guard in the docker targets:
docker-cli: ## Build the CLI Docker image
@test "$(DOCKER_REGISTRY)" != "your-registry" || (echo "Error: DOCKER_REGISTRY must be set" && exit 1)
...This would prevent accidentally pushing images to a non-existent registry.
9573153 to
a3b3190
Compare
Summary
go-sim-lb.cover.tmp(copy-paste artifact) togo-template.cover.tmpregistry/name:versionformatdockertarget to build all imagesinstalltarget for installing binaries to GOPATH/binrun-cliandrun-httpservertargets for local developmentrm -finstead ofunlinkfor better portabilityNew Targets Available
Configuration Variables
The Makefile now supports these configurable variables:
DOCKER_REGISTRY- Set your Docker registry (default:your-registry)make docker-cli DOCKER_REGISTRY=ghcr.io/flashbotsCloses #4
Generated with Claude Code