search

jitesoft / docker-lighttpd

Lighttpd alpine linux.
MIT License
8 stars 0 forks source link

Patch 1 #2

Closed Macleykun closed 2 years ago

Macleykun commented 2 years ago

Type of change

Description

Hopefully remove the script and just use the CMD and ENTRYPOINT instead

Related issue

Motivation

Improves a little, more a QoL i geuss.

Checklist

i wanted to test this, but somehow i kept getting this error (even with your fork) failed to compute cache key: "/out" not found: not found (second step 2/2?)

i do request ya test it yourself!

Macleykun commented 2 years ago

also: https://www.ctl.io/developers/blog/post/dockerfile-entrypoint-vs-cmd/

Johannestegner commented 2 years ago

Hi!

Thank you for the issue!

I'll start with the reason you get the error when trying to build the image:

Most images maintained by the Jitesoft org makes use of scripts which builds all binaries outside of the container. This is mainly to lower the time it takes to build them, make use of stuff like ccache and to make use of the native architectures that we got rather than build all through qemu. So the out directory that is mounted in the first step of the RUN command only exists in the pipeline (and as an artifact for a few hours).

I have been considering publishing the artifacts as binaries somewhere so that the image can fetch them through a standard wget/curl command, but that will require quite a lot of re-writes in most pipelines that are already running, so it have not happened yet.

When it comes to the actual issue, changing from CMD to Entrypoint, I could not agree more. The initial lighttpd image was one of the first one that was published by the jitesoft organisation, and was not as greatly throught through as many of our other images. The reason it have not been changed is primarily because it's used at quite a lot of different places and changing the functionality to drastic might interrupt peoples environments.

When it comes to this specific image, I do think it's possible to make it work quite easily though.

The general "rule" in later images, is that we always want to have an "entrypoint" script. It's most often not a very big one, could even be as simple as:

#!/bin/ash
exec "$@"

Or as an example: https://github.com/jitesoft/docker-nginx/blob/master/entrypoint

When it comes to testing stuff like this, I would say that the easiest way would be to just "extend" the image and test it:

FROM jitesoft/lighttpd:latest

ENTRYPOINT ["lighttpd"]
CMD ["-D","-f","${CONFIG_FILE}"]

Which produces the following error:

$ docker run --rm -i -t test-lighty
2022-07-08 11:47:18: (configfile.c.2158) opening configfile ${CONFIG_FILE} failed: No such file or directory

The problem being that the CMD does not interpret environment variables as a standard script. It's not unlikely that is the reason the decision to make 'startup' a script instead of a standard command.

One solution could be to replace the default config file with whatever content is in the file that the environment variable points to in the entrypoint script, that way we could use a static file path to the config and run with a more "standard" Entrypoint/CMD solution.

I will have to think a bit about this and test some potential solutions to make sure that the image does not have any direct breaking changes, and will leave the issue open for now!

Macleykun commented 2 years ago 
Most images maintained by the Jitesoft org makes use of scripts which builds all binaries outside of the container. This is mainly to lower the time it takes to build them, make use of stuff like ccache and to make use of the native architectures that we got rather than build all through qemu.
So the out directory that is mounted in the first step of the RUN command only exists in the pipeline (and as an artifact for a few hours).

Make sense :) felt like there was a part of the puzzle missing!

I have been considering publishing the artifacts as binaries somewhere so that the image can fetch them through a standard wget/curl command, but that will require quite a lot of re-writes in most pipelines that are already running, so it have not happened yet.

Understandable, tbh aslong as you are able to test it, it's fine i think :). Don't expect like a dozens of pr's every week/month so it'll probably be fine as is!

About the entrypoint, also true! I personally prefer to do it in one line (so having your example i would opt in to do it in the Dockerfile itself). Or if it's the url i would indeed go for the script methode!

And yes, i could've just ended it haha, not sure why i didn't come up with that :P.

Aah, that might explain the reason of the script even tho it's one single line! Perhaps i should rather opt in to make it a nice entrypoint script!

I do believe based on your explanation that this PR might not be that beneficial and the work that might be needed might not be worth it in the end.

Up to you if you want to keep this PR/issue open ofc :) Thanks for the explaintion it surely has helped me to understand this issue more (hopefully) https://github.com/jokob-sk/Pi.Alert/issues/10 (or more so that the container uses /dev/null to keep running, which makes the container hard to stop).

Johannestegner commented 2 years ago

I do believe based on your explanation that this PR might not be that beneficial and the work that might be needed might not be worth it in the end.

The PR might be worth closing, but the underlying issue that you created it for is very valid!

https://github.com/jokob-sk/Pi.Alert/issues/10 (or more so that the container uses /dev/null to keep running, which makes the container hard to stop).

Yes, some programs basically ignores the signal that "ctrl + c" sends, I think that the lighttpd image currently works in that way, and I will have to take a closer look on if there is anything that can be done to allow it to close gracefully while still listen to ctrl + c.

Been testing around a bit and made this small test image:

Dockerfile:

FROM registry.gitlab.com/jitesoft/dockerfiles/lighttpd:latest

COPY ./entrypoint /usr/local/bin
RUN chmod +x /usr/local/bin/entrypoint

ENTRYPOINT ["entrypoint"]
CMD ["-D"]

Entrypoint:

#!/bin/ash
set -e

if [ "${1#-}" != "$1" ]; then
  set -- lighttpd "$@" -f "${CONFIG_FILE}"
fi

exec "$@"

Entrypoint check if the "CMD" sends a parameter (-XXX) (with some ash-magick pattern matching test) and if so sets the command to use it and appends -f ${CONFIG_FILE} and if it's not a param it would just run the command passed.

I think this would keep the image working as before, but with a more useful entrypoint and easier to work with CMD.

That would allow the user to run docker run <image> ls and ls would run right away (without entrypoint override) as well as just docker run <image> and it would run the default command.

This would require a minor re-write of the cgi image as well, but I think that it would just basically add the above script to the end of it's own entrypoint script and replace the CMD in the same manner.

I'll have to test it a bit more and see if it breaks anything that I haven't thought of, but it looks kinda promising ;)

Macleykun commented 2 years ago

Oh indeed! Looks better! Agreed with it :) btw isn't it easier to set the execution bit with git instead? Of is it more so to be absolute certain the execution bit is set at all times?

Johannestegner commented 2 years ago

What do you mean with "execution bit"? :)

Macleykun commented 2 years ago

What do you mean with "execution bit"? :) So if we wanna execute a file, we do chmod u+x file The u+x (or the x) is the execution and bc it flips smth from off to on we call it a bit (that show i see it) In got ya can set that bit, but if ya download with zip those are gone.

Johannestegner commented 2 years ago

Aaah I understand!

Well yes, I usually (try to) check in those type of scripts with the +x flag through git, whilst I also prefer to run it in the docker image in case it was forgotten or if there is any difference in how the container handles the script. Basically a sanity check :)

Macleykun commented 2 years ago

is good :)

Johannestegner commented 2 years ago

Fixed this in 3bb79054cd6cc2d6e8f56b340aecfcc777b322df (currently building). Tested locally with overridden images and seems to work fine, will test further when image is published.

Closing this issue now. Thank you again! :)