Skip to content
This repository has been archived by the owner on Jun 26, 2024. It is now read-only.

Dockerfile improvements #11

Merged
merged 8 commits into from
Sep 12, 2022

Conversation

mcdonnnj
Copy link
Member

@mcdonnnj mcdonnnj commented Sep 9, 2022

🗣 Description

This pull request updates the Dockerfile for the project to follow Docker and Python best practices. There's also some cleanup around creating the unprivileged user.

💭 Motivation and context

We always try to follow best practices for the languages and tools we use and so this project should be updated to align with that goal.

🧪 Testing

Automated tests pass. I can build a local image and successfully invoke the image with --help as an argument.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.

It's better to combine related commands when possible to both logically
group them and to consolidate Docker image layers.
Rather than making the script executable and using it as an entrypoint
we instead call python3 to invoke the script as the entrypoint.
@mcdonnnj mcdonnnj added the improvement This issue or pull request will add or improve functionality, maintainability, or ease of use label Sep 9, 2022
@mcdonnnj mcdonnnj self-assigned this Sep 9, 2022
Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

These all look like good improvements to me.

Do any of the CISA_USER cleanup items (or anything else) need to be backported to the skeleton?

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

👍 👍

Base default values on the user and uid instead of a mix of values.
Instead of manually running a chown command to ensure ownership we
instead assign ownership as part of copying the files into the image.
Since they are managed in the Dockerfile we add version pins for the
pip, setuptools, and wheel packages.
Cleanup, correct, and streamline the commenting in the Dockerfile.
Instead of relying on the pip command we instead call it as a module
from the python3 command. This ensures that pip is installing packages
in the same Python version/environment as we use to run the
`email-update.py` script.
@mcdonnnj mcdonnnj force-pushed the improvement/update_Dockerfile branch from 312f295 to 2d59ea9 Compare September 12, 2022 04:50
@mcdonnnj
Copy link
Member Author

These all look like good improvements to me.

Do any of the CISA_USER cleanup items (or anything else) need to be backported to the skeleton?

I've been mulling a number of updates to cisagov/skeleton-docker and those changes are included in that list. You should see some PRs start to roll in this week to build a large update to that skeleton.

@mcdonnnj mcdonnnj merged commit e047298 into improvement/modernize_project Sep 12, 2022
@mcdonnnj mcdonnnj deleted the improvement/update_Dockerfile branch September 12, 2022 14:47
@mcdonnnj mcdonnnj mentioned this pull request Feb 15, 2023
10 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants