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

Fix 125 #144

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix 125 #144

wants to merge 2 commits into from

Conversation

josvo
Copy link

@josvo josvo commented Mar 19, 2021

What is the purpose of this change? What does it change?

This PR fixes #125 by creating docker images on every git tag and automatically push them to Dockerhub.
To make that work, the Dockerfile was changed to a multi-stage build, which makes it also easier because you don't need to have go installed when doing a docker build locally.
Please create the two secrets in Settings / Secrets

  • DOCKER_USERNAME
  • DOCKER_TOKEN
    Whenever you tag a new release, the workflow is triggered and creates an image for linux/amd64 and linux/arm/v7. This image is also tagged as latest.
    => Please tell if you want additional platforms supported.

For the Dockerfile, I tried to follow best practices - I know there is a bit of a range there. I prefer to have all additional files in a separate directory, that's why I changed the existing part of the docker build.

Was the change discussed in an issue or in the forum before?

Closes #125 (as soon as there is a new tag after this PR is merged).

Checklist

  • I have enabled maintainer edits for this PR
  • I have added tests for all changes in this PR (none needed)
  • I have added documentation for the changes (in the manual) (will happily do it if you think this PR is a good idea)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here) (see above)
  • I have run gofmt on the code in all commits (not needed)
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review (Feedback welcome!)

@wojas
Copy link
Contributor

wojas commented Mar 24, 2021

Nice! Are the docker secrets already available in this repo?

Copy link
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

I prefer the (less extensive) Dockerfile changes made in #145.

Regarding the docker hub secrets, I don't think that these are currently available to the CI. That would be up to @fd0 to decide on and provide those.

@@ -1,16 +1,23 @@
FROM alpine
FROM golang:alpine3.13 AS builder
LABEL stage=builder
Copy link
Member

Choose a reason for hiding this comment

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

Is that label necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Well no, it is a label. I wanted to make clear that this step is only needed for building the software. But I can delete it if you want.

@@ -1,16 +1,23 @@
FROM alpine
FROM golang:alpine3.13 AS builder
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason to pin alpine releases?

Copy link
Author

Choose a reason for hiding this comment

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

Reproducible builds and best practice. But here also: Not really needed, so can be reverted if preferred in the original way.

COPY docker/delete_user /usr/bin/
COPY docker/entrypoint.sh /entrypoint.sh
COPY rest-server /usr/bin
COPY docker/. .
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific need to change the location where the helper scripts / entrypoint.sh are stored? I've been bitten a few times by container updates which "just" changed a few paths, but caused my derived containers to break.

Copy link
Author

Choose a reason for hiding this comment

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

This is just my personal preference: Having all the additional stuff in the same location. But that is no "must". There also doesn't seem to be a best practice for that - so I can do it the way you want.

@josvo
Copy link
Author

josvo commented Mar 28, 2021

I prefer the (less extensive) Dockerfile changes made in #145.

So what about merging #145 first and then I could rebase so that this PR only provides the github actions part?

Regarding the docker hub secrets, I don't think that these are currently available to the CI. That would be up to @fd0 to decide on and provide those.

The secrets have to exist first. Without them, this PR doesn't make much sense. That's up to you to decide.

@MichaelEischer
Copy link
Member

I've merged #145 instead of the Dockerfile changes here.

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.

Restic REST-Server Image for ARM-Architectures on Docker-Hub
3 participants