Skip to content
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.

Sign up for GitHub

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

website: updating settings file #4035

Merged
merged 4 commits into from
Dec 10, 2024
+154 −6
Merged

website: updating settings file #4035

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0e0c4c
updating settings file
OchiengPaul442 Dec 10, 2024
ef2bcce
update
OchiengPaul442 Dec 10, 2024
afc8bbe
updates
OchiengPaul442 Dec 10, 2024
fff0667
updates
OchiengPaul442 Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/website/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ RUN pip install --no-cache-dir -r requirements.txt
COPY . .

# Expose the port the Django app will run on
EXPOSE 8000
EXPOSE 8080

# Add execution permissions to entrypoint.sh
RUN chmod +x /app/entrypoint.sh
Expand Down
86 changes: 82 additions & 4 deletions src/website/core/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def require_env_var(env_var: str) -> str:
ALLOWED_HOSTS = parse_env_list('ALLOWED_HOSTS', default='localhost,127.0.0.1')

# ---------------------------------------------------------
# Applications
# Installed Apps
# ---------------------------------------------------------
INSTALLED_APPS = [
# Django Defaults
Expand Down Expand Up @@ -115,9 +115,11 @@ def require_env_var(env_var: str) -> str:
CORS_ALLOWED_ORIGIN_REGEXES = parse_env_list('CORS_ORIGIN_REGEX_WHITELIST')
CSRF_TRUSTED_ORIGINS = parse_env_list('CSRF_TRUSTED_ORIGINS')

# If no CORS settings provided, consider defaulting to empty lists
CORS_ALLOWED_ORIGINS = CORS_ALLOWED_ORIGINS if CORS_ALLOWED_ORIGINS else []
CORS_ALLOWED_ORIGIN_REGEXES = CORS_ALLOWED_ORIGIN_REGEXES if CORS_ALLOWED_ORIGIN_REGEXES else []
# Ensure no trailing slashes and correct schemes
CORS_ALLOWED_ORIGINS = [origin.rstrip('/') for origin in CORS_ALLOWED_ORIGINS]
CORS_ALLOWED_ORIGIN_REGEXES = [regex.rstrip(
'/') for regex in CORS_ALLOWED_ORIGIN_REGEXES]
CSRF_TRUSTED_ORIGINS = [origin.rstrip('/') for origin in CSRF_TRUSTED_ORIGINS]

# Security cookies
CSRF_COOKIE_SECURE = not DEBUG
Expand Down Expand Up @@ -269,3 +271,79 @@ def require_env_var(env_var: str) -> str:
'scrollingContainer': '#scrolling-container',
},
}

# ---------------------------------------------------------
# File Upload Settings
# ---------------------------------------------------------
# Increase these values as needed to handle larger uploads
FILE_UPLOAD_MAX_MEMORY_SIZE = 10485760 # 10 MB
DATA_UPLOAD_MAX_MEMORY_SIZE = 10485760 # 10 MB

# ---------------------------------------------------------
# SSL and Proxy Settings (if behind a reverse proxy)
# ---------------------------------------------------------
SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https')
USE_X_FORWARDED_HOST = True
Comment on lines +285 to +286
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Ensure SSL and proxy settings align with deployment environment

Enabling SECURE_PROXY_SSL_HEADER and USE_X_FORWARDED_HOST is appropriate when the application is behind a reverse proxy that sets the X-Forwarded-Proto header, such as Nginx. If this isn't always the case, consider making these settings conditional based on an environment variable to prevent potential misconfigurations in other environments.


# ---------------------------------------------------------
# Logging Configuration
# ---------------------------------------------------------
LOG_DIR = BASE_DIR / 'logs'
LOG_DIR.mkdir(exist_ok=True) # Ensure log directory exists
Comment on lines +291 to +292
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid side effects in settings.py by removing directory creation

Creating directories within settings.py can lead to unintended side effects since settings are imported at various points during the application's lifecycle. It's recommended to avoid I/O operations in settings files. Consider removing LOG_DIR.mkdir(exist_ok=True) and ensuring that the log directory is created during the application's startup sequence or via deployment scripts.


LOGGING = {
'version': 1,
'disable_existing_loggers': False,
# Formatters
'formatters': {
'verbose': {
'format': '[%(asctime)s] %(levelname)s %(name)s [%(filename)s:%(lineno)d] %(message)s',
'datefmt': '%Y-%m-%d %H:%M:%S'
},
'simple': {
'format': '%(levelname)s %(message)s'
},
},
# Handlers
'handlers': {
'console': {
'class': 'logging.StreamHandler',
'formatter': 'verbose',
'level': 'DEBUG' if DEBUG else 'INFO',
},
'file': {
'class': 'logging.FileHandler',
'filename': LOG_DIR / 'django.log',
'formatter': 'verbose',
'level': 'INFO',
},
'error_file': {
'class': 'logging.FileHandler',
'filename': LOG_DIR / 'django_errors.log',
'formatter': 'verbose',
'level': 'ERROR',
},
},
# Loggers
'loggers': {
# Django Logs
'django': {
'handlers': ['console', 'file', 'error_file'],
'level': 'INFO',
'propagate': True,
},
# Cloudinary Logs
'cloudinary': {
'handlers': ['console', 'file', 'error_file'],
'level': 'INFO',
'propagate': True,
},
# Event App Logs
'apps.event': {
'handlers': ['console', 'file', 'error_file'],
'level': 'DEBUG' if DEBUG else 'INFO',
'propagate': False,
},
# Add other app loggers here if needed
}
}
26 changes: 22 additions & 4 deletions src/website/entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,33 @@
# Exit immediately if a command exits with a non-zero status
set -e

# Function to handle signals and gracefully shut down Gunicorn
_term() {
echo "Caught SIGTERM signal! Shutting down Gunicorn..."
kill -TERM "$child" 2>/dev/null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Assign the child process ID to enable graceful shutdown

The variable $child is referenced in the _term function but hasn't been assigned. Without capturing the Gunicorn process ID, the script cannot gracefully terminate Gunicorn upon receiving a SIGTERM signal. Assign the PID of the Gunicorn process to $child when starting it.

Suggested fix:

Start Gunicorn in the background and capture its PID:

-exec gunicorn core.wsgi:application \
+gunicorn core.wsgi:application \
  --bind 0.0.0.0:8000 \
  --timeout 600 \
  --log-level info \
  --workers "${GUNICORN_WORKERS:-3}" \
  --access-logfile '-' \
  --error-logfile '-' &

child=$!
wait "$child"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
kill -TERM "$child" 2>/dev/null
gunicorn core.wsgi:application \
--bind 0.0.0.0:8000 \
--timeout 600 \
--log-level info \
--workers "${GUNICORN_WORKERS:-3}" \
--access-logfile '-' \
--error-logfile '-' &
child=$!
wait "$child"
🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 9-9: child is referenced but not assigned.

(SC2154)

}

# Trap SIGTERM signal
trap _term SIGTERM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct signal name in trap command for POSIX compliance

In POSIX sh, signal names should not include the SIG prefix. Update the trap command to use TERM instead of SIGTERM to ensure proper signal trapping.

Apply this change:

-trap _term SIGTERM
+trap _term TERM
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
trap _term SIGTERM
trap _term TERM
🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 13-13: In POSIX sh, prefixing signal names with 'SIG' is undefined.

(SC3048)


# Run Django migrations
echo "Running migrations..."
python manage.py migrate --noinput

# Collect static files (ensure the static files directory exists)
# Collect static files
echo "Collecting static files..."
python manage.py collectstatic --noinput

# Pre-create logs directory if not exists
echo "Ensuring log directory exists..."
mkdir -p /app/logs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Ensure consistent log directory paths across the application

The logs directory is being created at /app/logs, whereas in settings.py, it is defined as BASE_DIR / 'logs'. Depending on how BASE_DIR is resolved, these might point to different locations, potentially causing confusion or missing logs. Verify that both paths refer to the same directory, and consider centralizing the log directory path definition.


# Start Gunicorn server to serve the Django application
echo "Starting Gunicorn server..."
exec gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --log-level info
# exec gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --workers ${GUNICORN_WORKERS:-3} --log-level info
echo "Starting Gunicorn server on port $PORT..."
exec gunicorn core.wsgi:application \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Adjust Gunicorn execution to respect signal handling

Using exec replaces the current shell with the Gunicorn process, which prevents the trap for TERM from working correctly. By starting Gunicorn without exec and capturing its PID, the script can handle signals properly and ensure a graceful shutdown.

Suggested change:

-exec gunicorn core.wsgi:application \
+gunicorn core.wsgi:application \

This aligns with the earlier suggestion to assign the Gunicorn process ID to $child.

Committable suggestion skipped: line range outside the PR's diff.

--bind 0.0.0.0:$PORT \
--timeout 600 \
--log-level info \
--workers "${GUNICORN_WORKERS:-3}" \
--access-logfile '-' \
--error-logfile '-'
Loading