feat: add docker image on release and master #135
Conversation
|
@khos2ow here is the PR 😆 |
There was a problem hiding this comment.
Thank you @anthonymq for the awesome work, functionality wise it's finished and /lgtm. But the only remaining open question is where to push the images:
- GitHub Packages or
- quay.io
|
@anthonymq in the meantime please add a new section in Installation guide as well, until we hear back from folks. |
Pull Request Test Coverage Report for Build 708956705
💛 - Coveralls |
|
This is great work! Thank you 🥇 Before I weigh in on which registry I would recommend, I am doing some local testing with GH Registry. Should be able to weigh in today. |
There was a problem hiding this comment.
After playing with GH Container Registry, I really like it. That said, it does pose a particular hurdle for this project as we would need to get @wadackel to enable the feature on the Organization before we can push. While not the biggest hurdle, it is one that we do not have a timeline on.
I went ahead and created a quay.io org and repo for this project. I added notes inline where we will need to update to point to the correct registry.
I also added a bot user to the Quay org and update the Repo secrets with the credentials.
Again, awesome work!
| RUN apk add --no-cache git && \ | ||
| mkdir /workdir | ||
|
|
||
| COPY git-chglog /usr/local/bin/git-chglog |
There was a problem hiding this comment.
I want to note that this works due to a "side effect" that we (and goreleaser) run make build on a 64-bit version of Linux. As a result, we can use the make build command outside of the container and then COPY it into the alpine image, 💥 it works.
A use case to consider: If a user attempts to build the image locally on a Mac or Windows host, the resulting docker image will throw a runtime error due to an incompatible architecture.
If we wanted to be pedantic, we could make a target that specifically calls out that we are creating the binary for the purpose of using within an apline image.
alpine: clean
env GOOS=linux GARCH=amd64 CGO_ENABLED=0 go build -ldflags=$(LDFLAGS) -o git-chglog ./cmd/git-chglog
docker: alpine
docker build --tag quay.io/git-chglog/git-chglog:local .Just something to consider.
There was a problem hiding this comment.
This, actually, was the main motivation for me on Makefile refactoring, because I was trying to test this locally and noticed make build doesn't statically build the binary which has been modified since.
That said in the snippet above alpine is not necessarily needed, but docker is very nice to have and very convenient. 👍
There was a problem hiding this comment.
Are you okay with ?
docker: build
docker build --tag git-chglog .
It will be easier to use locally and i could leverage that on the CI
There was a problem hiding this comment.
Right on. If we wanted to ensure that the GOHOST values are correct for binary used with the the docker target, what process do you suggest?
The make alpine was just an attempt to encapsulate the generation of the binary used in the docker image. Is there a set of parameters we could pass to make docker that would not require the alpine target? The modifications that first come to mind are adding a build step to the make docker target, but that is just the "hammer sees nail" solution 🤣
There was a problem hiding this comment.
This looks better IMO (somewhere in Build section of Makefile):
.PHONY: docker
docker: DOCKER_TAG ?= dev
docker: build ## Build Docker image
@ $(MAKE) --no-print-directory log-$@
docker build --pull --tag quay.io/git-chglog/git-chglog:$(DOCKER_TAG) .and then you can update publish.yml accordingly: DOCKER_TAG=edge make docker
Co-authored-by: Derek Smith <derek@clokwork.net>
There was a problem hiding this comment.
Great job @anthonymq on this. Really appreciate you working with us on this.
@khos2ow do you feel the discussion we are having regarding make docker is a blocker for merging this PR as is? I am okay with separating that out to a chore PR after this.
| We maintain the following tags: | ||
| - `edge`: Image that is build from the current `HEAD` of the main line branch. | ||
| - `latest`: Image that is built from the [latest released version](https://github.com/git-chglog/git-chglog/releases) | ||
| - `x.y.y` (versions): Images that are build from the tagged versions within Github. |
| RUN apk add --no-cache git && \ | ||
| mkdir /workdir | ||
|
|
||
| COPY git-chglog /usr/local/bin/git-chglog |
There was a problem hiding this comment.
Right on. If we wanted to ensure that the GOHOST values are correct for binary used with the the docker target, what process do you suggest?
The make alpine was just an attempt to encapsulate the generation of the binary used in the docker image. Is there a set of parameters we could pass to make docker that would not require the alpine target? The modifications that first come to mind are adding a build step to the make docker target, but that is just the "hammer sees nail" solution 🤣
|
@clok no it's not a blocker, but I wanted to make sure everything is ready before we merge, and I also wanted to backport older images to quay because it has a weird thing about history. So I want to manually build and push old tags and then merge this one which will update |
Done. That would be awesome to backport the images. Let's break out the Makefile changes into a chore PR. This is ready to 🤘 |
|
@khos2ow @anthonymq Added #137 as a follow up on this. |
What does this do / why do we need it?
It release a docker image for every release. I needed it in my Gitlab-ci to easily generate my changelog.
How this PR fixes the problem?
It builds and push an image to Dockerhub.
What should your reviewer look out for in this PR?
The dockerfile and the github actions files
Check lists
Additional Comments (if any)
You will need to create two secrets on the repo to authenticate to Dockerhub
Which issue(s) does this PR fix?
fixes #132