refactor: Refactor into python - #5
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Docker Autoheal application from a shell script-based implementation to a Python-based implementation. The refactoring modernizes the codebase, improves maintainability, and adopts a more structured approach using Python's ecosystem.
Key Changes:
- Complete rewrite from shell script to Python, creating a new
autohealpackage with modular components - Updated Dockerfile to use Python 3.14.1-alpine base image and install Python dependencies
- Removed all test infrastructure (bash test scripts, Docker Compose test files)
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
docker-entrypoint |
Removed 270-line shell script containing the original autoheal logic |
autoheal/__init__.py |
Added package initialization with version declaration |
autoheal/__main__.py |
Added main entry point with signal handling and configuration logging |
autoheal/config.py |
Added configuration management using dataclass for environment variables |
autoheal/docker_api.py |
Added Docker API interaction layer using the docker-py library |
autoheal/health_monitor.py |
Added container health monitoring loop |
autoheal/logging_utils.py |
Added custom logging formatter with emoji indicators |
autoheal/notifications.py |
Added webhook, Apprise, and post-restart script notification handlers |
autoheal/restart_tracker.py |
Added restart tracking functionality with file-based persistence |
pyproject.toml |
Added Python project configuration for packaging |
Dockerfile |
Updated to use Python base image and install Python dependencies |
tests/* |
Removed all test infrastructure files |
.gitignore / .dockerignore |
Added Python-specific ignore patterns |
.github/workflows/docker-publish-dev.yml |
Added dev branch Docker publishing workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "requests", | ||
| "docker" |
There was a problem hiding this comment.
Missing version constraints for dependencies. Both requests and docker should have version constraints to ensure reproducible builds and avoid potential compatibility issues. Consider using version specifiers like requests>=2.31.0,<3.0.0 and docker>=7.0.0,<8.0.0.
| "requests", | |
| "docker" | |
| "requests>=2.31.0,<3.0.0", | |
| "docker>=7.0.0,<8.0.0" |
| notify_webhook(f"Container {container_name} ({short_id}) exceeded restart threshold. Stopped!", config) | ||
| else: | ||
| logger.error(f"Failed to stop container {container_name} ({short_id})") | ||
| notify_webhook(f"Container {container_name} ({short_id}) exceeded restart threshold. Failed to stop!", config) | ||
| else: | ||
| logger.info(f"Container {container_name} ({short_id}) found to be unhealthy - Restarting") | ||
| if restart_container(container_id, int(stop_timeout), config): | ||
| logger.info(f"Successfully restarted container {container_name} ({short_id})") | ||
| notify_webhook(f"Container {container_name} ({short_id}) found to be unhealthy. Restarted!", config) | ||
| else: | ||
| logger.error(f"Failed to restart container {container_name} ({short_id})") | ||
| notify_webhook(f"Container {container_name} ({short_id}) found to be unhealthy. Failed to restart!", config) |
There was a problem hiding this comment.
Container name formatting inconsistency: The old shell script stripped the leading "/" from container names using ${CONTAINER_NAME:1} in webhook notifications (see original line 244, 248, 252, 256). The Python version doesn't strip this prefix, which may result in container names like "/my-container" in notifications instead of "my-container".
| COPY autoheal/ /app/autoheal/ | ||
|
|
||
| # Install Python dependencies | ||
| RUN pip install --no-cache-dir requests docker |
There was a problem hiding this comment.
Dependencies are installed without version constraints. This can lead to non-reproducible builds and potential compatibility issues. Consider pinning versions: pip install --no-cache-dir requests==2.31.0 docker==7.0.0 (adjust versions as needed).
| RUN pip install --no-cache-dir requests docker | |
| RUN pip install --no-cache-dir requests==2.31.0 docker==7.0.0 |
| try: | ||
| requests.post(config.webhook_url, json=payload, timeout=30) | ||
| logger.debug("Sent webhook notification") | ||
| except requests.RequestException as e: | ||
| logger.error(f"Failed to send webhook: {e}") |
There was a problem hiding this comment.
The webhook notification is now synchronous (blocking), while the original shell script executed webhooks as background processes (&) to prevent blocking. This could cause delays in the monitoring loop if the webhook endpoint is slow or unresponsive. Consider using threading or asyncio to send notifications asynchronously.
| def update_restart_info(self, container_id: str, first_time: int, count: int): | ||
| """Update restart info for a container.""" | ||
| lines = [] | ||
| with open(self.tracker_file, "r") as f: | ||
| lines = f.readlines() | ||
|
|
||
| # Remove existing entry | ||
| lines = [line for line in lines if not line.startswith(container_id + " ")] | ||
|
|
||
| # Add new entry | ||
| lines.append(f"{container_id} {first_time} {count}\n") | ||
|
|
||
| with open(self.tracker_file, "w") as f: | ||
| f.writelines(lines) |
There was a problem hiding this comment.
Race condition: The file operations in update_restart_info() read, modify, and write the tracker file without synchronization. If the monitoring loop processes multiple containers concurrently in the future, or if multiple instances run, this could lead to data corruption or lost updates. Consider using file locking (e.g., fcntl.flock) or a thread-safe data structure.
| if key.lower() == 'webhook_url': | ||
| # Mask sensitive URLs | ||
| str_value = str(value) | ||
| if str_value: | ||
| masked_value = str_value[:20] + '...' if len(str_value) > 20 else '***' | ||
| else: | ||
| masked_value = str_value |
There was a problem hiding this comment.
The masking logic for sensitive URLs only applies to 'webhook_url' (case-sensitive check), but there are other potentially sensitive values like 'apprise_url' that should also be masked. Consider applying the masking logic to all URL-related configuration values.
| if len(sys.argv) > 1 and sys.argv[1] == "autoheal": | ||
| main() | ||
| else: | ||
| # Exec other commands |
There was a problem hiding this comment.
Index error: os.execvp(sys.argv[1], sys.argv[1:]) will fail when len(sys.argv) == 1 (no arguments provided). This code path is reached when the script is run without arguments or with arguments other than "autoheal". Add a check or error message for this case.
| # Exec other commands | |
| # Exec other commands | |
| if len(sys.argv) == 1: | |
| print("Error: No command provided to execute.", file=sys.stderr) | |
| sys.exit(1) |
| try: | ||
| requests.post(config.apprise_url, json=payload, timeout=30) | ||
| logger.debug("Sent Apprise notification") | ||
| except requests.RequestException as e: | ||
| logger.error(f"Failed to send Apprise: {e}") |
There was a problem hiding this comment.
Similar to the webhook notification, the Apprise notification is now synchronous (blocking), while the original shell script executed it as a background process (&) to prevent blocking. This could cause delays in the monitoring loop. Consider using threading or asyncio to send notifications asynchronously.
| @@ -0,0 +1,72 @@ | |||
| """Health monitoring logic.""" | |||
|
|
|||
| import time | |||
There was a problem hiding this comment.
Import of 'time' is not used.
| import time |
| @@ -0,0 +1,47 @@ | |||
| """Notification handling for webhooks and Apprise.""" | |||
|
|
|||
| import json | |||
There was a problem hiding this comment.
Import of 'json' is not used.
| import json |
No description provided.