Refactor apply.sh container management script into labctl.py - #152
Conversation
WalkthroughThis change replaces shell-script-based Docker service management with a Python CLI tool ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant labctl.py
participant YAML Config
participant Docker Compose
User->>labctl.py: Run "labctl.py config apply"
labctl.py->>YAML Config: Load services.yaml
loop For each service in config
labctl.py->>Docker Compose: Execute (up/down/restart/pull/etc.) for service
end
labctl.py-->>User: Print summary/logs
sequenceDiagram
participant User
participant labctl.py
participant Docker Compose
User->>labctl.py: Run "labctl.py service <action> <service>"
labctl.py->>Docker Compose: Execute <action> for <service>
labctl.py-->>User: Print result
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
docs/setup.md (1)
112-116: Update examples to match new key nameIf the project standardises on
state:(see previous comment) these lines must be changed accordingly; otherwise newcomers will copy-paste a non-working example.
🧹 Nitpick comments (4)
Taskfile.yaml (1)
100-101: Empty{{.SERVICE}}produces a trailing space
taskpasses undefined template variables as empty strings.
IfSERVICEis optional the current command expands to
docker/labctl.py service up(note the trailing space) which some CLIs treat as an empty argument.Guard the argument or use a conditional:
cmds: - | if [ -n "{{.SERVICE}}" ]; then docker/labctl.py service {{.COMMAND}} {{.SERVICE}} else docker/labctl.py service {{.COMMAND}} ficonfig-example/docker/myhost/services.yaml (1)
3-19: Over-nested YAML complicates parsingUsing a list of single-key mappings forces consumers to iterate twice (
for category in services: for name in category:).
A flatter map is easier to read and deserialize:services: security: - name: crowdsec action: up - name: traefik action: up dashboard: - name: homepage action: upConsider switching before the structure ossifies.
docker/labctl.py (2)
171-174: Simplify if-else logic as suggested by static analysis.The static analysis hint is valid - this can be simplified using a binary operator.
Apply this diff to simplify the logic:
- if state_override: - state = state_override - else: - state = service.get('state', 'up') + state = state_override or service.get('state', 'up')
79-81: Consider adding command logging for better debugging.While the current implementation is correct, adding command logging could help with debugging and audit trails.
Consider adding debug logging:
def docker(cmd: list[str], env=None, stdin=None, stdout=None, stderr=None) -> None: + logger.debug(f"Executing: docker {' '.join(cmd)}") subprocess.run(["docker"] + cmd, env=env, stdin=stdin, stdout=stdout, stderr=stderr, check=True)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Taskfile.yaml(1 hunks)config-example/docker/myhost/apply.sh(0 hunks)config-example/docker/myhost/services.yaml(1 hunks)docker/README.md(2 hunks)docker/Taskfile.docker.yaml(3 hunks)docker/apply-local.sh(0 hunks)docker/backup.sh(2 hunks)docker/common.sh(0 hunks)docker/labctl.py(1 hunks)docs/setup.md(3 hunks)docs/web/Taskfile.web.yaml(2 hunks)
💤 Files with no reviewable changes (3)
- docker/apply-local.sh
- config-example/docker/myhost/apply.sh
- docker/common.sh
🧰 Additional context used
🧠 Learnings (1)
docs/setup.md (1)
Learnt from: bubacoder
PR: bubacoder/infra#89
File: ansible/bootstrap-ansible.sh:23-24
Timestamp: 2025-01-25T11:16:21.899Z
Learning: In the bootstrap-ansible.sh script, the `--ignore-installed` flag is only needed for pip installations on Ubuntu to fix PyYAML conflicts. The Debian installation works fine without this flag.
🪛 Ruff (0.12.2)
docker/labctl.py
171-174: Use binary operator state = state_override or service.get('state', 'up') instead of if-else-block
Replace if-else-block with state = state_override or service.get('state', 'up')
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (10)
docker/README.md (2)
20-21: Property name mismatch:actionvsstateThe docs say services are controlled with
action: up|down, but the sampleservices.yamlintroduced in this PR usesstate:.
Pick one term and use it consistently in docs, examples and code to avoid confusing users.
30-30: Good update to new CLI – looks correctdocker/Taskfile.docker.yaml (3)
12-12: LGTM: Clean migration to Python CLI.The migration from shell scripts to Python CLI calls is well-structured. The
config applycommand with different--modeflags provides a clean interface for different operations.Also applies to: 17-17, 22-22, 34-34
75-75: LGTM: Proper example generation for YAML configuration.The command correctly generates example YAML configuration files by replacing the hostname placeholder, which aligns with the new YAML-based configuration approach.
78-78: LGTM: Updated file tracking for YAML-based configuration.The changes properly update the sources and generates sections to track YAML configuration files instead of shell scripts, which is consistent with the new approach.
Also applies to: 82-82
docs/web/Taskfile.web.yaml (2)
7-7: LGTM: Excellent simplification of Docker command execution.The replacement of the multi-line shell script with a single Python CLI call significantly simplifies the task while maintaining the same functionality. This is a great improvement in maintainability.
37-37: LGTM: Service naming convention updated for new CLI.The change to combined service path format
tools/homelab-docsaligns with the new CLI interface that supports hierarchical service naming.docker/labctl.py (3)
42-55: LGTM: Robust symlink handling with proper error checking.The localhost symlink creation logic handles edge cases well, including checking for existing symlinks and providing appropriate error messages.
225-237: LGTM: Robust service name parsing logic.The service name parsing correctly handles hierarchical service names and provides appropriate error messages for invalid formats.
240-271: LGTM: Well-structured CLI argument parsing.The argument parsing structure is clean and follows good practices with proper subcommands and help text.
| set -euo pipefail | ||
|
|
||
| DOCKER_VOLUMES="/srv/docker-volumes" | ||
| DOCKER_VOLUMES="/srv/docker-volumes" # TODO configure |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DOCKER_VOLUMES should not be hard-wired
/srv/docker-volumes is unlikely to be correct on every host and is currently flagged with a TODO.
Expose this as an environment variable with a sensible default so the script can be reused unchanged across hosts.
-DOCKER_VOLUMES="/srv/docker-volumes" # TODO configure
+: "${DOCKER_VOLUMES:=/srv/docker-volumes}" # Allow override via env var🤖 Prompt for AI Agents
In docker/backup.sh at line 4, the DOCKER_VOLUMES path is hardcoded to
/srv/docker-volumes, which is not flexible for different hosts. Modify the
script to read DOCKER_VOLUMES from an environment variable if set, otherwise
default to /srv/docker-volumes. This allows the path to be customized per host
without changing the script.
3920b33 to
8c8eb56
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docker/labctl.py (2)
57-59: Consider using Path objects for consistency.The function works correctly but mixing string paths with Path objects elsewhere in the code reduces consistency.
def get_yaml_file(stack_dir: str, service_name: str) -> str: """Get the yaml file path for a service.""" - return os.path.join(stack_dir, f"{service_name}.yaml") + return str(Path(stack_dir) / f"{service_name}.yaml")
147-184: Simplify conditional logic per static analysis suggestion.The static analysis correctly identifies an opportunity to simplify the conditional logic.
# Determine stack directory stack_dir = os.path.join(docker_stacks_dir, category) - if state_override: - state = state_override - else: - state = service.get('state', 'up') + state = state_override or service.get('state', 'up') if state in ('up', 'update', 'pull', 'down', 'restart', 'recreate'): docker_command(stack_dir, name, state) else: logger.warning(f"Unknown state '{state}' for service {category}/{name}")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Taskfile.yaml(1 hunks)config-example/docker/myhost/apply.sh(0 hunks)config-example/docker/myhost/services.yaml(1 hunks)docker/README.md(2 hunks)docker/Taskfile.docker.yaml(3 hunks)docker/apply-local.sh(0 hunks)docker/backup.sh(2 hunks)docker/common.sh(0 hunks)docker/labctl.py(1 hunks)docs/setup.md(3 hunks)docs/web/Taskfile.web.yaml(2 hunks)
💤 Files with no reviewable changes (3)
- docker/apply-local.sh
- config-example/docker/myhost/apply.sh
- docker/common.sh
✅ Files skipped from review due to trivial changes (1)
- docs/web/Taskfile.web.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- docker/README.md
- docker/backup.sh
- config-example/docker/myhost/services.yaml
- docs/setup.md
- docker/Taskfile.docker.yaml
- Taskfile.yaml
🧰 Additional context used
🪛 Ruff (0.12.2)
docker/labctl.py
175-178: Use binary operator state = state_override or service.get('state', 'up') instead of if-else-block
Replace if-else-block with state = state_override or service.get('state', 'up')
(SIM108)
🔇 Additional comments (10)
docker/labctl.py (10)
1-24: Good module structure and type annotations.The imports are well-organized and the global variable typing is properly handled. The logging configuration is appropriate for a CLI tool.
26-33: LGTM: Network creation logic is solid.The function correctly checks for network existence and creates it only if missing. Good use of subprocess.DEVNULL for silent checks.
79-81: Good subprocess wrapper with proper error handling.The function correctly uses check=True to raise exceptions on command failure, which is appropriate for Docker commands.
125-133: LGTM: Proper YAML parsing implementation.This addresses the past review comment about using proper YAML parsing instead of string search. The implementation correctly parses YAML and checks for build directives.
136-144: Good configuration loading with error handling.The function properly handles YAML loading errors and exits with appropriate error codes.
186-190: LGTM: Clean path construction.The function correctly constructs the host configuration directory path using Path objects.
193-218: Good command implementation with proper global variable handling.The function properly handles the global variable and implements the configuration apply logic correctly.
244-272: Well-structured CLI interface.The argument parsing is comprehensive and follows good CLI design principles with proper subcommands and help text.
274-275: Standard Python entry point.Correct implementation of the standard Python entry point pattern.
62-76: Remove unnecessary None-check:host_config_diris always set before use
The globalhost_config_diris assigned in bothcmd_config_apply(lines 195–203) andcmd_service(lines 221–224) before any call toget_env_file_args. There is no execution path whereget_env_file_argsruns withhost_config_dirstillNone, so no guard againstNoneis needed here.Likely an incorrect or invalid review comment.
| def cmd_service(args) -> None: | ||
| """Manage individual Docker services.""" | ||
| global host_config_dir | ||
| host_config_dir = get_host_config_dir() | ||
|
|
||
| if not args.name: | ||
| logger.error("Service name is required") | ||
| sys.exit(1) | ||
|
|
||
| # Parse service name in format category/subcategory/name | ||
| name_parts = args.name.split('/') | ||
| if len(name_parts) < 2: | ||
| logger.error("Service name must be in format category/name or category/subcategory/name") | ||
| sys.exit(1) | ||
|
|
||
| # The last part is always the service name | ||
| service_name = name_parts[-1] | ||
| # Everything before the last part is the category path | ||
| category_path = '/'.join(name_parts[:-1]) | ||
|
|
||
| stack_dir = os.path.join(docker_stacks_dir, category_path) | ||
| docker_command(stack_dir, service_name, args.operation) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve input validation and error messages.
The service name parsing logic could be more robust and provide better error messages.
def cmd_service(args) -> None:
"""Manage individual Docker services."""
global host_config_dir
host_config_dir = get_host_config_dir()
if not args.name:
logger.error("Service name is required")
sys.exit(1)
# Parse service name in format category/subcategory/name
name_parts = args.name.split('/')
+ if not all(part.strip() for part in name_parts):
+ logger.error("Service name parts cannot be empty")
+ sys.exit(1)
+
if len(name_parts) < 2:
logger.error("Service name must be in format category/name or category/subcategory/name")
sys.exit(1)
# The last part is always the service name
service_name = name_parts[-1]
# Everything before the last part is the category path
category_path = '/'.join(name_parts[:-1])
stack_dir = os.path.join(docker_stacks_dir, category_path)
+ if not os.path.exists(stack_dir):
+ logger.error(f"Stack directory not found: {stack_dir}")
+ sys.exit(1)
+
docker_command(stack_dir, service_name, args.operation)📝 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.
| def cmd_service(args) -> None: | |
| """Manage individual Docker services.""" | |
| global host_config_dir | |
| host_config_dir = get_host_config_dir() | |
| if not args.name: | |
| logger.error("Service name is required") | |
| sys.exit(1) | |
| # Parse service name in format category/subcategory/name | |
| name_parts = args.name.split('/') | |
| if len(name_parts) < 2: | |
| logger.error("Service name must be in format category/name or category/subcategory/name") | |
| sys.exit(1) | |
| # The last part is always the service name | |
| service_name = name_parts[-1] | |
| # Everything before the last part is the category path | |
| category_path = '/'.join(name_parts[:-1]) | |
| stack_dir = os.path.join(docker_stacks_dir, category_path) | |
| docker_command(stack_dir, service_name, args.operation) | |
| def cmd_service(args) -> None: | |
| """Manage individual Docker services.""" | |
| global host_config_dir | |
| host_config_dir = get_host_config_dir() | |
| if not args.name: | |
| logger.error("Service name is required") | |
| sys.exit(1) | |
| # Parse service name in format category/subcategory/name | |
| name_parts = args.name.split('/') | |
| if not all(part.strip() for part in name_parts): | |
| logger.error("Service name parts cannot be empty") | |
| sys.exit(1) | |
| if len(name_parts) < 2: | |
| logger.error("Service name must be in format category/name or category/subcategory/name") | |
| sys.exit(1) | |
| # The last part is always the service name | |
| service_name = name_parts[-1] | |
| # Everything before the last part is the category path | |
| category_path = '/'.join(name_parts[:-1]) | |
| stack_dir = os.path.join(docker_stacks_dir, category_path) | |
| if not os.path.exists(stack_dir): | |
| logger.error(f"Stack directory not found: {stack_dir}") | |
| sys.exit(1) | |
| docker_command(stack_dir, service_name, args.operation) |
🤖 Prompt for AI Agents
In docker/labctl.py around lines 220 to 242, improve the input validation for
the service name by ensuring it strictly matches the expected formats
(category/name or category/subcategory/name) and provide clearer, more specific
error messages when the format is incorrect. Update the validation logic to
explicitly check for these cases and enhance the error messages to guide the
user on the correct input format.
| def create_localhost_link() -> None: | ||
| """Create 'localhost' symlink in the parent directory.""" | ||
| parent_dir = Path(host_config_dir).parent | ||
| hostname = socket.gethostname() | ||
| localhost_link = parent_dir / "localhost" | ||
| hostname_dir = parent_dir / hostname | ||
|
|
||
| if hostname_dir.exists() and hostname_dir.is_dir(): | ||
| # Create or update the localhost symlink | ||
| if localhost_link.exists(): | ||
| if localhost_link.is_symlink(): | ||
| localhost_link.unlink() | ||
| else: | ||
| logger.error(f"Error: {localhost_link} exists but is not a symlink. Cannot create link.") | ||
| return | ||
|
|
||
| try: | ||
| os.symlink(f"{hostname}/", localhost_link, target_is_directory=True) | ||
| except Exception as e: | ||
| logger.error(f"Error creating localhost symlink: {e}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and type safety.
The function has good logic for creating localhost symlinks but has a potential issue with global variable usage and could benefit from more specific exception handling.
def create_localhost_link() -> None:
"""Create 'localhost' symlink in the parent directory."""
+ if host_config_dir is None:
+ logger.error("host_config_dir is not initialized")
+ return
+
parent_dir = Path(host_config_dir).parent
hostname = socket.gethostname()
localhost_link = parent_dir / "localhost"
hostname_dir = parent_dir / hostname
if hostname_dir.exists() and hostname_dir.is_dir():
# Create or update the localhost symlink
if localhost_link.exists():
if localhost_link.is_symlink():
localhost_link.unlink()
else:
logger.error(f"Error: {localhost_link} exists but is not a symlink. Cannot create link.")
return
try:
os.symlink(f"{hostname}/", localhost_link, target_is_directory=True)
- except Exception as e:
+ except OSError as e:
logger.error(f"Error creating localhost symlink: {e}")📝 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.
| def create_localhost_link() -> None: | |
| """Create 'localhost' symlink in the parent directory.""" | |
| parent_dir = Path(host_config_dir).parent | |
| hostname = socket.gethostname() | |
| localhost_link = parent_dir / "localhost" | |
| hostname_dir = parent_dir / hostname | |
| if hostname_dir.exists() and hostname_dir.is_dir(): | |
| # Create or update the localhost symlink | |
| if localhost_link.exists(): | |
| if localhost_link.is_symlink(): | |
| localhost_link.unlink() | |
| else: | |
| logger.error(f"Error: {localhost_link} exists but is not a symlink. Cannot create link.") | |
| return | |
| try: | |
| os.symlink(f"{hostname}/", localhost_link, target_is_directory=True) | |
| except Exception as e: | |
| logger.error(f"Error creating localhost symlink: {e}") | |
| def create_localhost_link() -> None: | |
| """Create 'localhost' symlink in the parent directory.""" | |
| if host_config_dir is None: | |
| logger.error("host_config_dir is not initialized") | |
| return | |
| parent_dir = Path(host_config_dir).parent | |
| hostname = socket.gethostname() | |
| localhost_link = parent_dir / "localhost" | |
| hostname_dir = parent_dir / hostname | |
| if hostname_dir.exists() and hostname_dir.is_dir(): | |
| # Create or update the localhost symlink | |
| if localhost_link.exists(): | |
| if localhost_link.is_symlink(): | |
| localhost_link.unlink() | |
| else: | |
| logger.error(f"Error: {localhost_link} exists but is not a symlink. Cannot create link.") | |
| return | |
| try: | |
| os.symlink(f"{hostname}/", localhost_link, target_is_directory=True) | |
| except OSError as e: | |
| logger.error(f"Error creating localhost symlink: {e}") |
🤖 Prompt for AI Agents
In docker/labctl.py around lines 35 to 55, the function create_localhost_link
uses the global variable host_config_dir without explicit passing or
declaration, which can cause issues. Refactor the function to accept
host_config_dir as a parameter to avoid relying on globals. Additionally,
replace the broad Exception catch with more specific exceptions related to
symlink creation, such as OSError, to improve error handling and clarity.
| def docker_command(stack_dir: str, service_name: str, action: str) -> None: | ||
| """Execute Docker Compose command for a service.""" | ||
| print() # empty line for separation | ||
|
|
||
| yaml_file = get_yaml_file(stack_dir, service_name) | ||
| if not os.path.exists(yaml_file): | ||
| logger.error(f"YAML file not found: {yaml_file}") | ||
| return | ||
|
|
||
| env_file_args = get_env_file_args(service_name) | ||
|
|
||
| # Handle pull operations | ||
| if action in ["update", "pull"]: | ||
| logger.info(f">>> Pulling {stack_dir}/{service_name}") | ||
|
|
||
| if has_build_directive(yaml_file): | ||
| # Bake: https://docs.docker.com/guides/compose-bake/ | ||
| env = os.environ.copy() | ||
| env["COMPOSE_BAKE"] = "true" | ||
| docker(["compose", "-f", yaml_file, *env_file_args, "build", "--pull"], env=env) | ||
| else: | ||
| docker(["compose", "-f", yaml_file, *env_file_args, "pull"]) | ||
|
|
||
| # Handle other operations | ||
| match action: | ||
| case "up" | "update": | ||
| logger.info(f">>> Starting {stack_dir}/{service_name}") | ||
| docker(["compose", "-f", yaml_file, *env_file_args, "up", "--detach"]) | ||
|
|
||
| case "down": | ||
| logger.info(f">>> Stopping {stack_dir}/{service_name}") | ||
| docker(["compose", "-f", yaml_file, *env_file_args, "down"]) | ||
|
|
||
| case "restart": | ||
| logger.info(f">>> Restarting {stack_dir}/{service_name}") | ||
| docker(["compose", "-f", yaml_file, *env_file_args, "restart"]) | ||
|
|
||
| case "recreate": | ||
| logger.info(f">>> Recreating {stack_dir}/{service_name}") | ||
| docker(["compose", "-f", yaml_file, *env_file_args, "up", "--detach", "--force-recreate"]) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and consider separating concerns.
The function handles multiple operations but could benefit from better error handling and separation of concerns.
def docker_command(stack_dir: str, service_name: str, action: str) -> None:
"""Execute Docker Compose command for a service."""
print() # empty line for separation
yaml_file = get_yaml_file(stack_dir, service_name)
if not os.path.exists(yaml_file):
logger.error(f"YAML file not found: {yaml_file}")
- return
+ sys.exit(1) # Exit with error code for missing files
env_file_args = get_env_file_args(service_name)
# Handle pull operations
if action in ["update", "pull"]:
logger.info(f">>> Pulling {stack_dir}/{service_name}")
- if has_build_directive(yaml_file):
- # Bake: https://docs.docker.com/guides/compose-bake/
- env = os.environ.copy()
- env["COMPOSE_BAKE"] = "true"
- docker(["compose", "-f", yaml_file, *env_file_args, "build", "--pull"], env=env)
- else:
- docker(["compose", "-f", yaml_file, *env_file_args, "pull"])
+ try:
+ if has_build_directive(yaml_file):
+ # Bake: https://docs.docker.com/guides/compose-bake/
+ env = os.environ.copy()
+ env["COMPOSE_BAKE"] = "true"
+ docker(["compose", "-f", yaml_file, *env_file_args, "build", "--pull"], env=env)
+ else:
+ docker(["compose", "-f", yaml_file, *env_file_args, "pull"])
+ except subprocess.CalledProcessError as e:
+ logger.error(f"Failed to pull {service_name}: {e}")
+ sys.exit(1)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docker/labctl.py around lines 83 to 123, the docker_command function
currently handles multiple actions with minimal error handling and mixed
concerns. Refactor by splitting the function into smaller functions, each
responsible for a specific action (e.g., pull, up, down). Add try-except blocks
around docker command executions to catch and log exceptions clearly. This
improves readability, maintainability, and robustness of error handling.
8c8eb56 to
69b01c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
docker/labctl.py (3)
35-55: Address the unresolved global variable usage and exception handling issues.This function still has the issues mentioned in past review comments that appear to be unaddressed:
- Direct usage of global
host_config_dirwithout proper validation- Broad exception handling that should be more specific
The function should validate that
host_config_diris properly initialized and use more specific exception handling:def create_localhost_link() -> None: """Create 'localhost' symlink in the parent directory.""" + if host_config_dir is None: + logger.error("host_config_dir is not initialized") + return + parent_dir = Path(host_config_dir).parent hostname = socket.gethostname() localhost_link = parent_dir / "localhost" hostname_dir = parent_dir / hostname if hostname_dir.exists() and hostname_dir.is_dir(): # Create or update the localhost symlink if localhost_link.exists(): if localhost_link.is_symlink(): localhost_link.unlink() else: logger.error(f"Error: {localhost_link} exists but is not a symlink. Cannot create link.") return try: os.symlink(f"{hostname}/", localhost_link, target_is_directory=True) - except Exception as e: + except OSError as e: logger.error(f"Error creating localhost symlink: {e}")
215-237: Address unresolved input validation issues.The service name validation still has the issues mentioned in past review comments that appear to be unaddressed:
- No validation for empty parts in the service name
- No verification that the stack directory exists
def cmd_service(args) -> None: """Manage individual Docker services.""" global host_config_dir host_config_dir = get_host_config_dir() if not args.name: logger.error("Service name is required") sys.exit(1) # Parse service name in format category/subcategory/name name_parts = args.name.split('/') + if not all(part.strip() for part in name_parts): + logger.error("Service name parts cannot be empty") + sys.exit(1) + if len(name_parts) < 2: logger.error("Service name must be in format category/name or category/subcategory/name") sys.exit(1) # The last part is always the service name service_name = name_parts[-1] # Everything before the last part is the category path category_path = '/'.join(name_parts[:-1]) stack_dir = os.path.join(docker_stacks_dir, category_path) + if not os.path.exists(stack_dir): + logger.error(f"Stack directory not found: {stack_dir}") + sys.exit(1) + docker_command(stack_dir, service_name, args.operation)
94-102: Improve error handling consistency.The function returns early for missing files instead of exiting with an error code, which is inconsistent with other error handling patterns in the codebase.
compose_file = get_compose_file(stack_dir, service_name) if not os.path.exists(compose_file): logger.error(f"Compose file not found: {compose_file}") - return + sys.exit(1)
🧹 Nitpick comments (1)
docker/labctl.py (1)
212-212: Make cleanup duration configurable.The hard-coded
504h(21 days) filter value should be configurable to allow different cleanup policies.Consider adding a configuration option or command-line argument for the cleanup duration:
- docker(["image", "prune", "--all", "--force", "--filter", "until=504h"]) + cleanup_duration = config.get('cleanup_duration', '504h') + docker(["image", "prune", "--all", "--force", "--filter", f"until={cleanup_duration}"])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Taskfile.yaml(1 hunks)config-example/docker/myhost/apply.sh(0 hunks)config-example/docker/myhost/services.yaml(1 hunks)docker/README.md(2 hunks)docker/Taskfile.docker.yaml(3 hunks)docker/apply-local.sh(0 hunks)docker/backup.sh(2 hunks)docker/common.sh(0 hunks)docker/labctl.py(1 hunks)docs/setup.md(3 hunks)docs/web/Taskfile.web.yaml(2 hunks)
💤 Files with no reviewable changes (3)
- docker/apply-local.sh
- config-example/docker/myhost/apply.sh
- docker/common.sh
✅ Files skipped from review due to trivial changes (1)
- docs/web/Taskfile.web.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- docker/README.md
- Taskfile.yaml
- config-example/docker/myhost/services.yaml
- docs/setup.md
- docker/backup.sh
- docker/Taskfile.docker.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (3)
docker/labctl.py (3)
62-71: LGTM! Build detection properly implemented.The function now uses proper YAML parsing instead of string search, which addresses the previous review comment about fragile build detection.
188-199: LGTM! Host config directory handling properly implemented.The function correctly handles the type consistency for
host_config_diras a Path object, addressing the previous review comment.
147-179: LGTM! Robust service processing logic.The function has good validation for the configuration structure, proper error handling for invalid entries, and clear logging for debugging.
| def load_services_config(config_file: str) -> Dict: | ||
| """Load services configuration from YAML file.""" | ||
| try: | ||
| with open(config_file, 'r') as file: | ||
| config = yaml.safe_load(file) | ||
| return config | ||
| except Exception as e: | ||
| logger.error(f"Error loading configuration file {config_file}: {e}") | ||
| sys.exit(1) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace broad exception handling with specific exceptions.
The function uses broad Exception handling which makes debugging difficult and could mask unexpected errors.
def load_services_config(config_file: str) -> Dict:
"""Load services configuration from YAML file."""
try:
with open(config_file, 'r') as file:
config = yaml.safe_load(file)
return config
- except Exception as e:
+ except (FileNotFoundError, yaml.YAMLError, IOError) as e:
logger.error(f"Error loading configuration file {config_file}: {e}")
sys.exit(1)📝 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.
| def load_services_config(config_file: str) -> Dict: | |
| """Load services configuration from YAML file.""" | |
| try: | |
| with open(config_file, 'r') as file: | |
| config = yaml.safe_load(file) | |
| return config | |
| except Exception as e: | |
| logger.error(f"Error loading configuration file {config_file}: {e}") | |
| sys.exit(1) | |
| def load_services_config(config_file: str) -> Dict: | |
| """Load services configuration from YAML file.""" | |
| try: | |
| with open(config_file, 'r') as file: | |
| config = yaml.safe_load(file) | |
| return config | |
| except (FileNotFoundError, yaml.YAMLError, IOError) as e: | |
| logger.error(f"Error loading configuration file {config_file}: {e}") | |
| sys.exit(1) |
🤖 Prompt for AI Agents
In docker/labctl.py around lines 136 to 145, replace the broad Exception catch
in the load_services_config function with more specific exceptions such as
FileNotFoundError, yaml.YAMLError, or IOError. This will make error handling
clearer and debugging easier by only catching expected errors related to file
access and YAML parsing.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores