Skip to content

Add Dockerfile with minimal supported Python version (3.10) #16

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Dockerfile with minimal supported Python version (3.10) #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
FROM python:3.10-slim
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FROM python:3.10-slim
FROM docker.io/python:3.10-slim


RUN REQUIRED_PKGS=' \
ca-certificates \
curl \
git \
unzip \
Comment on lines +4 to +7
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review,

  • curl is 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 version
  • git is used by both the ASDF installation and the stacks tools as well (hosted on GitHub). Good point, this might be removed after
  • unzip is used by ASDF during when installation Terraform version and also by the installation of the stacks tool (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.

' \
&& apt-get update \
&& apt-get install --no-install-recommends --yes ${REQUIRED_PKGS} \
&& apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false \
&& rm -rf /var/lib/apt/lists/*
Comment on lines +11 to +12
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rm -rf /var/lib/apt/lists/* is all you need here, but I might be wrong on this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment on lines +14 to +28
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


RUN apt-get update \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This apt-get update is unnecessary and defeats the purpose of the rm -rf /var/lib/apt/lists/* above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

&& pip install git+https://github.com/tchemineau/cisco-thousandeyes-stacks.git@provides-dockerfile \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Let's switch to uv instead of pip, it's what we want to primarily support going forward (even though we're keeping pip compatibility.
  • 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Good point for the URL. Only submitted to get a review first, indeed this should be from master repo.

&& stacks --help
1 change: 0 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ dependencies = [
"cryptography>=43.0.3",
"deepmerge>=2.0",
"gitpython>=3.1.43",
"importlib>=1.0.4",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing importlib here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the installation of the stacks tool using pip3 fails otherwise on dependency requirements.

"jinja2>=3.1.4",
"packaging>=24.2",
"python-hcl2<6.0.0", # https://github.com/amplify-education/python-hcl2/issues/183
Expand Down