Skip to content
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: goreleaser release #101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

developer-guy
Copy link

@developer-guy developer-guy commented Dec 4, 2021

Copy link
Collaborator

@willnorris willnorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a bunch of comments below. I worry about one of the last ones actually being a deal breaker, which is an easy way to test the image publication. We get that for free with our actions based workflow today, which you can see in my own fork here: https://github.com/willnorris/addlicense/pkgs/container/addlicense

Looking at the configuration required, I'm also surprised by the amount of additional config we have to do manually that was handled for us with the actions approach before (setting image tags, labels, etc). If signing images with cosign is the end goal, does that actually rely on goreleaser in any way? (Though I'm now also a little concerned that the OIDC support in cosign is still very experimental, so is it really ready for adoption yet?)

I know y'all put some real work into this which I appreciate, and I'm happy to continue talking through things, but I'm now a little more concerned about whether this is actually a good move.

.goreleaser.yaml Outdated
goos:
- linux
- windows
- darwin
goarch:
- amd64
- arm64
#signs:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be good to keep this PR focused on just switching how the docker image is published, without introducing any of the signing functionality. You have that, since this is commented out, but I think it'd be clearer to just leave it out of this PR entirely.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, thanks fixed.

@@ -5,65 +5,47 @@ on:
tags:
- '*'

permissions:
contents: write
id-token: write
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this id-token permission is needed yet, is it? I believe that will be for signing (which we're not doing yet)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, thanks fixed.

-
name: Run GoReleaser
uses: goreleaser/goreleaser-action@5a54d7e660bda43b405e8463261b3d25631ffe86 #v2.7.0
- uses: docker/setup-qemu-action@27d0a4f181a40b142cce983c5393082c365d1480 # pin@v1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our convention is to include the full version we are pinning to. So # v1.10.0 in this case. Same with version comments below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks fixed.

uses: goreleaser/goreleaser-action@5a54d7e660bda43b405e8463261b3d25631ffe86 #v2.7.0
- uses: docker/setup-qemu-action@27d0a4f181a40b142cce983c5393082c365d1480 # pin@v1
- uses: docker/setup-buildx-action@94ab11c41e45d028884a99163086648e898eed25 # pin@v1
- uses: sigstore/cosign-installer@e5c096a9feb091d8afe0168547370270986f2f71 # [email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for this, since we're not signing right now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks fixed.

- uses: docker/setup-qemu-action@27d0a4f181a40b142cce983c5393082c365d1480 # pin@v1
- uses: docker/setup-buildx-action@94ab11c41e45d028884a99163086648e898eed25 # pin@v1
- uses: sigstore/cosign-installer@e5c096a9feb091d8afe0168547370270986f2f71 # [email protected]
- name: dockerhub-login
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't publish to dockerhub, so don't need this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks fixed.

.goreleaser.yaml Outdated
# - '${artifact}'
dockers:
- image_templates:
- 'google/addlicense:{{ .Tag }}-amd64'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't publish to dockerhub, so can remove this name throughout

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, fixed

# make a bare minimal image
FROM scratch

WORKDIR app
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we want to keep the behavior the same as the existing Dockerfile, so this should be:

# source to be scanned should be mounted to /src
WORKDIR /src

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, fixed


WORKDIR app

COPY addlicense ./
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COPY addlicense /app/addlicense

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, fixed

.goreleaser.yaml Outdated
dockers:
- image_templates:
- 'google/addlicense:{{ .Tag }}-amd64'
- 'ghcr.io/google/addlicense:{{ .Tag }}-amd64'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is really unfortunate that this is hardcoded. That makes it a lot harder to test this in forks, unless I'm missing something. For example, your https://github.com/developer-guy/addlicense/releases/tag/v0.999.0 tag demonstrates creating the release artifacts, but doesn't demonstrate publishing the docker image. That something we were able to do with the actions based approach.

It doesn't look like goreleaser's name templates supports the GitHub repo owner, and custom variables are a "pro" feature.

We need some way to test image publication to GHCR before making changes live.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, we can use those variables as an environment variable to pass the registry and the image name to the GoReleaser by using the same syntax, can't we?

${{ env.REGISTRY }}/${{ env.IMAGE_NAME }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willnorris
Copy link
Collaborator

Given GitHub's blog post yesterday, I tried using the purely Actions based cosign workflow on a personal project of mine, and it worked really well. I wonder if we just do that here as well? Any major downside that I'm not seeing?

@developer-guy
Copy link
Author

Given GitHub's blog post yesterday, I tried using the purely Actions based cosign workflow on a personal project of mine, and it worked really well. I wonder if we just do that here as well? Any major downside that I'm not seeing?

No, there is no major downside at all. You're right. We can do it that way as well. Still, GoReleaser already supports that, but it should wait for cosign v1.4.x for the keyless feature, that's why I commented out the signs and docker_signs sections within the .goreleaser.yml file, but anyway, we can do it without using GoReleaser also if you want to move forward like this.

Signed-off-by: Batuhan Apaydın <[email protected]>
Co-authored-by: Furkan Türkal <[email protected]>
Signed-off-by: Batuhan Apaydın <[email protected]>
@developer-guy
Copy link
Author

kindly ping @willnorris, cosign v1.4.1 with some bunch of fixes is released today, so, everything seems fine in cosign project which means that we can start using it to sign addlicense both binary and container image 🤩 similar works being done in several projects such as google/ko, goreleaser.

Copy link
Collaborator

@willnorris willnorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so there's a bunch of comments below. I also tried doing suggested edits to show the exact changes I had in mind.

By the end of all of this, I did get this working in my personal fork of the project (images published here: https://github.com/willnorris/addlicense/pkgs/container/addlicense).

But after all of that (and before you actually start implementing these changes), I'm honestly still not completely convinced that moving the docker build into goreleaser is really worth all this effort. I've certainly learned a lot through this process, getting these builds to work, playing with a lot of new features in both goreleaser and docker I've never used, and learning about cosign. So that's been fun. And it means I'm now coming from a much more informed position than when I started when I ask, what does using goreleaser to build the image actually gain us at this point? I'm not seeing it.

(That said, I'm certainly convinced on using cosign to sign binaries and docker images, which I think is what you were ultimately after?)

env:
DOCKER_CLI_EXPERIMENTAL: "enabled"
REGISTYRY: "ghcr.io"
IMAGE_NAME: "google/addlicense"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this still has the same "testing in forks issues" if we hardcode the value.

Suggested change
IMAGE_NAME: "google/addlicense"
IMAGE_NAME: ${{ github.repository }}

-
name: Run GoReleaser
uses: goreleaser/goreleaser-action@5a54d7e660bda43b405e8463261b3d25631ffe86 #v2.7.0
- uses: docker/setup-qemu-action@27d0a4f181a40b142cce983c5393082c365d1480 # [email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we had an existing convention for all of these comments that would be nice to not change... #v2.6.1. Though i guess a space helps with legibility.

The real reason I care is that I'm trying to get dependabot to add support for updating these comments, and I'd like to keep them in a consistent format.

Suggested change
- uses: docker/setup-qemu-action@27d0a4f181a40b142cce983c5393082c365d1480 # pin@v1.2.0
- uses: docker/setup-qemu-action@27d0a4f181a40b142cce983c5393082c365d1480 # v1.2.0

username: ${{ secrets.DOCKER_USERNAME }}
password: ${{ secrets.DOCKER_PASSWORD }}
- name: ghcr-login
if: startsWith(github.ref, 'refs/tags/v')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe change the trigger for this workflow to only match "v*" tags, so then we don't need this check here?

Suggested change
if: startsWith(github.ref, 'refs/tags/v')

if: startsWith(github.ref, 'refs/tags/v')
uses: docker/login-action@f054a8b539a109f9f41c372932f1ae047eff08c9 # [email protected]
with:
registry: ghcr.io
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
registry: ghcr.io
registry: ${{ env.REGISTRY }}

@@ -5,65 +5,48 @@ on:
tags:
- '*'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- '*'
- 'v*'

Comment on lines +30 to +35
- name: dockerhub-login
if: startsWith(github.ref, 'refs/tags/v')
uses: docker/login-action@f054a8b539a109f9f41c372932f1ae047eff08c9 # [email protected]
with:
username: ${{ secrets.DOCKER_USERNAME }}
password: ${{ secrets.DOCKER_PASSWORD }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dockerhub login is still here, but should be removed.

Suggested change
- name: dockerhub-login
if: startsWith(github.ref, 'refs/tags/v')
uses: docker/login-action@f054a8b539a109f9f41c372932f1ae047eff08c9 # [email protected]
with:
username: ${{ secrets.DOCKER_USERNAME }}
password: ${{ secrets.DOCKER_PASSWORD }}

@@ -5,65 +5,48 @@ on:
tags:
- '*'

permissions:
contents: write
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to re-add packages permission to publish images

Suggested change
contents: write
contents: write
packages: write

- "--platform=linux/arm64"
goarch: arm64
docker_manifests:
- name_template: '{{ .Env.REGISTRY }}/{{ .Env.IMAGE_NAME }}::{{ .Tag }}'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, fails to build image

Suggested change
- name_template: '{{ .Env.REGISTRY }}/{{ .Env.IMAGE_NAME }}::{{ .Tag }}'
- name_template: '{{ .Env.REGISTRY }}/{{ .Env.IMAGE_NAME }}:{{ .Tag }}'

image_templates:
- '{{ .Env.REGISTRY }}/{{ .Env.IMAGE_NAME }}:{{ .Tag }}-amd64'
- '{{ .Env.REGISTRY }}/{{ .Env.IMAGE_NAME }}:{{ .Tag }}-arm64'
- name_template: 'ghcr.io/google/addlicense:latest'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed one hardcoded image name

Suggested change
- name_template: 'ghcr.io/google/addlicense:latest'
- name_template: '{{ .Env.REGISTRY }}/{{ .Env.IMAGE_NAME }}:latest'

goos:
- linux
- windows
- darwin
goarch:
- amd64
- arm64
dockers:
- image_templates:
- '{{ .Env.REGISTRY }}/{{ .Env.IMAGE_NAME }}:{{ .Tag }}-amd64'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love having these extra *-{platform} images getting tagged 😕 (as seen here: https://github.com/willnorris/addlicense/pkgs/container/addlicense)

I tried doing multi-architecture builds using the the regular docker actions to build the image, and it's definitely possible to do multi-architecture images without the intermediate tags (the qemu tag here: https://github.com/willnorris/imageproxy-test/pkgs/container/imageproxy-test). I'm not sure if that can easily be done in goreleaser?

@willnorris
Copy link
Collaborator

ugh, I really hate how GitHub shows comments sometimes. I actually missed your reply here. sigh

So yeah, I think we should add signing support without goreleaser (at least for the docker images). But seriously, thanks for your patience with me on this PR. It's honestly been a lot of fun to learn about these things.

@developer-guy
Copy link
Author

Hello @willnorris, GoReleaser v1.2.2 has just been released yesterday, and now, GoReleaser is capable of signing releases with a keyless approach using GitHub Actions OIDC flow, and also, another notable feature has been added with v1.2.2 is SBOM support, again, now, GoReleaser can generate an SBOMs for container images by using Syft tool under the hood, please see it all in action on a sample repository:

👉 https://github.com/goreleaser/supply-chain-example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use GoReleaser to build and push container images
2 participants