-
Notifications
You must be signed in to change notification settings - Fork 0
DB fixes #40
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
DB fixes #40
Conversation
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 pull request implements database fixes to improve database initialization and permission handling for a Subscription Tracker application. The changes focus on ensuring proper file permissions and reliable database setup in containerized environments.
- Adds a comprehensive database initialization script with permission checking
- Enhances the Docker entrypoint script with database initialization and permission management
- Updates the database configuration to use absolute paths in the instance directory
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| init_db.py | New database initialization script with permission validation and table creation |
| docker-entrypoint.sh | Added database initialization function and enhanced permission handling |
| config.py | Updated database URL configuration to use absolute paths in instance directory |
| Dockerfile | Modified to create and set proper permissions for the instance directory |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| # Test database write capability | ||
| print("Testing database write capability...") | ||
| result = db.engine.execute(db.text("SELECT 1 as test")) |
Copilot
AI
Oct 6, 2025
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.
The execute() method is deprecated in SQLAlchemy 2.0+. Use db.session.execute() instead to avoid compatibility issues.
| result = db.engine.execute(db.text("SELECT 1 as test")) | |
| result = db.session.execute(db.text("SELECT 1 as test")) |
| if [[ "$1" == *"python"* ]] || [[ "$1" == *"gunicorn"* ]] || [[ "$1" == *"run.py"* ]]; then | ||
| echo "Initializing database..." | ||
|
|
||
| # Create database directory if it doesn't exist | ||
| mkdir -p /app/instance | ||
|
|
||
| # Set proper permissions for database operations | ||
| if [ "$(id -u)" = "0" ]; then | ||
| local owner="${PUID:-$APP_USER}" | ||
| local group="${GUID:-$APP_GROUP}" | ||
| chown "$owner:$group" /app/instance | ||
| chmod 755 /app/instance | ||
|
|
||
| # If database file exists, fix its permissions | ||
| if [ -f "/app/instance/subscriptions.db" ]; then | ||
| chown "$owner:$group" /app/instance/subscriptions.db | ||
| chmod 664 /app/instance/subscriptions.db | ||
| fi | ||
| else | ||
| # Running as non-root, ensure we can write to the directory | ||
| if [ ! -w "/app/instance" ]; then | ||
| echo "WARNING: /app/instance is not writable by current user $(id -u):$(id -g)" | ||
| echo "Please ensure the mounted volume has proper permissions:" | ||
| echo " sudo chown -R $(id -u):$(id -g) ./data" | ||
| fi | ||
| fi | ||
| fi |
Copilot
AI
Oct 6, 2025
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.
The pattern matching logic is fragile and could match unintended commands. Consider using a more explicit check like case "$1" in with specific command patterns or checking the actual command name.
| if [[ "$1" == *"python"* ]] || [[ "$1" == *"gunicorn"* ]] || [[ "$1" == *"run.py"* ]]; then | |
| echo "Initializing database..." | |
| # Create database directory if it doesn't exist | |
| mkdir -p /app/instance | |
| # Set proper permissions for database operations | |
| if [ "$(id -u)" = "0" ]; then | |
| local owner="${PUID:-$APP_USER}" | |
| local group="${GUID:-$APP_GROUP}" | |
| chown "$owner:$group" /app/instance | |
| chmod 755 /app/instance | |
| # If database file exists, fix its permissions | |
| if [ -f "/app/instance/subscriptions.db" ]; then | |
| chown "$owner:$group" /app/instance/subscriptions.db | |
| chmod 664 /app/instance/subscriptions.db | |
| fi | |
| else | |
| # Running as non-root, ensure we can write to the directory | |
| if [ ! -w "/app/instance" ]; then | |
| echo "WARNING: /app/instance is not writable by current user $(id -u):$(id -g)" | |
| echo "Please ensure the mounted volume has proper permissions:" | |
| echo " sudo chown -R $(id -u):$(id -g) ./data" | |
| fi | |
| fi | |
| fi | |
| case "$1" in | |
| python|python3|gunicorn|*/run.py) | |
| echo "Initializing database..." | |
| # Create database directory if it doesn't exist | |
| mkdir -p /app/instance | |
| # Set proper permissions for database operations | |
| if [ "$(id -u)" = "0" ]; then | |
| local owner="${PUID:-$APP_USER}" | |
| local group="${GUID:-$APP_GROUP}" | |
| chown "$owner:$group" /app/instance | |
| chmod 755 /app/instance | |
| # If database file exists, fix its permissions | |
| if [ -f "/app/instance/subscriptions.db" ]; then | |
| chown "$owner:$group" /app/instance/subscriptions.db | |
| chmod 664 /app/instance/subscriptions.db | |
| fi | |
| else | |
| # Running as non-root, ensure we can write to the directory | |
| if [ ! -w "/app/instance" ]; then | |
| echo "WARNING: /app/instance is not writable by current user $(id -u):$(id -g)" | |
| echo "Please ensure the mounted volume has proper permissions:" | |
| echo " sudo chown -R $(id -u):$(id -g) ./data" | |
| fi | |
| fi | |
| ;; | |
| *) | |
| # Not a main application command, do nothing | |
| ;; | |
| esac |
| sys.path.insert(0, '/app') | ||
|
|
||
| def check_database_permissions(): | ||
| """Check and fix database file permissions""" | ||
| instance_dir = Path('/app/instance') |
Copilot
AI
Oct 6, 2025
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.
Hard-coded path '/app' reduces portability. Consider using relative paths or environment variables to make the script more flexible across different deployment environments.
| sys.path.insert(0, '/app') | |
| def check_database_permissions(): | |
| """Check and fix database file permissions""" | |
| instance_dir = Path('/app/instance') | |
| APP_HOME = os.environ.get('APP_HOME', str(Path(__file__).resolve().parent)) | |
| sys.path.insert(0, APP_HOME) | |
| def check_database_permissions(): | |
| """Check and fix database file permissions""" | |
| instance_dir = Path(APP_HOME) / 'instance' |
No description provided.