-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: add Docker support for Folksonomy API #254
base: main
Are you sure you want to change the base?
Conversation
- Create multi-stage Dockerfile for efficient image building - Configure docker-compose.yml with PostgreSQL service - Set up PostgreSQL on alternative port (5433) to avoid conflicts - Add .dockerignore to optimize Docker builds
Hey @ekas-7, It's best to create an issue first for this pull request and then reference it. Also noticed you're pushing from the main branch, please create your own branch and push from there instead. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
==========================================
- Coverage 95.06% 94.57% -0.49%
==========================================
Files 5 5
Lines 324 387 +63
==========================================
+ Hits 308 366 +58
- Misses 16 21 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
@ekas-7 that's a great PR.
I have some comments though, I let you fix them.
Dockerfile
Outdated
# Copy the local_settings example to local_settings.py (if not exists) | ||
RUN if [ ! -f local_settings.py ]; then cp local_settings_example.py local_settings.py; fi |
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 think it's a good idea to do this.
If someone wants to use a local_settings it's better to mount it in the container. It's a bad idea to have it in container image IMO. (and it's not a mandatory file)
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.
Corrected
Dockerfile
Outdated
CMD python db-migration.py && \ | ||
uvicorn folksonomy.api:app --host 0.0.0.0 --port 8000 --proxy-headers |
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 prefer not to automatically run migrations. My long experience tells me it's better that migration are run by the user manually as needed (eg., in production, after a release).
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.
docker compose exec api python db-migration.py
creted a command for mannual migration
README.md
Outdated
## Requirements | ||
- Docker | ||
- Docker Compose |
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.
Give a minimal version number, as we sometimes have problem with that.
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.
Version Updated in Readme.md
README.md
Outdated
2. Start the services | ||
```bash | ||
docker-compose up -d | ||
``` |
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.
Use docker compose
instead of docker-compose
(deprecated)
consistently which my comment above, add the migration run (docker compose run --rm folksonomy_api python db-migration.py
, if we set postgres as a dependency of folksonomy_api)
README.md
Outdated
```bash | ||
./generate_openapi_json.py | ||
psql -h localhost -p 5433 -U folksonomy -d folksonomy |
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.
why rely on having psql localy instead of using the container ?
docker compose exec -u postgres folksonomy_db psql
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.
corrected with :
docker compose exec db psql -U folksonomy -d folksonomy
docker-compose.yml
Outdated
environment: | ||
- POSTGRES_USER=folksonomy | ||
- POSTGRES_PASSWORD=folksonomy | ||
- POSTGRES_DATABASE=folksonomy | ||
- POSTGRES_HOST=db |
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.
Would be better to put variable in a .env and reuse them here.
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.
volumes:
# Mount .env file for environment variables
- ./.env:/app/.env
# Mount a volume for logs
- api_logs:/app/logs
env_file:
- .env
depends_on:
docker-compose.yml
Outdated
context: . | ||
dockerfile: Dockerfile | ||
ports: | ||
- "8000:8000" |
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.
Please don't publish on all ports, this can be a security risk in public wifi.
So by default, publish to 127.0.0.1:8000
the best would be to use a variable and set it in .env (I would call it API_EXPOSE):
- "${API_EXPOSE}:8000"
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.
ports:
- "${API_EXPOSE:-8000}:8000"
docker-compose.yml
Outdated
- POSTGRES_PASSWORD=folksonomy | ||
- POSTGRES_DB=folksonomy | ||
ports: | ||
- "5433:5432" |
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 wouldn't expose the database and surely not on all address (see above)
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.
changed to
ports:
- "${DB_EXPOSE:-5433}:5432"
@alexgarel sir right onto improvements that u have mentioned in your comments |
Co-authored-by: Alex Garel <[email protected]>
feat: Add message for existing property in product tag API (openfoodfacts#248)
@alexgarel i have taken all the comments into account and created a new version of codebase. |
@alexgarel I will make changes by EOD. Thank u for ur insights on docker . |
Screen.Recording.2025-03-18.at.7.1.mp4@alexgarel i think we are good to go |
add Docker support for Folksonomy API
What
Screenshot
Fixes bug(s)
Part of