Skip to content
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

Update Docker configuration files #748

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

arkid15r
Copy link
Collaborator

@arkid15r arkid15r commented Oct 10, 2023

Checklist

  • I have followed the Contributor Guidelines.
  • The code has been thoroughly tested in my local development environment with flake8 and pylint.
  • The code is Python 3 compatible.
  • The code follows the PEP8 styling guidelines with 4 spaces indentation.
  • This Pull Request relates to only one issue or only one feature
  • I have referenced the corresponding issue number in my commit message
  • I have added the relevant documentation.
  • My branch is up-to-date with the Upstream master branch.

Changes proposed in this pull request

Update Docker configuration files to minimize the layers amount and make the code more readable.

upd:
The initial heredocs approach was replaced with RUN commands chained with && \ due to the unsatisfied buildkit requirements. I added missing apt-utils package to resolve debconf: delaying package configuration, since apt-utils is not installed and set DEBIAN_FRONTEND to noninteractive. Also added a couple of cleanup instructions to minimize the image size.

  - Use heredocs format for RUN command
  - Add minor readability improvements
@securestep9 securestep9 self-assigned this Oct 18, 2023
@securestep9
Copy link
Collaborator

This is not backwardly-compatible with Docker v20.10 as it fails with error:
Error response from daemon: dockerfile parse error line 9: unknown instruction: MKDIR

Screenshot 2023-10-23 at 01 17 32

It works in Docker v23.0, however the saving on the image provided by this PR is minimal, so I am not going to merge it just yet

  - Remove heredocs, use `&& \` for chaining
  - Add Docker build tests for 24.0, 23.0, and 20.10 versions
  - Add `build-docker-image` as a requirement for the `test` job
  - Add missing apt-utils
  - Set apt DEBIAN_FRONTEND to noninteractive
  - Add image cleanup instructions
@arkid15r
Copy link
Collaborator Author

It works in Docker v23.0, however the saving on the image provided by this PR is minimal, so I am not going to merge it just yet

Well, that makes sense. I'm surprised it worked at all as heredocs requires buiildkit/buildx. I've changed that part and added a sort of smoke tests for multiple docker engine versions -- 24.0, 23.0, and 20.10 (I guess that's what you were doing manually). It just checks whether the image is buildable. This job is required to be successful for running test job now.

PTAL when you have a chance.

Comment on lines +9 to +17
RUN mkdir -p .data/results && \
apt-get update && \
apt-get install -y $(cat requirements-apt-get.txt) && \
pip3 install --upgrade pip && \
pip3 install -r requirements.txt && \
pip3 install -r requirements-dev.txt && \
apt-get clean && \
rm -rf /root/.cache/* && \
rm -rf /var/lib/apt/lists/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if that's a good idea, it makes it harder to debug for each step if fails, and more time consuming to repeat the process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants