-
Notifications
You must be signed in to change notification settings - Fork 5
feat(makefile): adding docker-build and docker-run targets to the makefile
#71
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
base: main
Are you sure you want to change the base?
Conversation
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.
@atallison Thanks for taking this one up! I've left some review comments.
Makefile
Outdated
| PWD = $(shell pwd) | ||
| GO_BUILD= go build | ||
| GOFLAGS= CGO_ENABLED=0 | ||
| REGISTRY_IMAGE_NAME=golang_app |
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.
@atallison I would replace the var name with DOCKER_REPO for 2 reasons: the prefix DOCKER signifies the scope and a docker repo is usually synonymous with <registry>/<image>.
I would also replace the value to rog-golang-buddies/template.
Makefile
Outdated
| ## docker-build: Builds a docker image | ||
| .PHONY: docker-build | ||
| docker-build: | ||
| docker build . -t $(REGISTRY_IMAGE_NAME) |
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.
Given that we can run this as make docker REGISTRY_IMAGE_NAME=template override the image, we should also use DOCKER_TAG with a default value of latest. That way the tag can be explicitly set, specially in CI.
Makefile
Outdated
| ## docker-run: Runs the docker image built by make docker-build | ||
| .PHONY: docker-run | ||
| docker-run: | ||
| docker run $(REGISTRY_IMAGE_NAME) |
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.
@atallison, We should probably add the --rm option to automatically removes the container on exit.
Makefile
Outdated
| docker-build: | ||
| docker build . -t $(REGISTRY_IMAGE_NAME) | ||
|
|
||
| ## docker-run: Runs the docker image built by make docker-build |
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.
@atallison, Can we keep the help messages consistent? "Build" instead of "Builds" etc.
Maybe we should update the message to "Runs the docker image built via [make docker-build]" to separate out that it is a command?
Description
Adding three new make file targets,
make docker-build,make docker-runandmake dockerfor convenience in building and running the docker image of the project.Fixes #49
Current status
Semantic Versioning
featchangeType of change
How Has This Been Tested?
Manual
make docker-buildREGISTRY_IMAGE_NAMEvariable in the make filemake docker-buildmake docker-runmake dockerChecklist: