Refactor configuration usage, improve Dockerfile, and add startup validation & logging - #17
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's robustness and maintainability by centralizing configuration management, improving the Docker build process for efficiency and security, and integrating these settings consistently across core functionalities like authentication, rate limiting, and logging. The changes aim to provide a more secure, predictable, and observable application environment. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request significantly improves the application's configuration management, Docker setup, and startup procedures. The move to a centralized settings object and the adoption of modern FastAPI features like the lifespan manager are excellent changes. My review includes one critical fix for the Dockerfile to ensure the container can start correctly with a non-root user, and a suggestion to improve maintainability in the authentication module by removing duplicated logic.
I am having trouble creating individual review comments. Click here to see my feedback.
Dockerfile (28)
The container is configured to run as a non-privileged user app, which is an excellent security practice. However, the application is started on port 80, which is a privileged port. Non-root users cannot bind to ports below 1024, so this will cause the container to fail on startup. Please change this to a non-privileged port (e.g., 8080) and map the port during container runtime (e.g., docker run -p 80:8080 ...).
CMD ["/app/.venv/bin/fastapi", "run", "src/acebet/app/main.py", "--port", "8080", "--host", "0.0.0.0"]
src/acebet/app/dependencies/auth.py (123-127)
The logic for providing a fallback secret key (settings.acebet_secret_key or "acebet-dev-insecure-secret-key") is duplicated here and in the get_current_user function (lines 149-153). This violates the DRY (Don't Repeat Yourself) principle and could lead to inconsistencies if one location is updated but the other is not.
To improve maintainability, this logic should be centralized. For example, you could define module-level constants at the top of the file:
SECRET_KEY = settings.acebet_secret_key or "acebet-dev-insecure-secret-key"
ALGORITHM = settings.acebet_jwt_algorithmAnd then use these constants in both create_access_token and get_current_user.
Motivation
Description
Dockerfileto setPYTHONDONTWRITEBYTECODE/PYTHONUNBUFFERED, copypyproject.tomlanduv.lockfirst for layer caching, perform a two-stageuv syncto avoid installing the project twice, copysrcafterwards, and add an unprivilegedappuser to run the image.AppSettingsinsrc/acebet/app/config.pywith defaults foracebet_env, logging fields, and rate limits, added aredacted()helper to hideacebet_secret_key, and keptvalidate_config()to raise a clearRuntimeErroron invalid settings.src/acebet/app/dependencies/auth.pyto use the newsettingsobject for JWT encoding/decoding and removed previous module-level constants, while preserving a development fallback secret.src/acebet/app/main.pyto use an asynclifespanthat callsvalidate_config()at startup, wireLimiterdefaults fromsettings, switch logging tosettings.acebet_log_fileandsettings.acebet_log_level, and usesettings.acebet_access_token_expire_minutesfor token expiry.Testing
pytestand unit tests completed successfully.docker build .which completed without errors.Codex Task