Update website generator and container image tag helper scripts - #236
Conversation
WalkthroughReplaces several relative imports with absolute imports in docs/web, switches invocation of the update-docs script to a standalone script path, enhances export-services to support stdout/file output, adds a Taskfile task, and implements version- and date-aware sorting with a new --sort option for container tag retrieval. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/infra-mcp/tools/get_container_tags.py (1)
216-284: Same unusedlimitparameter issue as inget_docker_hub_tags.This method has the same unused
limitparameter issue. For consistency, apply the same solution chosen forget_docker_hub_tags(see previous comment at lines 144-214).
🧹 Nitpick comments (7)
scripts/infra-mcp/tools/get_container_tags.py (2)
88-122: Consider consistent error handling for datetime parsing.Line 114 parses
last_updatedwithout a try-except block, while lines 119-121 wrap similar parsing logic in error handling. Although the conditional check provides some protection, an unexpected datetime format could still cause an exception.Apply this diff to add consistent error handling:
# Priority 2: Version tags version = self._parse_version(tag['name']) if version: # Pad version tuple to 4 elements for consistent comparison padded_version = version + (0,) * (4 - len(version)) - updated = datetime.fromisoformat(tag['last_updated'].replace('Z', '+00:00')) if tag.get('last_updated') else datetime.min + try: + updated = datetime.fromisoformat(tag['last_updated'].replace('Z', '+00:00')) if tag.get('last_updated') else datetime.min + except (ValueError, AttributeError): + updated = datetime.min return (1, padded_version[:4], updated)
144-214: Address unusedlimitparameter flagged by static analysis.The
limitparameter is documented as "unused in fetching, used by caller" but triggers a linter warning. Consider either:
- Removing the parameter and letting callers handle limiting (cleaner API)
- Applying the limit after sorting (lines 208-209)
- Prefixing with underscore
_limitto indicate intentionally unusedThe current approach of fetching all tags and sorting before limiting is reasonable, but having an unused parameter in the signature may confuse callers.
Option 1: Remove the parameter
- def get_docker_hub_tags(self, image_name: str, limit: int = 10, architecture: str = "linux/amd64", sort_by: str = "version") -> list[dict[str, Any]]: + def get_docker_hub_tags(self, image_name: str, architecture: str = "linux/amd64", sort_by: str = "version") -> list[dict[str, Any]]: """Query Docker Hub for image tags with timestamp information. Args: image_name: Name of the image to query - limit: Maximum number of tags to return (unused in fetching, used by caller) architecture: Architecture to filter by (e.g., 'linux/amd64') sort_by: Sort method - 'version' (default), 'updated', or 'default' (Docker Hub order)Option 2: Apply limit after sorting
# Sort based on sort_by parameter self._sort_tags(tag_data, sort_by) + # Apply limit if specified + if limit: + tag_data = tag_data[:limit]docs/web/.gitignore (1)
4-4: Minor wording nit in generator commentConsider changing “based the content” to “based on the content” for clarity:
-# Generated by update-docs.py, based the content in <repo-root>/docker/ and additional locations +# Generated by update-docs.py, based on the content in <repo-root>/docker/ and additional locationsdocs/web/docker_scanner.py (1)
7-7: Local import change may affect package-style usageSwitching to
from compose_processor import ComposeFileProcessorworks whendocs/webis onsys.pathas a plain directory (e.g., runningupdate-docs.pydirectly), but will fail if this module is imported as part of a package, e.g.from docs.web.docker_scanner import DockerComposeScanner, unlesscompose_processoris also available as a top‑level module.If you still support
docs.webas an importable package anywhere (tests, tools, or other code), consider either:
- using a package-qualified import (e.g.,
from docs.web.compose_processor import ComposeFileProcessor), and running viapython -m; or- confirming that these modules are only ever used via the standalone scripts and not imported as a package.
docs/web/update-docs.py (1)
11-13: Direct local imports tie this script to “run as file” usageUsing bare imports (
from docker_scanner import ..., etc.) is compatible with invokingupdate-docs.pydirectly (as in the Dockerfile), since the script directory is onsys.path. However, this will break ifDocsProcessoris ever imported from a package context (e.g.,from docs.web.update_docs import DocsProcessor), because these modules won’t resolve as top‑level packages there.If you still need package-style usage anywhere, consider:
- reverting to package-relative imports and running via
python -m, or- fully qualifying the imports with the package name and ensuring
docs/webis a proper package.Otherwise, if the contract is “script only”, this change is consistent—just worth confirming that nothing else relies on importing these modules.
docs/web/Dockerfile (1)
11-11: Consider invoking the generator viapythonfor robustnessCalling
/repo/docs/web/update-docs.pydirectly relies on the script being executable and the shebang staying correct. To avoid depending on file mode and still keep behavior the same, you could let the image’s Python run it explicitly:-RUN --mount=type=bind,ro,source=.,target=/repo /repo/docs/web/update-docs.py --repository-path /repo --output-content-path /src/content +RUN --mount=type=bind,ro,source=.,target=/repo python /repo/docs/web/update-docs.py --repository-path /repo --output-content-path /src/contentNot strictly required, but often a bit more resilient across environments.
docs/web/export-services.py (1)
25-33: Stdout/file export behavior looks good; a couple of small polish opportunitiesThe new “file or stdout” behavior and CLI wiring look solid. Two small tweaks to consider:
- Make the
output_filecheck explicit to avoid edge cases and improve readability:- # Write to YAML file or stdout - if output_file: + # Write to YAML file or stdout + if output_file is not None:
- Update the
--output-filehelp text to reflect the new default-to-stdout behavior, e.g.:- parser.add_argument( - "--output-file", - type=str, - help="Specify the output YAML file path" - ) + parser.add_argument( + "--output-file", + type=str, + help="Specify the output YAML file path (defaults to stdout if omitted)" + )These don’t change behavior but make the intent clearer to callers.
Also applies to: 71-84, 100-103, 115-116
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/web/.gitignore(1 hunks)docs/web/Dockerfile(1 hunks)docs/web/README.md(1 hunks)docs/web/__init__.py(0 hunks)docs/web/docker_scanner.py(1 hunks)docs/web/export-services.py(4 hunks)docs/web/update-docs.py(1 hunks)scripts/infra-mcp/tools/get_container_tags.py(5 hunks)
💤 Files with no reviewable changes (1)
- docs/web/init.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python files must pass Ruff linting
Files:
docs/web/export-services.pydocs/web/update-docs.pydocs/web/docker_scanner.pyscripts/infra-mcp/tools/get_container_tags.py
**/Dockerfile*
📄 CodeRabbit inference engine (AGENTS.md)
Dockerfiles must pass Hadolint linting
Files:
docs/web/Dockerfile
🧬 Code graph analysis (4)
docs/web/export-services.py (2)
docs/web/docker_scanner.py (1)
DockerComposeScanner(10-96)docs/web/export_services.py (1)
export_services(26-77)
docs/web/update-docs.py (3)
docs/web/docker_scanner.py (1)
DockerComposeScanner(10-96)docs/web/git_utils.py (1)
get_git_root(20-45)docs/web/link_processor.py (1)
LinkProcessor(8-153)
docs/web/docker_scanner.py (1)
docs/web/compose_processor.py (1)
ComposeFileProcessor(8-112)
docs/web/Dockerfile (1)
docs/web/update_docs.py (4)
process(197-207)process_docker_directory(136-195)process_directory(119-125)__init__(20-56)
🪛 Ruff (0.14.6)
scripts/infra-mcp/tools/get_container_tags.py
144-144: Unused method argument: limit
(ARG002)
216-216: Unused method argument: limit
(ARG002)
🔇 Additional comments (5)
scripts/infra-mcp/tools/get_container_tags.py (4)
38-86: LGTM! Robust version parsing with comprehensive edge case handling.The method correctly handles various tag formats (v-prefixed, suffixed, numeric-only) and includes appropriate boundary checks and error handling.
124-142: LGTM! Clean implementation of flexible sorting.The in-place sorting with support for multiple sort orders is well-structured and easy to understand.
421-433: LGTM! Proper propagation of sort parameter.The
sort_byparameter is correctly extracted from arguments with a sensible default and properly passed to both registry methods.
586-587: Breaking change in default sort behavior: users expecting registry order will now get version-sorted results.The default sort order changed from
'default'(registry order) to'version'. This is a breaking change for existing scripts or workflows that depend on this tool, though no internal usages are found in the repository. Users can preserve the old behavior by explicitly passing--sort default.Since this is a newly added/recently modified standalone CLI tool (commit "36655fe Implement tag sorting by version") with no references in the codebase, the impact is limited to external users and manual invocations. The change is well-intentioned (version sorting is more useful by default), but users should be aware of this shift in default behavior.
docs/web/README.md (1)
8-8: Build-process doc now matches the new script entrypointThe reference to
update-docs.pyhere aligns with the Dockerfile and .gitignore updates; the build description looks consistent.
36655fe to
0aa06d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/infra-mcp/tools/get_container_tags.py (1)
107-141: Architecture-aware digest extraction and tag normalization look good; minor reuse opportunityThe
_extract_arch_digest+_create_tag_data_dicthelpers correctly standardize Docker Hub tag data and pick the digest for the requested OS/arch pair, which should make downstream consumers much simpler.For consistency and to avoid drift, you could consider reusing
_parse_archsimilarly in the registry manifest loop (wherearchitecture.split('/')is duplicated) so both Docker Hub and registry paths share the same parsing semantics, but that’s optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/web/.gitignore(1 hunks)docs/web/Dockerfile(1 hunks)docs/web/README.md(1 hunks)docs/web/Taskfile.web.yaml(1 hunks)docs/web/__init__.py(0 hunks)docs/web/docker_scanner.py(1 hunks)docs/web/export-services.py(5 hunks)docs/web/update-docs.py(1 hunks)scripts/infra-mcp/tools/get_container_tags.py(6 hunks)
💤 Files with no reviewable changes (1)
- docs/web/init.py
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/web/Dockerfile
- docs/web/.gitignore
- docs/web/README.md
- docs/web/export-services.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
YAML files must pass YAML linting and validation
Files:
docs/web/Taskfile.web.yaml
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python files must pass Ruff linting
Files:
docs/web/docker_scanner.pydocs/web/update-docs.pyscripts/infra-mcp/tools/get_container_tags.py
🧬 Code graph analysis (2)
docs/web/docker_scanner.py (1)
docs/web/compose_processor.py (1)
ComposeFileProcessor(8-112)
docs/web/update-docs.py (3)
docs/web/docker_scanner.py (1)
DockerComposeScanner(10-96)docs/web/git_utils.py (1)
get_git_root(20-45)docs/web/link_processor.py (1)
LinkProcessor(8-153)
🪛 Ruff (0.14.6)
scripts/infra-mcp/tools/get_container_tags.py
190-190: Unused method argument: limit
(ARG002)
236-236: Unused method argument: limit
(ARG002)
⏰ 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). (1)
- GitHub Check: check
🔇 Additional comments (9)
docs/web/update-docs.py (1)
11-13: LGTM! Import changes align with the script standardization.The conversion to absolute imports is consistent with the broader PR changes and aligns with the script's standalone execution model (indicated by the shebang on line 1).
docs/web/Taskfile.web.yaml (1)
57-60: LGTM! New task follows the existing pattern.The new task correctly invokes the
export-services.pyscript with the appropriate output file path. The script is executable with proper shebang, and the output directory exists. YAML file passes linting validation.docs/web/docker_scanner.py (1)
7-7: Verify the import works with the new execution context.The absolute import
from compose_processor import ComposeFileProcessorassumesdocs/webis inPYTHONPATHor the script is executed from that directory. Ensure this works correctly in all execution contexts (local development, Docker container, CI/CD) by either:
- Documenting the required execution context, or
- Adjusting the import to use relative imports or explicit path configuration (e.g., using
__file__orsys.pathmanipulation)scripts/infra-mcp/tools/get_container_tags.py (6)
38-87: Version parsing helper matches documented tag formatsThe normalization and digit-only extraction here align with the documented examples (
18.1,v18.1,18.1-trixie, etc.), and the early exits keep non-version tags out of version-aware sorting without raising. This looks solid for Docker-style tags.
142-174: Version-aware sort key correctly enforces the intended priorityThe key structure (
latest> versioned tags > non-version tags) and padding of the version tuple to fixed length match the docstring and work cleanly withreverse=Truein_sort_tags, giving you descending version order while always pinninglatestto the top. No issues here.
175-189:_sort_tagsis a clear, extensible dispatcher for sort modesThe three-mode dispatcher (
version,updated,default) is straightforward, and delegating to_version_sort_keycentralizes the more complex logic nicely. Once_parse_datetimehandles HTTP dates properly, this should give predictable behavior across Docker Hub and registries.
441-454:sort_bypropagation intoget_image_tagsis good; be aware of behavior change for other subcommandsDeriving
sort_byviagetattr(args, 'sort', 'version')and passing it through toget_docker_hub_tags/get_registry_tagswires the CLI option through cleanly.Note that for
list-same-hashandget-most-specific-tag, there is no--sortflag, so these flows now implicitly use"version"sorting instead of whatever the registry’s default order was. Because_version_sort_keyprioritizeslatestand then highest versions, the default “reference tag” whenargs.tagis omitted will effectively belatestor the highest version, which is probably what you want—but it is a subtle behavior change worth double-checking against existing usage.
606-607: CLI--sortoption forlist-recentis well-scoped and consistent with backendExposing
--sortwith choices['version', 'updated', 'default']and defaulting to'version'matches the helper behavior and keeps invalid values out at parse time. This is a nice, minimal CLI surface expansion.
190-201: Thelimitparameter inget_docker_hub_tagsIS actually used and does not violate Ruff ARG002The parameter is used at line 231 with
return tag_data[:limit], which slices the result list to the specified limit. The code is already correctly implementing the suggested fix. No changes are required, and the current implementation passes Ruff linting.The docstring note "(unused in fetching, used by caller)" is misleading since
limitis actively used in the method body to trim results. Consider clarifying the docstring to say something like "Maximum number of tags to return (used to limit results)".Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.