Move Docker Compose stacks to per-app subdirectories - #266
Conversation
WalkthroughMoves many Docker compose source mounts into a centralized Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(40,116,166,0.5)
participant Config as "Prometheus config\n(disk: docker/monitoring/prometheus/config/prometheus.yaml)"
end
rect rgba(33,150,83,0.5)
participant Prom as Prometheus
end
rect rgba(255,152,0,0.5)
participant Node as node_exporter
participant Cad as cadvisor
end
rect rgba(156,39,176,0.5)
participant Targets as "Monitored Targets\n(servers, NAS, containers)"
end
Config->>Prom: mount configuration
Prom->>Node: scrape /metrics (configured interval)
Prom->>Cad: scrape /metrics (configured interval)
Node->>Targets: collect host metrics
Cad->>Targets: collect container metrics
Node-->>Prom: return metrics
Cad-->>Prom: return metrics
Prom->>Prom: store/aggregate metrics and expose API
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
4c4c04c to
ac9fc5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/web/compose_processor.py`:
- Around line 85-91: The loop over services assumes labels is a dict and calls
labels.items(), which breaks when Docker Compose provides labels as a list;
update the logic in the function that iterates services (the block using
variables services, labels, and homepage_labels) to normalize labels first: if
labels is a list, convert it into a dict by splitting each "KEY=VALUE" entry
into key and value (skip malformed entries), otherwise use the dict as-is, then
proceed to build homepage_labels by filtering keys that start with "homepage."
and stripping that prefix before returning.
In `@docs/web/export-services.py`:
- Line 69: The code builds documentation by concatenating service["head_lines"]
into the documentation variable using "".join which removes all separators and
collapses multi-line comments; change the join to
"\n".join(service["head_lines"]) so line breaks are preserved (and keep the
existing .strip() if you still want to trim leading/trailing whitespace); update
the assignment to documentation to use the newline-joined string to avoid losing
original comment line breaks coming from head_lines as prepared in
compose_processor.py.
In `@docs/web/update-docs.py`:
- Around line 239-265: In _build_service_frontmatter, avoid the KeyError by
validating metadata contains the required "name" key before accessing
metadata["name"]; add an early check (e.g., if "name" not in metadata: raise
ValueError with a clear message including the offending metadata or service
identifier) so the function fails with a descriptive error instead of a
KeyError, then proceed to build lines as before.
🧹 Nitpick comments (10)
docker/monitoring/prometheus/prometheus.yaml (1)
20-20: Consider documenting or restricting the root user requirement.Running the Prometheus container as
user: "0:0"(root) is a security concern. While this may be necessary for write access to the volume at${DOCKER_VOLUMES}/prometheus, consider either:
- Adding a comment explaining why root is required
- Pre-creating the volume directory with appropriate ownership so a non-root user can be used
docs/web/git_utils.py (2)
36-37: Consider removing redundantFileNotFoundErrorhandling.The
FileNotFoundErrorcatch at line 36-37 is unlikely to trigger sinceshutil.which("git")at line 25-27 already validates the git executable exists. This exception would only occur in an extremely rare race condition where the executable is removed between the check and execution.While keeping it isn't harmful (it provides defense-in-depth), you could simplify by removing this branch if you prefer a leaner implementation.
27-27: Ruff TRY003: Long exception messages outside the class.The static analysis tool flags that long messages should be defined within the exception class. This is a minor style preference—the current approach is readable and functional.
If you want to suppress these warnings, you can add class-level default messages:
♻️ Optional refactor to address TRY003
class GitExecutableNotFoundError(RuntimeError): """Raised when Git executable cannot be found.""" + def __init__(self, message: str = "Git executable not found. Please install Git and ensure it is on your PATH.") -> None: + super().__init__(message) class NotInGitRepositoryError(RuntimeError): """Raised when not running inside a Git repository.""" + def __init__(self, message: str = "Unable to locate git repository. Are you running this inside a Git repo?") -> None: + super().__init__(message)Then simplify the raise statements:
- raise GitExecutableNotFoundError("Git executable not found. Please install Git and ensure it is on your PATH.") + raise GitExecutableNotFoundError()Also applies to: 37-37, 39-39
docs/web/export-services.py (1)
35-57: Logger configuration has asymmetric behavior.In verbose mode, a
StreamHandlerwith formatting is added, but in non-verbose mode, only the level is set without adding a handler. This means non-verbose logging depends on the root logger's configuration.This works in practice because
logging.info()falls back to the root logger, but it's worth noting for maintainability. If explicit handler control in non-verbose mode is desired later, consider adding a handler in both branches.docs/web/compose_processor.py (1)
64-65: Specify explicit encoding for file operations.The
open()calls at lines 64 and 111 don't specify encoding, defaulting to the system locale. For Docker Compose files (which are typically UTF-8), explicitly specifyingencoding="utf-8"ensures consistent behavior across different environments.♻️ Proposed fix
- with open(file_path) as stream: + with open(file_path, encoding="utf-8") as stream: compose_dict = yaml.safe_load(stream)- with open(source_file_path) as compose_file: + with open(source_file_path, encoding="utf-8") as compose_file: lines = compose_file.readlines()Also applies to: 111-112
docs/web/update-docs.py (4)
44-63: Logger handler may accumulate on repeated instantiation.If
DocsProcessoris instantiated multiple times (e.g., in tests), the handler gets added each time without checking for existing handlers. This can cause duplicate log messages.♻️ Suggested fix to prevent duplicate handlers
def _setup_logging(self, verbose: bool) -> logging.Logger: """Configure and return logger instance. Args: verbose: Whether to enable debug logging Returns: Configured logger instance """ logger = logging.getLogger(__name__) logger.setLevel(logging.DEBUG if verbose else logging.INFO) - if verbose: + if verbose and not logger.handlers: handler = logging.StreamHandler() handler.setLevel(logging.DEBUG) formatter = logging.Formatter(" %(levelname)s: %(message)s") handler.setFormatter(formatter) logger.addHandler(handler) return logger
65-84: Return type annotation doesn't match actual YAML structure.The method returns
data.get("locations", [])which returns whatever is in the YAML file. If the YAML contains a list of lists instead of a list of tuples, Python will accept it but the type hintlist[tuple[str, str, int]]won't be enforced at runtime. Consider validating the structure or documenting the expected YAML format.Also, the empty list fallback
[]would silently continue with no locations rather than failing explicitly if the config is malformed.♻️ Consider adding basic validation
try: with config_path.open() as config_file: data = yaml.safe_load(config_file) - return data.get("locations", []) + locations = data.get("locations") + if not locations: + self.logger.warning(f"No locations found in configuration file: {config_path}") + return [(src, tgt, weight) for src, tgt, weight in locations] if locations else [] except FileNotFoundError:
104-112: Missing directory recreation after clearing.The
_clear_directorymethod only recreates the directory if it existed before. If the directory doesn't exist initially, it won't be created. This is inconsistent with the method name "clear" which implies the directory should exist afterward.♻️ Ensure directory always exists after clearing
def _clear_directory(self, directory: Path) -> None: """Delete all content in directory and recreate it. Args: directory: Directory path to clear """ if directory.exists() and directory.is_dir(): shutil.rmtree(directory) - self._ensure_directory(directory) + self._ensure_directory(directory)
195-217: Potential iteration issue when source directory doesn't exist.If
source_dirdoesn't exist,source_dir.iterdir()will raiseFileNotFoundError. Consider adding existence check or catching the exception.♻️ Add existence check
def _process_directory( self, source_relative: str, target_relative: str, weight: int = 0, ) -> None: """Process all markdown files in a directory. Args: source_relative: Source directory path relative to repository root target_relative: Target directory path relative to output content directory weight: Hugo weight parameter for ordering """ source_dir = self.repository_path / source_relative + if not source_dir.exists(): + self.logger.warning(f"Source directory does not exist: {source_dir}") + return + for filename in source_dir.iterdir():docs/web/link_processor.py (1)
217-244: Regex substitution may replace unintended occurrences.Using
re.subwithout a count limit will replace all occurrences of the same link pattern. If the same link appears multiple times with different intended targets, this could cause issues. Consider usingcount=1or a more targeted replacement strategy.♻️ Limit substitution to first occurrence
original_link_pattern = re.escape(f"[{link_text}]({link_url})") new_link = f"[{link_text}]({updated_link})" - return re.sub(original_link_pattern, new_link, content) + return re.sub(original_link_pattern, new_link, content, count=1)However, since the same link text+url combination should logically resolve to the same target, replacing all occurrences may be intentional. Consider documenting this behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/web/update-docs.py`:
- Around line 239-252: The ValueError raised in _build_service_frontmatter
contains a long formatted metadata object which triggers Ruff TRY003; instead,
log or record the full metadata separately and raise a short, static error
message (or a custom exception) like "Missing required 'name' in service
metadata" from _build_service_frontmatter, referencing the metadata variable for
the logger/recorder so the long content is not interpolated into the exception
message.
🧹 Nitpick comments (2)
docs/web/export-services.py (1)
35-57: Ensure INFO logs aren’t silently dropped when not verbose.A handler is only attached in verbose mode, so INFO logs can be lost (and repeated calls can add duplicate handlers). Consider attaching a single handler regardless of verbosity and just adjust its level.
♻️ Suggested tweak
def configure_logger(verbose: bool) -> logging.Logger: """Configure and return a logger with appropriate verbosity level. @@ - logger = logging.getLogger(__name__) - - if not verbose: - logger.setLevel(logging.INFO) - return logger - - handler = logging.StreamHandler() - handler.setLevel(logging.DEBUG) - formatter = logging.Formatter(" %(levelname)s: %(message)s") - handler.setFormatter(formatter) - logger.addHandler(handler) - logger.setLevel(logging.DEBUG) + logger = logging.getLogger(__name__) + if not logger.handlers: + handler = logging.StreamHandler() + handler.setLevel(logging.DEBUG if verbose else logging.INFO) + formatter = logging.Formatter(" %(levelname)s: %(message)s") + handler.setFormatter(formatter) + logger.addHandler(handler) + logger.setLevel(logging.DEBUG if verbose else logging.INFO) return loggerdocs/web/update-docs.py (1)
44-63: Avoid silent INFO logs and duplicate handlers.A handler is only attached in verbose mode; INFO logs may disappear otherwise, and repeated initializations can stack handlers. Consider attaching a single handler and just toggling the level.
♻️ Suggested tweak
def _setup_logging(self, verbose: bool) -> logging.Logger: @@ - logger = logging.getLogger(__name__) - logger.setLevel(logging.DEBUG if verbose else logging.INFO) - - if verbose: - handler = logging.StreamHandler() - handler.setLevel(logging.DEBUG) - formatter = logging.Formatter(" %(levelname)s: %(message)s") - handler.setFormatter(formatter) - logger.addHandler(handler) + logger = logging.getLogger(__name__) + if not logger.handlers: + handler = logging.StreamHandler() + handler.setLevel(logging.DEBUG if verbose else logging.INFO) + formatter = logging.Formatter(" %(levelname)s: %(message)s") + handler.setFormatter(formatter) + logger.addHandler(handler) + logger.setLevel(logging.DEBUG if verbose else logging.INFO) return logger
3e0c452 to
5e5b200
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker/security/traefik/traefik.yaml (1)
64-64: COPY path inconsistent with the new config directory structure.The volume mount was changed from
./traefik/to./config/(line 29), but theCOPYcommand in the Dockerfile still referencestraefik/logrotate.conf. This will cause the build to fail when the file isn't found at the old path.🐛 Proposed fix
- COPY traefik/logrotate.conf /etc/logrotate.conf + COPY config/logrotate.conf /etc/logrotate.conf
🤖 Fix all issues with AI agents
In `@docker/ai/autogenstudio/autogenstudio.yaml`:
- Line 14: The compose entry now uses build: . which assumes a Dockerfile is
present in the build context root; verify the Dockerfile location and either add
a dockerfile: <relative-path-to-Dockerfile> to the same service stanza (using
the Dockerfile's relative path) or change the build context to the subdirectory
that contains the Dockerfile so Docker can find it (confirm by searching for
files named Dockerfile and updating the build: or dockerfile: fields
accordingly).
In `@docs/web/compose_processor.py`:
- Around line 132-155: _parse_compose_lines currently only recognizes the '---'
delimiter and returns empty yaml_lines if it's missing; update it to fallback
when '---' is absent: scan from the top collecting leading comment lines (lines
starting with "#" or "# ") into head_lines until the first non-comment line,
then treat the remainder of the file as yaml_lines (so yaml_lines =
remaining_lines). Modify the logic in _parse_compose_lines to set yaml_started
when the delimiter is seen OR when the first non-comment line is encountered
(using existing variables head_lines, yaml_lines, yaml_started) so files without
'---' still produce proper yaml_lines.
In `@docs/web/export-services.py`:
- Around line 35-57: configure_logger currently only attaches a StreamHandler in
verbose mode so INFO logs never appear when verbose=False; change it to always
ensure a handler is attached (e.g., create a StreamHandler, set level to INFO
for non-verbose and DEBUG for verbose, apply the same Formatter, add it to
logger) while avoiding duplicate handlers by checking logger.handlers before
adding; keep using logger.setLevel(logging.DEBUG) for verbose and logging.INFO
for non-verbose and ensure the handler level matches the chosen verbosity
(references: function configure_logger, variables logger, handler, formatter).
In `@docs/web/git_utils.py`:
- Around line 7-39: The long error messages must be removed from raise sites and
placed into the exception classes: update GitExecutableNotFoundError and
NotInGitRepositoryError to set their default message (e.g., implement
__init__(self, msg: str = "Git executable not found...") ->
super().__init__(msg)) so callers can just raise GitExecutableNotFoundError or
raise NotInGitRepositoryError (and use "raise GitExecutableNotFoundError() from
exc" in except blocks where chaining is needed). Change all current raise sites
in get_git_root (the git_cmd is None case and both except blocks) to raise the
exceptions without inline strings (use the exception constructors or bare raise
with chaining as appropriate).
In `@docs/web/update-docs.py`:
- Around line 65-84: The _load_config method assumes yaml.safe_load returns a
mapping but safe_load can return None or a non-mapping; update _load_config to
validate the parsed value from yaml.safe_load (called `data`) before using
data.get: after loading, check that `data` is a dict/mapping and that
`data.get("locations")` is a list of (source, target, weight) tuples (or at
least a list), otherwise log a clear error with self.logger.error/exception and
sys.exit(1) or return an empty list; reference the existing function name
`_load_config`, the variable `config_path`, and the `yaml.safe_load` call to
locate where to add these checks and the fallback behavior.
♻️ Duplicate comments (1)
docs/web/update-docs.py (1)
239-271: Ruff TRY003: long exception message with metadata.Ruff flags long interpolated exception messages; log the metadata separately and raise a short message instead.
As per coding guidelines, this should pass Ruff linting.🐛 Proposed fix
- if "name" not in metadata: - raise ValueError(f"Metadata missing required 'name' key: {metadata}") + if "name" not in metadata: + self.logger.error("Metadata missing required 'name' key: %s", metadata) + raise ValueError("Metadata missing required 'name' key")
🧹 Nitpick comments (8)
scripts/restructure-services.py (6)
96-97: Specify explicit file encoding.Opening files without an explicit encoding uses the system default, which can vary across platforms and cause issues with non-ASCII characters.
Proposed fix
- with open(path) as f: + with open(path, encoding="utf-8") as f: data = yaml.safe_load(f)
115-116: Narrow the exception type for invalid service entries.Ruff flags this as BLE001 (blind exception). Since you're parsing dictionary data, catching
KeyErrororTypeErrorwould be more precise and avoid masking unexpected errors.As per coding guidelines, Python code must pass Ruff linting.
Proposed fix
- except Exception as e: + except (KeyError, TypeError) as e: logger.warning(f"Skipping invalid service entry: {e}")
262-267: Specify encoding and narrow exception type.Same issues as in
load_service_list: missing explicit encoding and blind exception catch (BLE001).Proposed fix
try: - with open(paths.compose_file) as f: + with open(paths.compose_file, encoding="utf-8") as f: content = f.read() _show_transform_preview(content, service.service_name, messages) - except Exception as e: + except OSError as e: messages.append(f"⚠ Could not read compose file for transform preview: {e}")
303-312: Specify explicit file encoding for compose file operations.File read and write operations should use explicit encoding for consistency and portability.
Proposed fix
# Read and transform compose file - with open(paths.compose_file) as f: + with open(paths.compose_file, encoding="utf-8") as f: content = f.read() transformed_content = transform_volume_mounts(content, service.service_name) # Write transformed compose file to stack directory new_compose_file = temp_stack / f"{service.service_name}.yaml" - with open(new_compose_file, "w") as f: + with open(new_compose_file, "w", encoding="utf-8") as f: f.write(transformed_content)
387-398: Consider reordering operations for safer rollback.If the rename at line 397 fails after
compose_file.unlink()(line 388) andshutil.rmtree(config_dir)(line 392), the original files are lost but the new structure isn't in place. Consider deleting old files only after the rename succeeds.Proposed safer ordering
try: _perform_restructure(service, paths, messages) - # Delete old compose file - compose_file.unlink() - - # Delete old config directory if it exists - if config_exists: - shutil.rmtree(config_dir) - # Rename stack directory to final name if final_dir.exists(): shutil.rmtree(final_dir) stack_dir.rename(final_dir) messages.append(f"✓ Renamed: {stack_dir.relative_to(docker_dir)}/ → {final_dir.relative_to(docker_dir)}/") + # Delete old compose file after successful rename + compose_file.unlink() + + # Delete old config directory if it exists + if config_exists: + shutil.rmtree(config_dir) + return True, "\n ".join(messages)
378-378: Replace ambiguous Unicode character.Ruff flags
ℹ(INFORMATION SOURCE) as ambiguous (RUF001). Use ASCII equivalent for consistency.As per coding guidelines, Python code must pass Ruff linting.
Proposed fix
- messages.append("ℹ No config directory (OK)") + messages.append("i No config directory (OK)")docker/monitoring/prometheus/prometheus.yaml (1)
20-20: Consider running Prometheus as a non-root user.Running as
user: "0:0"(root) works but is less secure. If volume permissions allow, consider using a dedicated UID/GID (e.g.,65534:65534for nobody) to follow least-privilege principles.docs/web/update-docs.py (1)
44-63: Consider attaching a handler even when verbose is off.INFO logs are used later, but only verbose mode installs a handler. If default progress logs are expected, add a handler for the non-verbose path too.
💡 Suggested tweak
- logger.setLevel(logging.DEBUG if verbose else logging.INFO) - - if verbose: - handler = logging.StreamHandler() - handler.setLevel(logging.DEBUG) - formatter = logging.Formatter(" %(levelname)s: %(message)s") - handler.setFormatter(formatter) - logger.addHandler(handler) - - return logger + handler = logging.StreamHandler() + handler.setLevel(logging.DEBUG if verbose else logging.INFO) + formatter = logging.Formatter(" %(levelname)s: %(message)s") + handler.setFormatter(formatter) + + if not logger.handlers: + logger.addHandler(handler) + + logger.setLevel(logging.DEBUG if verbose else logging.INFO) + return logger
| def configure_logger(verbose: bool) -> logging.Logger: | ||
| """Configure and return a logger with appropriate verbosity level. | ||
|
|
||
| Args: | ||
| verbose: Enable debug level logging | ||
|
|
||
| Returns: | ||
| Configured logger instance | ||
| """ | ||
| logger = logging.getLogger(__name__) | ||
|
|
||
| if not verbose: | ||
| logger.setLevel(logging.INFO) | ||
| return logger | ||
|
|
||
| handler = logging.StreamHandler() | ||
| handler.setLevel(logging.DEBUG) | ||
| formatter = logging.Formatter(" %(levelname)s: %(message)s") | ||
| handler.setFormatter(formatter) | ||
| logger.addHandler(handler) | ||
| logger.setLevel(logging.DEBUG) | ||
|
|
||
| return logger |
There was a problem hiding this comment.
Non-verbose mode currently won’t emit INFO logs.
Only verbose mode attaches a handler, so the INFO logs below won’t show unless the root logger is configured elsewhere. Consider attaching a handler (or using basicConfig) for the default path too.
💡 Suggested fix
- if not verbose:
- logger.setLevel(logging.INFO)
- return logger
-
- handler = logging.StreamHandler()
- handler.setLevel(logging.DEBUG)
- formatter = logging.Formatter(" %(levelname)s: %(message)s")
- handler.setFormatter(formatter)
- logger.addHandler(handler)
- logger.setLevel(logging.DEBUG)
-
- return logger
+ handler = logging.StreamHandler()
+ handler.setLevel(logging.DEBUG if verbose else logging.INFO)
+ formatter = logging.Formatter(" %(levelname)s: %(message)s")
+ handler.setFormatter(formatter)
+
+ if not logger.handlers:
+ logger.addHandler(handler)
+
+ logger.setLevel(logging.DEBUG if verbose else logging.INFO)
+ return logger📝 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 configure_logger(verbose: bool) -> logging.Logger: | |
| """Configure and return a logger with appropriate verbosity level. | |
| Args: | |
| verbose: Enable debug level logging | |
| Returns: | |
| Configured logger instance | |
| """ | |
| logger = logging.getLogger(__name__) | |
| if not verbose: | |
| logger.setLevel(logging.INFO) | |
| return logger | |
| handler = logging.StreamHandler() | |
| handler.setLevel(logging.DEBUG) | |
| formatter = logging.Formatter(" %(levelname)s: %(message)s") | |
| handler.setFormatter(formatter) | |
| logger.addHandler(handler) | |
| logger.setLevel(logging.DEBUG) | |
| return logger | |
| def configure_logger(verbose: bool) -> logging.Logger: | |
| """Configure and return a logger with appropriate verbosity level. | |
| Args: | |
| verbose: Enable debug level logging | |
| Returns: | |
| Configured logger instance | |
| """ | |
| logger = logging.getLogger(__name__) | |
| handler = logging.StreamHandler() | |
| handler.setLevel(logging.DEBUG if verbose else logging.INFO) | |
| formatter = logging.Formatter(" %(levelname)s: %(message)s") | |
| handler.setFormatter(formatter) | |
| if not logger.handlers: | |
| logger.addHandler(handler) | |
| logger.setLevel(logging.DEBUG if verbose else logging.INFO) | |
| return logger |
🤖 Prompt for AI Agents
In `@docs/web/export-services.py` around lines 35 - 57, configure_logger currently
only attaches a StreamHandler in verbose mode so INFO logs never appear when
verbose=False; change it to always ensure a handler is attached (e.g., create a
StreamHandler, set level to INFO for non-verbose and DEBUG for verbose, apply
the same Formatter, add it to logger) while avoiding duplicate handlers by
checking logger.handlers before adding; keep using
logger.setLevel(logging.DEBUG) for verbose and logging.INFO for non-verbose and
ensure the handler level matches the chosen verbosity (references: function
configure_logger, variables logger, handler, formatter).
Summary by CodeRabbit
Chores
Monitoring
Documentation
New Features
✏️ Tip: You can customize this high-level summary in your review settings.