-
Notifications
You must be signed in to change notification settings - Fork 13
Update Dockerfile #350
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: master
Are you sure you want to change the base?
Update Dockerfile #350
Conversation
|
A small note, if you build the image passing all the docker build \
--build-arg TM_VERSION=0.6.14 \
--build-arg VCS_REF="$(git rev-parse HEAD)" \
--build-arg BUILD_DATE="$(date -u +%Y-%m-%dT%H:%M:%SZ)" \
-t cristiancantoro/task-maker-rust:0.6.14 \
.For dev/testing just build it like this: docker build \
--build-arg TM_VERSION=0.6.14 \
-t cristiancantoro/task-maker-rust:0.6.14 \
. |
ed0ccec to
c2fb113
Compare
edomora97
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.
Overall nice! Thanks for taking the time!
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.
This Docker image is:
- Forced to run based on a tmr release (it downloads the source of a git tag)
- Builds tmr
I think it would be better to either:
- Force the build based on a tmr release, but grab the
.debfile that is already built by the CI.
- The CI needs minor tweaks to publish the debs, but they have already been built
- Allow the build from any commit, or even uncommitted changes.
- Do not download the source, but take it from the context (i.e., slightly change the Dockerfile and the build command)
| Option 1 | Option 2 | |
|---|---|---|
| Speed of the build | Fast | Slow |
| Customizing what packages are installed | Yes | Yes |
| Making code changes | Need a release | Don't even need a commit |
| Dockerfile complexity | Simple | More complex |
| Maintainability issues | Relies on CI building the release into .deb files | Relies on tmr compatibility with Rust version¹ |
¹ If rustup downloads a newer version of Rust, the build may fail.
Given the use case of this Docker image (a quick and easy way of deploying task-maker), I'm biased to solution 1.
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.
Why not both? I have created another Dockerfile where I COPY my local repository and build task-maker-rust. I think it would be nice to provide a image for uses that want to use task-maker-rust as soon as possible and another for the people interested in developing and contributing.
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.
@edomora97 wrote:
Option 1 Option 2 Speed of the build Fast Slow
Regarding the speed of build, this is a bit faster than the previous version (building from source), but it's still not super fast as on my machine it takes ~ 10 minutes (against 16.5 minutes).
tools/docker/Dockerfile
Outdated
| # symlink `task-maker` and `tusk-maker-tools` into /usr/local/bin/ | ||
| USER root | ||
| RUN ln -s /opt/task-maker-rust/"task-maker-rust-${TM_VERSION}"/target/release/task-maker /usr/local/bin/ \ | ||
| && ln -s /opt/task-maker-rust/"task-maker-rust-${TM_VERSION}"/target/release/task-maker /usr/local/bin/task-maker-rust \ | ||
| && ln -s /opt/task-maker-rust/"task-maker-rust-${TM_VERSION}"/target/release/task-maker-tools /usr/local/bin/ |
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'd prefer if this is packaged as a deb, and then installed with the package manager. There many files that needs to be installed, and the full list is already defined in code:
Also, now that I look at the CI I see that there may be problems with the TM_DATA_DIR not being exported in the release pipeline. @veluca93 why aren't you using the same commands as build_release.sh in the release CI? To be honest, I don't exactly remember how that script was used, probably we had a Docker image to package tmr with old system libraries, instead of building a different version for each OS.
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.
Probably because I didn't really remember that script existed :D
We should probably set the variable...
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.
The thing that was strange to me is that the .deb package installs task-maker-rust and task-maker-tools, instead cargo build --release build task-maker and task-maker-tools, that's why I was creating the symlink for both task-maker and task-maker-rust.
tools/docker/Dockerfile
Outdated
| RUN userdel --remove ubuntu | ||
|
|
||
| # create a group and a user called taskmaker, create the home directory | ||
| RUN groupadd ${TM_GID:+-g "${TM_GID}"} taskmaker |
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 we always have called task-maker with the dash, can you add it also to the user name?
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.
To be fair, in the previous version of the Dockerfile the user was called user :-D. I will change it to task-maker.
tools/docker/entrypoint.sh
Outdated
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.
This is getting really complex. Why not migrate it to a Python script? It will be so much cleaner and maintainable 😅
|
Why doesn't the dockerfile just add the APT repository and add the package from there? That seems way simpler than most other alternatives, it should boil down to roughly three commands: |
|
@veluca93 wrote:
When I sat down to write the new |
I'm a bit of a noob, but can you install a specific old version of tmr from the APT repository? |
Yup, see https://askubuntu.com/a/428778 -- tldr: |
- Update base image from ubuntu:18.04 to ubuntu:24.04 (LTS release)
- Update LABELs
- Build `task-maker-rust` from source, since the `.deb` package is no
longer available
- Add support for running multiple workers
- install `task-maker-rust` using released deb package - convert entrypoint.sh to entrypoint.py
c2fb113 to
f4f6d47
Compare
|
Sorry for the delay, this is what I have changed:
I saw that @veluca93's APT repo only contains the latest version of If, for whatever reason, you prefer that the Dockerfile installs the deb from the APT repository then Dockerfile#L71-L78 should be changed to the following: # install task-maker-rust from APT repo
ARG TM_DEB_VERSION="${TM_VERSION}-1~ubuntu-24.04"
ARG TM_DEB_NAME="task-maker-rust_${TM_DEB_VERSION}_amd64.deb"
RUN (test -n "$TM_VERSION" || (echo "Please use --build-arg TM_VERSION=X.Y.Z" >&2 && exit 1)) \
&& . /etc/os-release \
&& echo "deb [signed-by=/etc/apt/keyrings/task-maker-rust.asc] https://artifacts.lucaversari.it/olimpiadi-informatica/task-maker-rust/latest/deb/${UBUNTU_CODENAME} /" \
| tee /etc/apt/sources.list.d/task-maker-rust.list \
&& curl https://artifacts.lucaversari.it/signing-key.asc \
| tee /etc/apt/keyrings/task-maker-rust.asc > /dev/null \
&& apt update \
&& apt install -yy task-maker-rust="${TM_DEB_VERSION}" \
&& rm -rf /var/lib/apt/lists/*(note that the deb package name is |
|
I have also updated |
This PR does the following:
task-maker-rustfrom source, since the.debpackage is no longer available<nproc>-1workers, or you can specify the number with-j)I think the image could be published on Docker Hub and/or distributed with the GitHub container registry.
h/t to @edomora97 for the guidance on the changes.