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

Related to issue #83: Added support for container image #105

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

tuxiem
Copy link

@tuxiem tuxiem commented Dec 15, 2023

Added:
dockerfile
docker.md documentation on usage and build-it-yourself
workflow for building and pushing container image to gchr.io

Might need edits in .github/workflow/docker.yml to work, if the ${{ secrets.GITHUB_TOKEN }} needs permissions.

Copy link
Owner

@patrickdappollonio patrickdappollonio left a comment

Choose a reason for hiding this comment

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

Hey @tuxiem!

Thank you SO MUCH for your contribution and apologies I couldn't get to this faster. This is great so far, I only have a handful of concerns, namely whether using alpine is a good idea, and perhaps switching into something else.

Additionally, this project uses .goreleaser.yml but the code you proposed does not take advantage of that. Instead it defines its own flow. You could leverage goreleaser to do most of the heavy lifting for you. Another project of mine creates a dockerfile like this:

https://github.com/patrickdappollonio/http-server/blob/master/Dockerfile

And then it's referenced from goreleaser like this:

https://github.com/patrickdappollonio/http-server/blob/master/.goreleaser.yml#L29-L41

So then both testing and releasing pipelines are just a matter of using goreleaser each time, like here (for a version release) or here for a PR review test (it's called Dry-run goreleaser).

Let me know what you think! If you think this is something you could do, I will be more than happy to take it and would appreciate your help tremendously! If not, I completely understand it. What I would do then is take over your pull request to add the additional changes, then add the remaining ones, so you're credited where it's due.

Let me know!

.github/workflows/docker.yml Outdated Show resolved Hide resolved
Comment on lines +3 to +5
on:
push:
branches: ['main']

Choose a reason for hiding this comment

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

We might need to have a way to test building (but not releasing) the container for commits, so people know nothing breaks on the container side whenever they propose a change.

Copy link
Author

Choose a reason for hiding this comment

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

I'll need to investigate on that part

.github/workflows/docker.yml Outdated Show resolved Hide resolved
with:
context: .
push: true
tags: ${{ steps.meta.outputs.tags }}

Choose a reason for hiding this comment

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

For this, I'm a bit opinionated, but four versions should be released if, say, we're releasing version 3.7.4 (as an example, we don't have that version but let's assume we do):

  • latest, as in overriding the latest tag
  • v3
  • v3.7
  • and finally v3.7.4

This is because this repo tries to follow Semver, so people that need the guarantee that no major breaking change appear in a minor change can confidently pull the newer version of the image. If I ever do, that's on me, not on the people maintaining these apps deployed to environments.

I do something similar here: https://github.com/patrickdappollonio/http-server/pkgs/container/docker-http-server although with no minor version, just major and patch, and this has proven to work in corporate environments where that stability can be called out during usage.

Dockerfile Outdated Show resolved Hide resolved
docs/docker.md Outdated Show resolved Hide resolved
docs/docker.md Outdated Show resolved Hide resolved
docs/docker.md Outdated Show resolved Hide resolved
docs/docker.md Outdated Show resolved Hide resolved
docs/docker.md Outdated Show resolved Hide resolved
@tuxiem
Copy link
Author

tuxiem commented Dec 19, 2023

@patrickdappollonio I will review this when I have some time - I expect it to be around the new year.

@patrickdappollonio
Copy link
Owner

No problem and completely understandable! Happy to discuss if you have any questions after the new year! Thanks again for your help and willingness.

patrickdappollonio and others added 11 commits January 8, 2024 01:13
Co-authored-by: Patrick D'appollonio <930925+patrickdappollonio@users.noreply.github.com>
Co-authored-by: Patrick D'appollonio <930925+patrickdappollonio@users.noreply.github.com>
Co-authored-by: Patrick D'appollonio <930925+patrickdappollonio@users.noreply.github.com>
Co-authored-by: Patrick D'appollonio <930925+patrickdappollonio@users.noreply.github.com>
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.

None yet

2 participants