feat: task runner enabler with huey #1422
Conversation
WalkthroughThe pull request introduces a comprehensive implementation of the Huey task queue system across multiple configuration files. The changes encompass adding a new background task service for processing asynchronous tasks, specifically focusing on checking and notifying about expired controls. The modifications span Docker Compose configurations, Django settings, and task definitions, enabling a more robust background processing mechanism for the CISO Assistant application. Changes
Sequence DiagramsequenceDiagram
participant Scheduler
participant HueyWorker
participant Database
participant EmailService
Scheduler->>HueyWorker: Trigger daily task
HueyWorker->>Database: Retrieve expired controls
Database-->>HueyWorker: Return control list
HueyWorker->>HueyWorker: Group controls by owner
HueyWorker->>EmailService: Send notification emails
EmailService-->>HueyWorker: Confirm email sent
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
backend/core/tasks.py (2)
1-6: Clean up unused imports.The following imports are not used in the code:
datetimefromdatetimetimezonefromdatetimeQfromdjango.db.models-from datetime import date, datetime, timezone +from datetime import date from huey import crontab from huey.contrib.djhuey import periodic_task, task, db_periodic_task, db_task from core.models import AppliedControl -from django.db.models import Q🧰 Tools
🪛 Ruff (0.8.2)
1-1:
datetime.datetimeimported but unusedRemove unused import
(F401)
1-1:
datetime.timezoneimported but unusedRemove unused import
(F401)
5-5:
django.db.models.Qimported but unusedRemove unused import:
django.db.models.Q(F401)
23-26: Fix incorrect comment in periodic task.The comment states "Every five minutes" but the crontab is set to run every minute.
@periodic_task(crontab(minute="*/1")) def every_min(): - print("Every five minutes this will be printed by the consumer") + print("Every minute this will be printed by the consumer")README.md (1)
411-415: Enhance task runner documentation.The Huey setup instructions could be more comprehensive. Consider adding:
- Configuration options explanation
- Task monitoring instructions
- Error handling guidelines
- Production deployment considerations
Here's a suggested addition to the documentation:
### Task Runner (Huey) CISO Assistant uses Huey with Redis for background task processing. #### Configuration - `REDIS_URL`: Redis connection URL (default: redis://localhost:6379/?db=1) - `HUEY.immediate`: Set to True for synchronous execution in development - `HUEY.results`: Enable/disable task result storage #### Monitoring Monitor tasks using the Huey consumer: ```bash python manage.py run_huey --verboseProduction Setup
- Configure Redis with password:
docker run -d -p 6379:6379 redis:alpine --requirepass your_secure_password- Set environment variables:
export REDIS_URL="redis://:your_secure_password@localhost:6379/?db=1"- Run multiple Huey workers:
python manage.py run_huey -w 4 -k process</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 0babd1bc71946f1579c557bc815be795466c9f51 and be4c924d599644b67fb52637b501a14e2c263f0c. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `backend/poetry.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (4)</summary> * `README.md` (4 hunks) * `backend/ciso_assistant/settings.py` (2 hunks) * `backend/core/tasks.py` (1 hunks) * `backend/pyproject.toml` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Ruff (0.8.2)</summary> <details> <summary>backend/core/tasks.py</summary> 1-1: `datetime.datetime` imported but unused Remove unused import (F401) --- 1-1: `datetime.timezone` imported but unused Remove unused import (F401) --- 5-5: `django.db.models.Q` imported but unused Remove unused import: `django.db.models.Q` (F401) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (11)</summary> * GitHub Check: enterprise-startup-functional-test (3.12) * GitHub Check: startup-functional-test (3.12) * GitHub Check: startup-docker-compose-test * GitHub Check: functional-tests (3.12, chromium) * GitHub Check: test (3.12) * GitHub Check: build (3.12) * GitHub Check: ruff (3.12) * GitHub Check: enterprise-functional-tests (3.12, chromium) * GitHub Check: enterprise-startup-docker-compose-test * GitHub Check: Analyze (python) * GitHub Check: Analyze (javascript-typescript) </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>backend/ciso_assistant/settings.py (1)</summary> `417-423`: _:hammer_and_wrench: Refactor suggestion_ **Enhance Redis configuration security.** The Redis configuration could be improved: 1. The default URL uses db=1 which is good (avoiding db=0) 2. However, it lacks password authentication Consider updating the Redis URL configuration to support authentication: ```diff HUEY = { "huey_class": "huey.RedisHuey", "name": "ciso_assistant", "results": True, "immediate": False, # True for local dev, set to False to run in "live" regardless of DEBUG - "url": os.environ.get("REDIS_URL", "redis://localhost:6379/?db=1"), + "url": os.environ.get("REDIS_URL", "redis://:password@localhost:6379/?db=1"), }Also, consider adding these environment variables to the README's configuration section:
- REDIS_URL: For configuring Redis connection with authentication
- REDIS_PASSWORD: For setting Redis password in development
backend/pyproject.toml (1)
40-40: Verify Redis client compatibility.The Redis client version 5.2.1 is appropriate, but let's verify compatibility with Huey 2.5.2.
✅ Verification successful
Redis 5.2.1 is compatible with Huey 2.5.2
Redis 5.2.1 meets Huey's minimum requirement of Redis>=5.0 and is suitable for all features including priority support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check compatibility between redis and huey versions # Get Huey's requirements pip install huey==2.5.2 --dry-run 2>&1 | grep -i "redis" # Check for any known issues with redis 5.2.1 pip index versions redis | head -n 5Length of output: 1023
Script:
#!/bin/bash # Check Huey's source code for Redis client usage pip download huey==2.5.2 --no-deps -d /tmp cd /tmp && tar xf huey-2.5.2.tar.gz cat huey-2.5.2/huey/storage.py | grep -A 10 "class RedisStorage"Length of output: 1095
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/ciso_assistant/settings.py (1)
415-416: Enhance Redis setup documentation.The current comment for Redis setup is minimal. Consider adding more comprehensive documentation:
- Container persistence options
- Security considerations
- Production deployment notes
Consider updating the comment to:
-# for dev: docker run -d -p 6379:6379 redis:alpine +# Development Redis setup: +# docker run -d \ +# --name redis-huey \ +# -v redis-data:/data \ +# -p 127.0.0.1:6379:6379 \ +# redis:alpine \ +# redis-server --appendonly yes +# +# Note: For production, ensure: +# - Proper authentication is configured +# - Network access is restricted +# - Data persistence is properly configured
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/ciso_assistant/settings.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
backend/ciso_assistant/settings.py (2)
159-159: LGTM: Huey Django integration enabled.The addition of "huey.contrib.djhuey" to INSTALLED_APPS is correct and necessary for integrating Huey with Django.
417-425: Review and enhance the Huey configuration.Several important considerations for the current configuration:
- The
immediatesetting's comment suggests different behaviors for dev/prod, which could lead to confusion- The file-based task storage path might need existence checks
- The Redis URL in the comment exposes default credentials
Consider these improvements:
- Use environment variables for configuration:
HUEY = { "huey_class": "huey.FileHuey", - # "huey_class": "huey.RedisHuey", "name": "ciso_assistant", "results": True, - "immediate": False, # True for local dev, set to False to run in "live" regardless of DEBUG + "immediate": os.getenv("HUEY_IMMEDIATE", "False") == "True", "path": BASE_DIR / "db" / "huey-tasks", - # "url": os.environ.get("REDIS_URL", "redis://localhost:6379/?db=1"), }
- Add path existence check:
# Add before HUEY configuration huey_tasks_path = BASE_DIR / "db" / "huey-tasks" huey_tasks_path.mkdir(parents=True, exist_ok=True)
- For Redis configuration, consider a separate settings file or environment-specific settings:
# settings_redis.py HUEY = { "huey_class": "huey.RedisHuey", "name": "ciso_assistant", "results": True, "immediate": os.getenv("HUEY_IMMEDIATE", "False") == "True", "url": os.getenv("REDIS_URL", "redis://:password@localhost:6379/1"), }Let me verify the current Redis configuration in the codebase:
✅ Verification successful
Simplified Huey configuration improvements needed
The current task implementation is lightweight and works well with FileHuey. Here are the validated improvements:
- Add directory existence check to prevent startup issues:
# Add before HUEY configuration huey_tasks_path = BASE_DIR / "db" / "huey-tasks" huey_tasks_path.mkdir(parents=True, exist_ok=True)
- Use environment variable for immediate mode to clarify dev/prod behavior:
HUEY = { "huey_class": "huey.FileHuey", "name": "ciso_assistant", "results": True, - "immediate": False, # True for local dev, set to False to run in "live" regardless of DEBUG + "immediate": os.getenv("HUEY_IMMEDIATE", "False") == "True", "path": BASE_DIR / "db" / "huey-tasks", }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Redis-related configurations and usage echo "Checking for Redis configurations..." rg -l 'redis' --type-add 'requirements:*.txt' --type-add 'toml:*.toml' --type requirements --type toml echo "Checking for Huey task definitions..." rg -l 'periodic_task|task' --type pythonLength of output: 384
Script:
#!/bin/bash # Check Redis dependencies and Huey configuration echo "=== Checking pyproject.toml for Redis dependencies ===" cat backend/pyproject.toml echo -e "\n=== Searching for task definitions ===" find . -name "*.py" -type f -exec grep -l "periodic_task\|task" {} \; echo -e "\n=== Checking db directory structure ===" find backend/db -type d 2>/dev/null || echo "db directory not found" echo -e "\n=== Checking Redis-related imports ===" find . -name "*.py" -type f -exec grep -l "import.*redis\|from.*redis" {} \;Length of output: 1786
Script:
#!/bin/bash echo "=== Checking tasks.py content ===" cat backend/core/tasks.py echo -e "\n=== Checking db directory permissions ===" ls -la backend/db/Length of output: 1719
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/core/tasks.py (2)
8-10: Remove outdated documentation comment.The comment referring to "basic placeholders" is no longer relevant as the code now contains actual business logic.
-# basic placeholders from the official doc -# https://huey.readthedocs.io/en/latest/django.html
1-5: Clean up unused imports.Several imports are not being used in the code:
datetime,timezonefromdatetimeperiodic_task,db_taskfromhuey.contrib.djhueyQfromdjango.db.models-from datetime import date, datetime, timezone +from datetime import date from huey import crontab -from huey.contrib.djhuey import periodic_task, task, db_periodic_task, db_task +from huey.contrib.djhuey import task, db_periodic_task from core.models import AppliedControl -from django.db.models import Q🧰 Tools
🪛 Ruff (0.8.2)
1-1:
datetime.datetimeimported but unusedRemove unused import
(F401)
1-1:
datetime.timezoneimported but unusedRemove unused import
(F401)
3-3:
huey.contrib.djhuey.periodic_taskimported but unusedRemove unused import
(F401)
3-3:
huey.contrib.djhuey.db_taskimported but unusedRemove unused import
(F401)
5-5:
django.db.models.Qimported but unusedRemove unused import:
django.db.models.Q(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/tasks.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/tasks.py
1-1: datetime.datetime imported but unused
Remove unused import
(F401)
1-1: datetime.timezone imported but unused
Remove unused import
(F401)
3-3: huey.contrib.djhuey.periodic_task imported but unused
Remove unused import
(F401)
3-3: huey.contrib.djhuey.db_task imported but unused
Remove unused import
(F401)
5-5: django.db.models.Q imported but unused
Remove unused import: django.db.models.Q
(F401)
35-35: Local variable subject is assigned to but never used
Remove assignment to unused variable subject
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/core/tasks.py (1)
12-31: 🛠️ Refactor suggestionEnhance periodic task implementation.
The current implementation has several areas for improvement:
- Running every minute might be too frequent for checking expired ETAs
- Missing error handling
- Lacks logging for monitoring and debugging
+import logging + +logger = logging.getLogger(__name__) + -@db_periodic_task(crontab(minute="*/1")) +@db_periodic_task(crontab(minute="0", hour="*/1")) # Run hourly instead of every minute def check_controls_with_expired_eta(): - expired_controls = ( - AppliedControl.objects.exclude(status="active") - .filter(eta__lt=date.today(), eta__isnull=False) - .prefetch_related("owner") - ) + try: + logger.info("Starting expired controls check") + expired_controls = ( + AppliedControl.objects.exclude(status="active") + .filter(eta__lt=date.today(), eta__isnull=False) + .prefetch_related("owner") + ) - # Group by individual owner - owner_controls = {} - for control in expired_controls: - for owner in control.owner.all(): - if owner.email not in owner_controls: - owner_controls[owner.email] = [] - owner_controls[owner.email].append(control) + # Group by individual owner + owner_controls = {} + for control in expired_controls: + for owner in control.owner.all(): + if owner.email not in owner_controls: + owner_controls[owner.email] = [] + owner_controls[owner.email].append(control) - # Send personalized email to each owner - for owner_email, controls in owner_controls.items(): - send_notification_email(owner_email, controls) + # Send personalized email to each owner + for owner_email, controls in owner_controls.items(): + logger.info(f"Scheduling notification for {owner_email} with {len(controls)} expired controls") + send_notification_email(owner_email, controls) + + logger.info(f"Completed expired controls check. Processed {len(owner_controls)} owners") + except Exception as e: + logger.error(f"Failed to check expired controls: {str(e)}") + raiseLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/core/tasks.py (2)
1-7: Remove unused imports.The following imports are not used in the code:
periodic_taskfromhuey.contrib.djhueydb_taskfromhuey.contrib.djhueyQfromdjango.db.modelsfrom datetime import date from huey import crontab -from huey.contrib.djhuey import periodic_task, task, db_periodic_task, db_task +from huey.contrib.djhuey import task, db_periodic_task from core.models import AppliedControl -from django.db.models import Q from django.core.mail import send_mail from django.conf import settings🧰 Tools
🪛 Ruff (0.8.2)
3-3:
huey.contrib.djhuey.periodic_taskimported but unusedRemove unused import
(F401)
3-3:
huey.contrib.djhuey.db_taskimported but unusedRemove unused import
(F401)
5-5:
django.db.models.Qimported but unusedRemove unused import:
django.db.models.Q(F401)
34-48: Add type hints and error handling to email task.Consider adding type hints and error handling for better maintainability and reliability.
+from typing import List @task() -def send_notification_email(owner_email, controls): +def send_notification_email(owner_email: str, controls: List[AppliedControl]) -> None: subject = f"CISO Assistant: You have {len(controls)} expired control(s)" message = "The following controls have expired:\n\n" for control in controls: message += f"- {control.name} (ETA: {control.eta})\n" message += "\nThis reminder will stop once the control is marked as active or you update the ETA.\n" - # think templating and i18n - send_mail( - subject=subject, - message=message, - from_email=settings.DEFAULT_FROM_EMAIL, - recipient_list=[owner_email], - fail_silently=False, - ) + try: + logger.info(f"Sending notification email to {owner_email}") + send_mail( + subject=subject, + message=message, + from_email=settings.DEFAULT_FROM_EMAIL, + recipient_list=[owner_email], + fail_silently=False, + ) + logger.info(f"Successfully sent notification email to {owner_email}") + except Exception as e: + logger.error(f"Failed to send notification email to {owner_email}: {str(e)}") + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/ciso_assistant/settings.py(2 hunks)backend/core/tasks.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/tasks.py
3-3: huey.contrib.djhuey.periodic_task imported but unused
Remove unused import
(F401)
3-3: huey.contrib.djhuey.db_task imported but unused
Remove unused import
(F401)
5-5: django.db.models.Q imported but unused
Remove unused import: django.db.models.Q
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test (3.12)
🔇 Additional comments (3)
backend/core/tasks.py (1)
13-31: 🛠️ Refactor suggestionOptimize periodic task execution and add error handling.
Consider the following improvements:
- Running every minute might be too frequent for this type of check. Consider hourly or daily execution.
- Add error handling and logging for better monitoring.
- Consider adding batch processing for large datasets.
-@db_periodic_task(crontab(minute="*/1")) +@db_periodic_task(crontab(hour="*/1")) # Run hourly instead def check_controls_with_expired_eta(): - expired_controls = ( - AppliedControl.objects.exclude(status="active") - .filter(eta__lt=date.today(), eta__isnull=False) - .prefetch_related("owner") - ) + try: + logger.info("Starting expired controls check") + expired_controls = ( + AppliedControl.objects.exclude(status="active") + .filter(eta__lt=date.today(), eta__isnull=False) + .prefetch_related("owner") + ) - # Group by individual owner - owner_controls = {} - for control in expired_controls: - for owner in control.owner.all(): - if owner.email not in owner_controls: - owner_controls[owner.email] = [] - owner_controls[owner.email].append(control) + # Group by individual owner + owner_controls = {} + batch_size = 1000 # Process in batches to avoid memory issues + for control in expired_controls.iterator(chunk_size=batch_size): + for owner in control.owner.all(): + if owner.email not in owner_controls: + owner_controls[owner.email] = [] + owner_controls[owner.email].append(control) - # Send personalized email to each owner - for owner_email, controls in owner_controls.items(): - send_notification_email(owner_email, controls) + # Send personalized email to each owner + for owner_email, controls in owner_controls.items(): + send_notification_email(owner_email, controls) + + logger.info(f"Completed expired controls check. Processed {len(owner_controls)} owners") + except Exception as e: + logger.error(f"Failed to check expired controls: {str(e)}") + raiseLikely invalid or redundant comment.
backend/ciso_assistant/settings.py (2)
159-159: LGTM!The Huey app is correctly added to INSTALLED_APPS.
415-418: Consider making email configuration more robust.The email configuration is only set for DEBUG mode. Consider:
- Providing a fallback configuration for non-DEBUG mode
- Adding email configuration validation
Run this script to check email configuration across the codebase:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
backend/core/tasks.py (6)
1-8: Clean up unused imports.Remove the following unused imports to maintain clean code:
periodic_taskdb_taskQfrom datetime import date from huey import crontab -from huey.contrib.djhuey import periodic_task, task, db_periodic_task, db_task +from huey.contrib.djhuey import task, db_periodic_task from core.models import AppliedControl -from django.db.models import Q from django.core.mail import send_mail from django.conf import settings🧰 Tools
🪛 Ruff (0.8.2)
3-3:
huey.contrib.djhuey.periodic_taskimported but unusedRemove unused import
(F401)
3-3:
huey.contrib.djhuey.db_taskimported but unusedRemove unused import
(F401)
5-5:
django.db.models.Qimported but unusedRemove unused import:
django.db.models.Q(F401)
9-11: Enhance module documentation.The current documentation only references Huey docs. Consider adding a module docstring describing the purpose and functionality of these tasks.
+""" +This module implements background tasks for CISO Assistant using Huey. + +It provides functionality for: +- Periodic checking of controls with expired ETAs +- Sending email notifications to control owners +""" + # basic placeholders from the official doc # https://huey.readthedocs.io/en/latest/django.html
13-14: Consider reducing task frequency.Running this task every minute might be too frequent and could impact system resources. Consider running it hourly or daily since ETAs are date-based, not time-based.
-@db_periodic_task(crontab(minute="*/1")) +@db_periodic_task(crontab(hour="0")) # Run once daily at midnight
21-31: Consider using Django's grouping capabilities.The manual grouping logic could be simplified using Django's capabilities.
- # Group by individual owner - owner_controls = {} - for control in expired_controls: - for owner in control.owner.all(): - if owner.email not in owner_controls: - owner_controls[owner.email] = [] - owner_controls[owner.email].append(control) + from django.db.models import Prefetch + from itertools import groupby + from operator import attrgetter + + # Flatten and sort by owner email + control_owners = [(owner.email, control) + for control in expired_controls + for owner in control.owner.all()] + control_owners.sort(key=lambda x: x[0]) + + # Group using itertools + owner_controls = { + email: [item[1] for item in group] + for email, group in groupby(control_owners, key=lambda x: x[0]) + }
34-50: Add type hints and improve documentation.The email notification task would benefit from type hints and proper documentation.
@task() -def send_notification_email(owner_email, controls): +def send_notification_email(owner_email: str, controls: list['AppliedControl']) -> None: + """ + Send an email notification to a control owner about their expired controls. + + Args: + owner_email: The email address of the control owner + controls: List of expired AppliedControl instances + + Raises: + SMTPException: If email sending fails + """
43-43: Consider implementing the templating TODO.The comment suggests using templates and i18n. This should be implemented to improve maintainability and internationalization support.
Would you like me to help create an email template and implement i18n support for the email notifications?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/tasks.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/tasks.py
3-3: huey.contrib.djhuey.periodic_task imported but unused
Remove unused import
(F401)
3-3: huey.contrib.djhuey.db_task imported but unused
Remove unused import
(F401)
5-5: django.db.models.Q imported but unused
Remove unused import: django.db.models.Q
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/core/tasks.py (1)
15-19: LGTM! Efficient database query.Good use of
prefetch_relatedfor optimizing the owner relationship query.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
docker-compose-pg-build.yml (1)
75-81: Enhance Caddy reverse proxy configuration.The current Caddy configuration might not handle WebSocket connections that could be needed for real-time features. Consider adding WebSocket support.
command: - caddy - reverse-proxy + - --header-up + - "Connection {>Connection}" + - --header-up + - "Upgrade {>Upgrade}" - --from - https://localhost:8443 - --to - frontend:3000backend/core/tasks.py (1)
55-61: Consider using HTML email templates.The current implementation uses plain text emails. Consider using HTML templates for better formatting and user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(4 hunks)backend/ciso_assistant/settings.py(3 hunks)backend/core/tasks.py(1 hunks)docker-compose-build.yml(1 hunks)docker-compose-pg-build.yml(1 hunks)docker-compose-pg.yml(1 hunks)docker-compose.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- backend/ciso_assistant/settings.py
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
🔇 Additional comments (1)
docker-compose-build.yml (1)
27-28: Review volume sharing security implications.Both services share the same volume (
./db:/code/db). While this is necessary for database access, ensure that appropriate file permissions and access controls are in place.Also applies to: 15-16
docker-compose-build.yml
Outdated
| huey: | ||
| container_name: huey | ||
| build: | ||
| context: ./backend | ||
| dockerfile: Dockerfile | ||
| depends_on: | ||
| - backend | ||
| restart: always | ||
| environment: | ||
| - ALLOWED_HOSTS=backend,localhost | ||
| - CISO_ASSISTANT_URL=https://localhost:8443 | ||
| - DJANGO_DEBUG=False | ||
| volumes: | ||
| - ./db:/code/db | ||
| entrypoint: | ||
| - /bin/sh | ||
| - -c | ||
| - | | ||
| poetry run python manage.py run_huey -w 2 -k process |
There was a problem hiding this comment.
💡 Codebase verification
Environment variable inconsistency confirmed in development configurations
The review comment is correct. The inconsistency in DJANGO_DEBUG settings exists in development configurations:
docker-compose.yml: backend=True, huey=Falsedocker-compose-build.yml: backend=True, huey=False
Note that in the production-like setup (docker-compose-pg.yml), both services correctly use DJANGO_DEBUG=False, which should be used as a reference for consistent configuration.
🔗 Analysis chain
Ensure consistent environment variables between backend and huey services.
The DJANGO_DEBUG setting differs between the services (True for backend, False for huey). This inconsistency could lead to unexpected behavior since both services share the same codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other environment variable inconsistencies between backend and huey services
diff <(rg -A 10 'backend:.*environment:' | grep -E '^\s*-') <(rg -A 10 'huey:.*environment:' | grep -E '^\s*-')
Length of output: 225
Script:
#!/bin/bash
# First, find all docker-compose files
echo "Docker compose files:"
fd "docker-compose" --type f
echo -e "\nBackend service environment variables:"
rg -A 15 "^\s+backend:" docker-compose*.yml | rg "environment:" -A 10
echo -e "\nHuey service environment variables:"
rg -A 15 "^\s+huey:" docker-compose*.yml | rg "environment:" -A 10
Length of output: 8180
backend/core/tasks.py
Outdated
| @db_periodic_task(crontab(minute="*/1")) | ||
| def check_controls_with_expired_eta(): | ||
| expired_controls = ( | ||
| AppliedControl.objects.exclude(status="active") | ||
| .filter(eta__lt=date.today(), eta__isnull=False) | ||
| .prefetch_related("owner") | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize periodic task scheduling and database query.
The current implementation has several areas for improvement:
- Running every minute (
*/1) might be too frequent for this type of check - Missing transaction handling for database operations
- Could benefit from query optimization
-@db_periodic_task(crontab(minute="*/1"))
+@db_periodic_task(crontab(minute="0", hour="*/1")) # Run hourly instead
def check_controls_with_expired_eta():
+ from django.db import transaction
+ with transaction.atomic():
expired_controls = (
AppliedControl.objects.exclude(status="active")
.filter(eta__lt=date.today(), eta__isnull=False)
- .prefetch_related("owner")
+ .select_related("owner") # More efficient for single owner
+ .only("name", "eta", "owner__email") # Select only needed fields
)📝 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.
| @db_periodic_task(crontab(minute="*/1")) | |
| def check_controls_with_expired_eta(): | |
| expired_controls = ( | |
| AppliedControl.objects.exclude(status="active") | |
| .filter(eta__lt=date.today(), eta__isnull=False) | |
| .prefetch_related("owner") | |
| ) | |
| @db_periodic_task(crontab(minute="0", hour="*/1")) # Run hourly instead | |
| def check_controls_with_expired_eta(): | |
| from django.db import transaction | |
| with transaction.atomic(): | |
| expired_controls = ( | |
| AppliedControl.objects.exclude(status="active") | |
| .filter(eta__lt=date.today(), eta__isnull=False) | |
| .select_related("owner") # More efficient for single owner | |
| .only("name", "eta", "owner__email") # Select only needed fields | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/tasks.py (1)
1-16: Clean up imports and add type hints.
- Remove unused
randomimport- Remove duplicate
loggingimport- Consider adding type hints for better code maintainability
-import random - -import logging.config +import logging.config import structlog +from typing import Dict, List logging.config.dictConfig(settings.LOGGING) logger = structlog.getLogger(__name__)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/tasks.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
🔇 Additional comments (2)
backend/core/tasks.py (2)
18-36: 🛠️ Refactor suggestionOptimize database query and add error handling.
The current implementation could be improved for better performance and reliability:
@db_periodic_task(crontab(hour="6")) def check_controls_with_expired_eta(): + from django.db import transaction + + try: + with transaction.atomic(): - expired_controls = ( - AppliedControl.objects.exclude(status="active") - .filter(eta__lt=date.today(), eta__isnull=False) - .prefetch_related("owner") - ) + expired_controls = ( + AppliedControl.objects.exclude(status="active") + .filter(eta__lt=date.today(), eta__isnull=False) + .select_related("owner") # More efficient for single owner relationships + .iterator() # Use iterator for large datasets + ) - owner_controls = {} + owner_controls: Dict[str, List[AppliedControl]] = {} for control in expired_controls: for owner in control.owner.all(): if owner.email not in owner_controls: owner_controls[owner.email] = [] owner_controls[owner.email].append(control) # Send personalized email to each owner for owner_email, controls in owner_controls.items(): send_notification_email(owner_email, controls) + except Exception as e: + logger.error(f"Failed to process expired controls: {str(e)}") + raiseLikely invalid or redundant comment.
38-75: 🛠️ Refactor suggestionEnhance email notification task with retries and validation.
The email notification implementation needs several improvements for better reliability and maintainability:
+from typing import List, Optional +from django.core.validators import validate_email +from django.core.exceptions import ValidationError +from core.models import AppliedControl + -@task() +@task(retries=3, retry_delay=60) # Add retry for transient failures -def send_notification_email(owner_email, controls): +def send_notification_email(owner_email: str, controls: List[AppliedControl]) -> None: # Check required email settings required_settings = ["EMAIL_HOST", "EMAIL_PORT", "DEFAULT_FROM_EMAIL"] missing_settings = [ setting for setting in required_settings if not hasattr(settings, setting) or not getattr(settings, setting) ] if missing_settings: error_msg = f"Cannot send email notification: Missing email settings: {', '.join(missing_settings)}" logger.error(error_msg) - return + raise ValueError(error_msg) # Raise for retry mechanism if not owner_email: logger.error("Cannot send email notification: No recipient email provided") - return + return # Invalid email is not retryable + + try: + validate_email(owner_email) + except ValidationError: + logger.error(f"Invalid email address: {owner_email}") + return # Invalid email is not retryable subject = f"CISO Assistant: You have {len(controls)} expired control(s)" - message = "Hello,\n\nThe following controls have expired:\n\n" + + # Plain text message + text_message = "Hello,\n\nThe following controls have expired:\n\n" for control in controls: - message += f"- {control.name} (ETA: {control.eta})\n" - message += "\nThis reminder will stop once the control is marked as active or you update the ETA.\n" - message += "Log in to your CISO Assistant portal and check 'my assignments' section to get to your controls directly.\n\n" - message += "Thank you." + text_message += f"- {control.name} (ETA: {control.eta})\n" + text_message += "\nThis reminder will stop once the control is marked as active or you update the ETA.\n" + text_message += "Log in to your CISO Assistant portal and check 'my assignments' section to get to your controls directly.\n\n" + text_message += "Thank you." + + # HTML message + html_message = f""" + <html> + <body> + <p>Hello,</p> + <p>The following controls have expired:</p> + <ul> + {"".join(f"<li>{control.name} (ETA: {control.eta})</li>" for control in controls)} + </ul> + <p>This reminder will stop once the control is marked as active or you update the ETA.</p> + <p>Log in to your CISO Assistant portal and check 'my assignments' section to get to your controls directly.</p> + <p>Thank you.</p> + </body> + </html> + """ try: send_mail( subject=subject, - message=message, + message=text_message, + html_message=html_message, from_email=settings.DEFAULT_FROM_EMAIL, recipient_list=[owner_email], fail_silently=False, + headers={'X-Auto-Response': 'auto-generated'} ) logger.info(f"Successfully sent notification email to {owner_email}") except Exception as e: logger.error(f"Failed to send notification email to {owner_email}: {str(e)}") + raise # Propagate error for retry mechanismLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
docker-compose-build.yml (1)
15-33:⚠️ Potential issueEnvironment variable inconsistency detected.
The
DJANGO_DEBUGsetting differs between the services:
- backend:
DJANGO_DEBUG=True- huey:
DJANGO_DEBUG=FalseThis inconsistency could lead to unexpected behavior since both services share the same codebase.
Apply this diff to maintain consistency:
environment: - - DJANGO_DEBUG=False + - DJANGO_DEBUG=Truedocker-compose.yml (2)
16-16:⚠️ Potential issueSpecify exact image tag for production deployments.
Using
latesttag in production can lead to unexpected behavior during deployments.Apply this diff:
- image: ghcr.io/intuitem/ciso-assistant-community/backend:latest + image: ghcr.io/intuitem/ciso-assistant-community/backend:${VERSION:-latest}
20-24:⚠️ Potential issueEnvironment variable inconsistency detected.
The
DJANGO_DEBUGsetting differs between services:
- backend:
DJANGO_DEBUG=True- huey:
DJANGO_DEBUG=Falsedocker-compose-pg.yml (1)
29-32:⚠️ Potential issueEnhance database credential security.
Database credentials are hardcoded in the configuration.
Apply this diff:
- - POSTGRES_NAME=ciso_assistant - - POSTGRES_USER=ciso_assistant - - POSTGRES_PASSWORD=ciso_assistant + - POSTGRES_NAME=${POSTGRES_NAME:-ciso_assistant} + - POSTGRES_USER=${POSTGRES_USER:-ciso_assistant} + - POSTGRES_PASSWORD=${POSTGRES_PASSWORD:-ciso_assistant}docker-compose-pg-build.yml (1)
13-17:⚠️ Potential issueEnhance security and consistency across services.
Two issues need attention:
- Environment variable inconsistency:
DJANGO_DEBUG=Truein backend butFalsein huey- Hardcoded database credentials
Apply these diffs:
# For backend and huey services - - POSTGRES_NAME=ciso_assistant - - POSTGRES_USER=ciso_assistant - - POSTGRES_PASSWORD=ciso_assistant + - POSTGRES_NAME=${POSTGRES_NAME:-ciso_assistant} + - POSTGRES_USER=${POSTGRES_USER:-ciso_assistant} + - POSTGRES_PASSWORD=${POSTGRES_PASSWORD:-ciso_assistant}# For huey service - - DJANGO_DEBUG=False + - DJANGO_DEBUG=TrueAlso applies to: 32-36
🧹 Nitpick comments (1)
docker-compose-pg-build.yml (1)
1-1: Consider implementing a centralized configuration management.To address the recurring issues across Docker Compose files:
- Create a
.envfile for environment variables- Use Docker Secrets for sensitive information in production
- Implement consistent debug settings across services
- Standardize image versioning strategy
This will improve maintainability and security while reducing configuration drift between environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docker-compose-build.yml(1 hunks)docker-compose-pg-build.yml(1 hunks)docker-compose-pg.yml(1 hunks)docker-compose.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docker-compose.yml (1)
14-31: Consider health checks for service dependencies.The
hueyservice depends onbackend, but there's no health check to ensure the backend is fully operational before starting.Add health checks to ensure proper service startup:
depends_on: - - backend + backend: + condition: service_healthy + healthcheck: + test: ["CMD", "curl", "-f", "http://backend:8000/health"] + interval: 10s + timeout: 5s + retries: 3docker-compose-pg-build.yml (2)
21-43: Optimize Huey worker configuration for development.The current configuration uses fixed values for workers and scheduler interval. For development, these should be configurable.
Make the worker count and scheduler interval configurable:
entrypoint: - /bin/sh - -c - | - poetry run python manage.py run_huey -w 2 --scheduler-interval 60 + poetry run python manage.py run_huey -w ${HUEY_WORKERS:-2} --scheduler-interval ${SCHEDULER_INTERVAL:-60}
1-83: Standardize service configurations across compose files.The compose files have inconsistent configurations for similar services. Consider:
- Standardizing restart policies
- Using consistent environment variable patterns
- Implementing uniform health checks
- Setting resource limits consistently
I can help create a base compose file with common configurations that can be extended. Would you like me to generate that?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docker-compose-build.yml(1 hunks)docker-compose-pg-build.yml(1 hunks)docker-compose-pg.yml(1 hunks)docker-compose.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
🔇 Additional comments (2)
docker-compose-build.yml (1)
15-33:⚠️ Potential issueEnsure consistent environment variables between backend and huey services.
The
DJANGO_DEBUGsetting differs between the services (Truefor backend,Falsefor huey). This inconsistency could lead to unexpected behavior since both services share the same codebase.Apply this diff to align the debug settings:
environment: - - DJANGO_DEBUG=False + - DJANGO_DEBUG=TrueLikely invalid or redundant comment.
docker-compose-pg.yml (1)
19-39: 🛠️ Refactor suggestionEnhance security of the huey service configuration.
The service configuration could be improved for production use:
- Database credentials are hardcoded
- No resource limits specified
- No logging configuration
Apply these security enhancements:
environment: - - POSTGRES_NAME=ciso_assistant - - POSTGRES_USER=ciso_assistant - - POSTGRES_PASSWORD=ciso_assistant + - POSTGRES_NAME=${POSTGRES_NAME:-ciso_assistant} + - POSTGRES_USER=${POSTGRES_USER:-ciso_assistant} + - POSTGRES_PASSWORD=${POSTGRES_PASSWORD:-ciso_assistant} + deploy: + resources: + limits: + cpus: '0.50' + memory: 512M + logging: + driver: "json-file" + options: + max-size: "10m" + max-file: "3"Likely invalid or redundant comment.
|
once #1434 is merged, we can make use of the extra setting. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/core/tasks.py (3)
1-11: Remove unused imports to improve code cleanliness.The following imports are not used in the code:
periodic_taskfromhuey.contrib.djhueydb_taskfromhuey.contrib.djhueyQfromdjango.db.modelsrandomfrom datetime import date from huey import crontab -from huey.contrib.djhuey import periodic_task, task, db_periodic_task, db_task +from huey.contrib.djhuey import task, db_periodic_task from core.models import AppliedControl -from django.db.models import Q from django.core.mail import send_mail from django.conf import settings import logging -import random from global_settings.models import GlobalSettings🧰 Tools
🪛 Ruff (0.8.2)
3-3:
huey.contrib.djhuey.periodic_taskimported but unusedRemove unused import
(F401)
3-3:
huey.contrib.djhuey.db_taskimported but unusedRemove unused import
(F401)
5-5:
django.db.models.Qimported but unusedRemove unused import:
django.db.models.Q(F401)
9-9:
randomimported but unusedRemove unused import:
random(F401)
19-37: Optimize database query and add error handling.The current implementation could be improved in terms of query efficiency and error handling:
@db_periodic_task(crontab(hour="6")) def check_controls_with_expired_eta(): + try: expired_controls = ( AppliedControl.objects.exclude(status="active") .filter(eta__lt=date.today(), eta__isnull=False) - .prefetch_related("owner") + .select_related("owner") # More efficient for single owner + .only("name", "eta", "owner__email") # Select only needed fields ) # Group by individual owner owner_controls = {} for control in expired_controls: for owner in control.owner.all(): if owner.email not in owner_controls: owner_controls[owner.email] = [] owner_controls[owner.email].append(control) # Send personalized email to each owner for owner_email, controls in owner_controls.items(): send_notification_email(owner_email, controls) + except Exception as e: + logger.error(f"Failed to check expired controls: {str(e)}") + raiseConsider adding an index for frequent queries:
class AppliedControl(models.Model): class Meta: indexes = [ models.Index(fields=['status', 'eta']), ]
39-86: Enhance email notification robustness and type safety.Consider the following improvements:
+from typing import List, Optional +from django.core.validators import validate_email +from django.core.exceptions import ValidationError -@task() +@task(retries=3, retry_delay=60) # Add retry mechanism -def send_notification_email(owner_email, controls): +def send_notification_email(owner_email: str, controls: List[AppliedControl]) -> None: # TODO this will probably will move to a common section later on. notifications_enable_mailing = GlobalSettings.objects.get(name="general").value.get( "notifications_enable_mailing", False ) if not notifications_enable_mailing: logger.warning( "Email notification is disabled. You can enable it under Extra/Settings. Skipping for now." ) return # Check required email settings required_settings = ["EMAIL_HOST", "EMAIL_PORT", "DEFAULT_FROM_EMAIL"] missing_settings = [ setting for setting in required_settings if not hasattr(settings, setting) or not getattr(settings, setting) ] if missing_settings: error_msg = f"Cannot send email notification: Missing email settings: {', '.join(missing_settings)}" logger.error(error_msg) return if not owner_email: logger.error("Cannot send email notification: No recipient email provided") return + # Validate email format + try: + validate_email(owner_email) + except ValidationError: + logger.error(f"Invalid email address format: {owner_email}") + return subject = f"CISO Assistant: You have {len(controls)} expired control(s)" message = "Hello,\n\nThe following controls have expired:\n\n" for control in controls: message += f"- {control.name} (ETA: {control.eta})\n" message += "\nThis reminder will stop once the control is marked as active or you update the ETA.\n" message += "Log in to your CISO Assistant portal and check 'my assignments' section to get to your controls directly.\n\n" message += "Thank you." try: send_mail( subject=subject, message=message, from_email=settings.DEFAULT_FROM_EMAIL, recipient_list=[owner_email], fail_silently=False, + headers={'X-Auto-Response': 'auto-generated'} ) logger.info(f"Successfully sent notification email to {owner_email}") except Exception as e: logger.error(f"Failed to send notification email to {owner_email}: {str(e)}") + raise # Propagate error for retry mechanism
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/tasks.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/tasks.py
3-3: huey.contrib.djhuey.periodic_task imported but unused
Remove unused import
(F401)
3-3: huey.contrib.djhuey.db_task imported but unused
Remove unused import
(F401)
5-5: django.db.models.Q imported but unused
Remove unused import: django.db.models.Q
(F401)
9-9: random imported but unused
Remove unused import: random
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
Summary by CodeRabbit
Documentation
New Features
Configuration
Performance