zenhack / simp_le

Simple Let's Encrypt client
GNU General Public License v3.0
224 stars 38 forks source link

Docker fixes #74

Closed faircopy closed 7 years ago

buchdag commented 7 years ago

Thanks for the fixes !

I only have two remarks:

zenhack commented 7 years ago

Re: bash, @buchdag, do you typically actually launch a shell in the container as part of your usual workflow?

My own inclination would be to leave bash out, but given the size difference I don't care too much one way or another.

buchdag commented 7 years ago

@zenhack in normal, everyday use of a container: no.

But as soon as you have to troubleshoot something inside the container you have to launch a shell, so my first reflex in that case is to do docker run --rm -it somecontainer bash or something similar.

It might be more obviously useful for containers that provide a continuously running service rather than a one shot command like this one.

That's really just a matter of personal preference anyway, I'd prefer having it but I'm perfectly fine with removing it too.

I'm more skeptical about the need to do an apk upgrade.

zenhack commented 7 years ago

Yeah, if it were a long running think I might feel differently.

faircopy commented 7 years ago

Regarding bash, these are the features of an interactive shell that I use for troubleshooting, the default shell included in Alpine (ash) has them:

See: docker run --rm -it zenhack/simp_le sh


As for apk upgrade, even though the official alpine:3.6 image on Docker Hub was updated 2 weeks ago, its source repository on GitHub was last updated 3 months ago. By upgrading we apply the following changes at the moment:

That's a lot of musl patches. I don't understand the security implications of those, so I personally tend to choose upgrading even though doing so comes with a 2.9 MB increase in image size. I can't tell whether the maintainers of the alpine image are being lazy with the updates or are purposefully delaying it because there are no security issues. I removed this commit from the pull request, because this is really an upstream issue (or more probably non-issue).

buchdag commented 7 years ago

@faircopy agreed, bash isn't required and I'm being a bit lazy wanting to use it instead of sh.

zenhack commented 7 years ago

Ok, I'm going to go ahead and merge this.

FWIW: it seems odd to me to go to the trouble of backporting a patch if it's not actually important enough to update the image. But then, I'm an Arch user, Arch has a fairly clear policy of expecting upstream to deal with its own bugs.

buchdag commented 7 years ago

@zenhack seems like dockerhub isn't building zenhack/simp_le on new commits yet (ie automated build isn't working), the registry says the image was last pushed a month ago and the Dockerfile does not have @faircopy fixes.

Do you need help with dockerhub config ?

zenhack commented 7 years ago

Hm, odd. The settings look like they're pretty clearly set to build on push. I just triggered the build manually.