-
Notifications
You must be signed in to change notification settings - Fork 33
Add Dockerfile #115
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
base: main
Are you sure you want to change the base?
Add Dockerfile #115
Conversation
willholley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally looks good, though I think we can remove tini. Is there a plan to publish this on a cadence with GitHub Actions or similar?
| # Install dependencies (Tini) | ||
| RUN set -ex; \ | ||
| apt-get update; \ | ||
| apt-get install -y --no-install-recommends tini; \ | ||
| rm -rf /var/lib/apt/lists/*; \ | ||
| tini --version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need tini any more with modern container runtimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is right, the Docker API 1.25+ provides the --init flag for docker run to implement the same functionality. However, that is what makes me doubtful: this is not enabled by default and it has to be explicitly requested by the user or the environment that integrates this image, for example Kubernetes. But why should they know about this — this is an implicit assumption makes the whole system a bit more brittle. Adding tini explicitly for the container image could remove this worry and potential source of bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this comment and assumed tini was still useful.
When I went to confirm it could be removed though, I found documents saying that Kubernetes in particular doesn't automatically use tini or equivalent as PID 1, so I also wonder if it might be more robust to leave it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's no harm in it but it's not required if you are handing SIGTERM correctly and not spawning subprocesses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, Clouseau will not spawn subprocesses but later it might be — again, here would be planting an implicit assumption that the authors of the code have to be aware of to avoid bugs. The correct handling of SIGTERM is assumed but has not been fully verified. Brittle system as I said above.
Create multi-stage Docker image that fetches Clouseau release artifacts and runs with Eclipse Temurin JRE 21.
Currently the CI is failing when attempting to download Python 3.12.7,
```
Error:
0: Failed to install core:[email protected]:
0: HTTP status server error (503 Service Unavailable) for url (https://github.com/astral-sh/python-build-standalone/releases/download/20241016/cpython-3.12.7+20241016-x86_64-unknown-linux-gnu-install_only_stripped.tar.gz)
```
so upgrade to a version that is known to exist.
Create multi-stage Docker image that fetches Clouseau release artifacts and runs with Eclipse Temurin JRE 21.