-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat(makefile): adding docker-build
and docker-run
targets to the makefile
#71
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
@@ -2,6 +2,7 @@ SHELL=/bin/bash -e -o pipefail | |||
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-run
andmake docker
for convenience in building and running the docker image of the project.Fixes #49
Current status
Semantic Versioning
feat
changeType of change
How Has This Been Tested?
Manual
make docker-build
REGISTRY_IMAGE_NAME
variable in the make filemake docker-build
make docker-run
make docker
Checklist: