[Feat] DB 고도화 및 Alembic을 통한 migration #43
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #43 +/- ##
=======================================
Coverage 82.30% 82.30%
=======================================
Files 42 42
Lines 1430 1430
Branches 87 87
=======================================
Hits 1177 1177
Misses 227 227
Partials 26 26
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Alembic for database migrations, which is a great step towards improving the project's maintainability and scalability. The changes are well-structured, but there are a few areas that could be improved for better clarity and robustness.
Summary of Findings
- Environment variable usage in Makefile: The Makefile uses environment variables in a way that might not be consistent across different systems. It's better to explicitly define the environment variables within the Makefile or ensure they are consistently set in the environment.
- Missing ENV variable in k8s configmaps: The removal of the
ENVvariable from the k8s configmaps could lead to inconsistencies between the application's environment and the configuration. It's important to ensure that the application is aware of its environment (dev, prod, etc.). - Hardcoded environment in k8s deployment: The
ENVvariable is hardcoded toPRODin the k8s deployment. This might not be desirable in all cases, especially for development or testing environments. It's better to make this configurable.
Merge Readiness
The pull request introduces a significant feature (Alembic migrations) and is generally well-implemented. However, the issues identified regarding environment variable handling in the Makefile and k8s configurations should be addressed before merging. I am unable to approve this pull request, and recommend that another reviewer also reviews this code before merging. Addressing the high severity issues is highly recommended before merging.
| - name: ENV | ||
| value: "PROD" |
There was a problem hiding this comment.
Hardcoding the ENV variable to PROD might not be suitable for all deployments (e.g., development, staging). Consider making this configurable via a parameter or a separate config file.
- name: ENV
valueFrom:
configMapKeyRef:
name: fastapi-env
key: ENV
| @export DESCRIPTION=$$(cat README.md) && \ | ||
| set -o allexport && \ | ||
| source envs/$${ENV,,}.env && \ | ||
| set +o allexport && \ |
There was a problem hiding this comment.
Consider using dotenv to manage environment variables instead of relying on shell commands. This can improve readability and maintainability. Also, consider whether DESCRIPTION should be an environment variable at all, or whether it should be passed directly to the alembic command as an argument.
Also, it might be better to use export directly in the .env file, instead of using set -o allexport.
@uv sync
@dotenv -f envs/$${ENV,,}.env -- uv run alembic upgrade head # Example using python-dotenv
|
|
||
| class Configs(BaseSettings): | ||
| ENV: ENVIRONMENT | ||
| ENV: ENVIRONMENT = ENVIRONMENT.TEST |
There was a problem hiding this comment.
While setting a default environment is good, ensure this default (TEST) is appropriate for all contexts. Consider raising an error if no environment is explicitly set, to force a conscious decision.
| ENV: ENVIRONMENT = ENVIRONMENT.TEST | |
| ENV: ENVIRONMENT # No default, requires explicit setting |
| dev.env: | | ||
| ENV="DEV" | ||
| PORT="8000" |
There was a problem hiding this comment.
Consider keeping the ENV variable in the configmap for clarity and consistency. Removing it might lead to confusion about the environment the application is running in.
dev.env: |
ENV="DEV"
PORT="8000"
| prod.env: | | ||
| ENV="PROD" | ||
| PORT="8000" |
There was a problem hiding this comment.
Consider keeping the ENV variable in the configmap for clarity and consistency. Removing it might lead to confusion about the environment the application is running in.
prod.env: |
ENV="PROD"
PORT="8000"
What type of PR is this?
What this PR does / why we need it