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

Build via multi-stage, update to Go 1.19, Alpine 3.16 #30

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

tianon
Copy link
Contributor

@tianon tianon commented Oct 14, 2022

This is possible now thanks to docker-library/bashbrew#43 -- there are some more dependencies before we can take this all the way linked from docker-library/bashbrew#43 (comment), but we're close enough that we can start testing at this level. 👍

@tianon tianon requested a review from yosifkit October 14, 2022 17:29
@tianon
Copy link
Contributor Author

tianon commented Oct 14, 2022

Heh, at least #29 works to successfully catch the bug! I guess we need a new Notary release before we can actually do this? 😬

@tianon tianon marked this pull request as ready for review October 14, 2022 23:32
@tianon tianon force-pushed the multistage branch 2 times, most recently from d195fd7 to 44c3bbd Compare October 14, 2022 23:56
@tianon
Copy link
Contributor Author

tianon commented Oct 15, 2022

Welp, apparently I don't understand notaryproject/notary#1602 as well as I thought I did because that manual cherry-pick of the jose2go module update didn't fix the failing tests. 🙈

@jonnystoten
Copy link
Member

We might also need to cherry-pick notaryproject/notary#1635 for this to work on Go 1.17+. Hopefully we'll make a new notary release soon which will include all of these fixes!

@tianon
Copy link
Contributor Author

tianon commented Oct 17, 2022

Dang, that was an easy one to cherry-pick 😅 (and it looks like it worked! 🤘)

Do you think these changes are pretty reasonable for us to backport into new builds of the older release this way? Do you have any objectionary thoughts on the PR in general? 😇

@jonnystoten
Copy link
Member

Awesome, glad that was painless 🎉

Do you think these changes are pretty reasonable for us to backport into new builds of the older release this way?

I'm not sure, I think I'll defer to you on that! Do we often backport these kind of changes for old releases, or is notary unique? I think these changes are pretty harmless but I guess it could be confusing for a user if, say, the official image works and the release from notary does not.

Hopefully we can have a notary release this week so it might be better to wait for that, but I appreciate we've been saying it will happen soon for a long time! 😬

My only other thought on this PR is that it might be nice to have a single Dockerfile with three stages, one that does the build step, and then two more stages for the server and signer, each of which just copies over the appropriate binary. This way we wouldn't need to duplicate the build step. I'm not sure if this is possible with the rest of the build pipeline though, and definitely not for this PR!

@yosifkit
Copy link

Dockerfile with three stages

The Official Images build pipeline does not support a --target, so it would only be able to push the final stage.

@tianon
Copy link
Contributor Author

tianon commented Oct 18, 2022

Yeah, with only two files it should be pretty easy to keep them in sync, but templating would be my recommended solution if we want something more explicit. 👍

Given "Notary" doesn't release binaries, this is probably fine for us to release, but if you really do believe a new release is imminent I don't see a lot of damage in waiting instead. 😄

@jonnystoten
Copy link
Member

OK, yeah I thought the pipeline might not support that yet.

Ahh yes it looks like the 0.7.0 release was weird as there's no actual 'release' in GitHub, just a tag 🤔 It looks like previous releases have got binaries linked from the GitHub releases, I assume that's the intended correct way to release 🤔

Yes I think it's fine to release this 👍

@tianon
Copy link
Contributor Author

tianon commented Oct 24, 2022

Alright, let's do it! 😄

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.

3 participants