-
Notifications
You must be signed in to change notification settings - Fork 0
fix: docker makefile recipies and pre-commit configuration #12
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: main
Are you sure you want to change the base?
Conversation
efcaguab
commented
Dec 18, 2025
- Improve dependencies of docker recopies in Makefile (prevents need to manually create volume)
- Fix pre-commit dependencies to include codespell
- Add docker-volume dependency to docker-compose recipes - Add codespell[toml]>=2.4 to pre-commit dependencies - Add PYTHON and PIP variables to Makefile for flexibility
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.
Pull request overview
This PR improves the Makefile and pre-commit configuration by automating docker volume creation and fixing codespell dependencies. The changes make docker-based workflows more robust by automatically creating required external volumes, and ensure codespell is properly available in pre-commit hooks. Additionally, the Makefile is refactored to use centralized PYTHON and PIP variables for better maintainability.
Key Changes:
- Added
docker-volumeas a prerequisite to docker-related targets to automatically create the external GCP volume - Introduced PYTHON and PIP variables to centralize Python command invocations throughout the Makefile
- Added codespell[toml]>=2.4 to pre-commit hook's additional_dependencies
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Makefile | Added PYTHON/PIP variables, made docker targets depend on docker-volume, standardized Python command usage |
| .pre-commit-config.yaml | Added codespell[toml]>=2.4 to additional_dependencies for the codespell hook |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| .PHONY: docker-ci-test ## Runs tests using prod image, exporting coverage.xml report. | ||
| docker-ci-test: | ||
| docker-ci-test: docker-volume |
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 don't believe this should be here. The idea of this command is to execute the unit tests in isolation against the installed package in a docker container. This is used in the CI/CD. Why would you need the volume with authentication information?
I would say the same for reqs and reqs-upgrade.
It could be useful in docker-shell if you didn't run docker-gcp before.
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 added these pre-reqs because reqs and reqs upgrade would fail without creating the volume first. But you're right that we should't really need a volume with credentials to do these things.
Perhaps the real fix is adding a separate dev service in the docker compose that doesn't need GCP? Or just remove the mount from the dev service if nothing else needs it?
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.
@efcaguab Oh I see. Yes, I think the correct solution is to add a separate service for the requirements:
req:
build:
context: .
target: dev
volumes:
- ".:/opt/project"
entrypoint: /bin/bashI think we need a dev service with the volume because some people use the docker container to develop (I don't...).