Dockerfile upgrade #234
Dockerfile upgrade #234
Conversation
|
Hey, @tetofonta, thanks for this! I forgot I had to approve the CI run to check the solution, and there are also a couple of PRs I need to check upon before merging anything else, but I'll try to get back to you as soon as possible. |
Dockerfile
Outdated
| #RUN set -ex; echo "mosquitto_sub -t '\$SYS/#' -C 1 -p 1883 | grep -v Error || exit 1" > /healthcheck.sh | ||
| RUN set -ex; ldconfig; | ||
|
|
||
| #HEALTHCHECK --interval=30s --timeout=30s --start-period=5s --retries=3 \ | ||
| # CMD ["/bin/sh", "-c", "/healthcheck.sh"] | ||
|
|
There was a problem hiding this comment.
Could you explain what are these healtchecks bits that are commented?
There was a problem hiding this comment.
That's an example for doing health checking on the process, but this is very configuration dependent so I preferred to leave it out and really leave just an hint for anyone who may require it.
For example it does not account for authentication nor tls. In this particular case the user should configure another listener bound on localhost without any authentication in order to use this particular health check command.
There was a problem hiding this comment.
Gotcha, thanks for clarifying. In that case, I'd prefer to remove it entirely or add comments explaining what it is about.
There was a problem hiding this comment.
I made a new commit removing the comments.
Do you think the argument "health checking" should be mentioned in the readme?
Dockerfile
Outdated
| RUN set -ex; apt-get update; apt-get install -y wget build-essential cmake libssl-dev libcjson-dev | ||
|
|
||
| # Get libwebsocket. Debian's libwebsockets is too old for Mosquitto version > 2.x so it gets built from source. | ||
| RUN export LWS_VERSION=2.4.2 && \ | ||
| wget https://github.com/warmcat/libwebsockets/archive/v${LWS_VERSION}.tar.gz -O /tmp/lws.tar.gz && \ | ||
| mkdir -p /build/lws && \ | ||
| tar --strip=1 -xf /tmp/lws.tar.gz -C /build/lws && \ | ||
| rm /tmp/lws.tar.gz && \ | ||
| cd /build/lws && \ | ||
| RUN set -ex; \ | ||
| wget https://github.com/warmcat/libwebsockets/archive/v${LWS_VERSION}.tar.gz -O /tmp/lws.tar.gz; \ | ||
| mkdir -p /build/lws; \ | ||
| tar --strip=1 -xf /tmp/lws.tar.gz -C /build/lws; \ | ||
| rm /tmp/lws.tar.gz; \ | ||
| cd /build/lws; \ |
There was a problem hiding this comment.
Out of curiosity, why are you changing all instances of && to ; ?
I would think we would WANT the build to fail if any step had an error, but here it would keep going 🤔
There was a problem hiding this comment.
It's for readibility. set -ex will do the same thing by exiting in case of non zero exit code of any command in the script (and will print it out before executing)
There was a problem hiding this comment.
Hmm, I was not familiar that set -ex changes the behavior that way. I would think && is more expected by most users, since it's used in most docker examples. Regardless, it seems like you're mixing up some style changes (switching && to ;) which should probably be in it's own commit, to more easily distinguish the actual logic changes 🤔
There was a problem hiding this comment.
I'm primarily asking because I had not seen that construct before and was asking to learn myself. It's ultimately up to iegomez what he's comfortable with 😄
There was a problem hiding this comment.
Oh dang, I have forgotten line 90 =( You are right.
As for my understanding, docker executes all the run programs as /bin/sh -c '<run command>' (because if you leave a RUN entry without commands it errors out with /bin/sh: 0: -c requires an argument)
You can see this behavior of set -e/set -ex in a linux shell:
/bin/sh -c "set -ex; echo 'a'; [ 1 -eq 2 ]; echo 'b'"
this will run until [ 1 -eq 2 ] returns non zero so it does not execute the second echo and it is the same as
/bin/sh -c "echo 'a' && [ 1 -eq 2 ] && echo 'b'"
I'm making a commit fixing those &&, sorry for the mistake 😇
There was a problem hiding this comment.
@tetofonta can you do a rebase of your commit and split it up into logical commits that we can more easily review? If you want to change all the && to set -ex and ; then those need to be their own commit, so that the actual changes can be reviewed by themselves.
There was a problem hiding this comment.
Btw, @kfdm, @tetofonta , would you be interested in being maintainers of this project?
I ask because I don't look at it much these days, every few months I get some time and start looking at issues and PRs, but I could certainly rely on a couple more hands watching it.
No pressure though, feel free to say no. And I really mean that, helping with a particular issue is great help already, I'm not trying to drag you into something you might not want.
Cheers!
There was a problem hiding this comment.
I definitely want to see this project continue (and I'm using it in my own project ) though Go is not a language I use on a regular basis, and the http auth is the only one I'm familiar with, so I'm not sure if I'm one of the most qualified. I could mostly only check minor Docker or http plugin requests.
That said, for the most part this library seems to do what it does well, and may not actually need a lot of attention besides small bug fixes, unless you had some other long term plans you wanted to attempt.
There was a problem hiding this comment.
He, seems like your project is private, I got a 404.
And yeah, that's fair, the project tends to be quiet most of the time until someone brings up some issue or a feature request and depending on the time I might not be too responsive. So let's see how it goes for now, and thank you very much for your response.
There was a problem hiding this comment.
Hmm, thought I had already made it public. It should be public now.
|
As said before, I fixed the style for Dockerfile using now |
|
Heads-up: I'll merge this tomorrow unless there's any concern. |
|
Functionality it's probably all fine. All my nits were basically about formatting and git history 👍 |
Git history wise, I'm gonna squash and merge anyway. Edit: And I actually accidentally just merged as is. 🤦 |
Hi!
I've refactored the Dockerfile removing duplicate entries.
libwebsocketwas compiled twice in two images but the second time was not required because the library is linked statically in the binary. (or at least it seems so, everything is working so...)Also the final image now contains
mosquitto_*utils and the required libraries (libmosquitto.soandlibmosquittopp.so) enabling health checking of the container and some more scripting/automation during deployments.