Add Dockerfile with minimal supported Python version (3.10) #16
Conversation
|
This is an interesting addition to the repo, however:
Otherwise, we're open to accepting this, thanks for your contribution! |
| ca-certificates \ | ||
| curl \ | ||
| git \ | ||
| unzip \ |
There was a problem hiding this comment.
ca-certificates: we shouldn't need to install anymore than what the image includes, unless you're telling me it doesn't have any, then this is acceptable. Have we verified that or is this cargo-culted, is my ask.curl: doesn't look like we're using cURL anywhere in this Dockerfile, if so, please remove it.git: all we're using GIt is to download ASDF, for which I'll add comments later.unzip: same as cURL, we aren't using it so this just bloats the image.
There was a problem hiding this comment.
Thanks for the review,
curlis used by ASDF to download latest Terraform available version. We can decide to get rid of it or keep it in the image so that the user can eventually update its Terraform versiongitis used by both the ASDF installation and thestackstools as well (hosted on GitHub). Good point, this might be removed afterunzipis used by ASDF during when installation Terraform version and also by the installation of thestackstool (mainly by some Python dependencies, but not sure)
For ca-certificates, I can't recall, but I think I got TLS warning preventing me to download from GitHub and all Python module dependencies.
For most of them, true, we can decide if we want to keep them or not.
Let me know what you'd prefer.
| @@ -0,0 +1,32 @@ | |||
| FROM python:3.10-slim | |||
There was a problem hiding this comment.
| FROM python:3.10-slim | |
| FROM docker.io/python:3.10-slim |
| && apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false \ | ||
| && rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
I think rm -rf /var/lib/apt/lists/* is all you need here, but I might be wrong on this one.
There was a problem hiding this comment.
You need to purge all installed dependencies pull from the packages we want, as well as unused packages. This is what the line 11 does.
I might have kept the line 11 because I found out that there was no need to uninstall tools provisioned for installation what we want (stacks and ASDF).
| ARG ASDF_VERSION="v0.16.7" | ||
| ENV ASDF_DATA_DIR="/opt/asdf-data" | ||
| ENV ASDF_INSTALL_DIR="/opt/asdf" | ||
| ENV PATH="${ASDF_DATA_DIR}/shims:${ASDF_INSTALL_DIR}/bin:${PATH}" | ||
|
|
||
| RUN git clone \ | ||
| --branch "${ASDF_VERSION}" \ | ||
| --depth=1 \ | ||
| https://github.com/asdf-vm/asdf.git "${ASDF_INSTALL_DIR}" \ | ||
| && asdf --help | ||
|
|
||
| RUN asdf plugin add tfswitch \ | ||
| && asdf install tfswitch latest \ | ||
| && asdf global tfswitch latest \ | ||
| && tfswitch --latest |
There was a problem hiding this comment.
This doesn't belong to Stacks and shouldn't be part of this image.
May I suggest removing this from the Stacks Dockerfile and then building an image FROM stacks that adds these tools if you need them? Otherwise we're bloating the image with unrelated stuff.
There was a problem hiding this comment.
You don't need Terraform for running stacks ?
Indeed, tfswitch is not needed. It is only a convenient way to get the latest version of Terraform. I can uninstall it, definitely.
| && asdf global tfswitch latest \ | ||
| && tfswitch --latest | ||
|
|
||
| RUN apt-get update \ |
There was a problem hiding this comment.
This apt-get update is unnecessary and defeats the purpose of the rm -rf /var/lib/apt/lists/* above.
There was a problem hiding this comment.
What do you mean ? If there is no cache file in /var/lib/apt/lists/, you can't run an apt-get install command if you did not run an apt-get update before to get updated lists.
| && tfswitch --latest | ||
|
|
||
| RUN apt-get update \ | ||
| && pip install git+https://github.com/tchemineau/cisco-thousandeyes-stacks.git@provides-dockerfile \ |
There was a problem hiding this comment.
- Let's switch to
uvinstead ofpip, it's what we want to primarily support going forward (even though we're keepingpipcompatibility. - This line installs stacks from your own branch on your own fork, we want to use the code in the Docker build context instead so this Dockerfile is not bound to (1) any network services, like GitHub; (2) any specific Git repository; and (3) any specific Git branch.
There was a problem hiding this comment.
Thanks. Good point for the URL. Only submitted to get a review first, indeed this should be from master repo.
| @@ -9,7 +9,6 @@ dependencies = [ | |||
| "cryptography>=43.0.3", | |||
| "deepmerge>=2.0", | |||
| "gitpython>=3.1.43", | |||
| "importlib>=1.0.4", | |||
There was a problem hiding this comment.
Why are we removing importlib here?
There was a problem hiding this comment.
Because the installation of the stacks tool using pip3 fails otherwise on dependency requirements.
Description
The idea is to provide a minimal
Dockerfileto build a container image that can runstackstool properly.I would like to reuse that mechanism to eventually publish the container image to docker hub, and use it in the CI to do some more advanced testing.
For my use-case, I found out the result container image very useful to quickly run
stacks encryptcommand line.Let me know your thought, and if that could be useful.
Type of Change
Checklist