feat: implement software reporting for containers #2
Conversation
📝 WalkthroughWalkthroughThe changes add functionality for handling container images within the deployment plugin. A private method in the container environment class now parses an image URI and appends a default tag if necessary. The shell command decoration routine is updated to incorporate this information, and the software reporting method now returns a report containing the image URI and tag. In addition, separate manager classes for UDocker and Podman containers are introduced to perform image inspection via subprocess calls. A new test method is added to verify the correctness of the software report’s details. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as ContainerEnv
participant Spec as Image Spec
participant Shell as Shell Command
participant Mgr as {PodmanManager / UDockerManager}
participant Report as SoftwareReport
Env->>Spec: _get_image_uri_and_tag()
Spec-->>Env: Return image URI and tag
Env->>Shell: decorate_shellcmd(cmd) using image info
Shell-->>Env: Executes decorated command
Env->>Mgr: Determine container type and call inspect_image(image_id)
Mgr-->>Env: Return full image ID (if available)
Env->>Report: Construct SoftwareReport with image URI, tag, & image ID
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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:
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 (
|
Uses image tag (provided) and image hash (after inspection of the local registry).
btraven00
force-pushed
the
feat/software-report
branch
from
206d83a to
d3f1664
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/test_plugin.py (2)
57-74: Consider verifying the return code ofsp.run.Currently, the call to
sp.rundoes not check for errors or non-zero exit codes, which may mask potential issues with fetching the container image or running the command. Addingcheck=Trueor verifyingresult.returncodehelps ensure that failures are captured.-sp.run(decorated_cmd, shell=True, executable=self.shell_executable) +result = sp.run(decorated_cmd, shell=True, executable=self.shell_executable) +assert result.returncode == 0, f"Container run failed with exit code {result.returncode}"
110-126: Apply the same validation in Podman tests.Similar to the UDocker container test, verifying the exit code here helps detect failures during container fetching or command execution.
-sp.run(decorated_cmd, shell=True, executable=self.shell_executable) +result = sp.run(decorated_cmd, shell=True, executable=self.shell_executable) +assert result.returncode == 0, f"Container run failed with exit code {result.returncode}"src/snakemake_software_deployment_plugin_container/__init__.py (1)
198-199: Streamline conditional checks.The nested condition for
rootfsanddiff_idscan be simplified to a single check for improved readability, as suggested by static analysis.-if "rootfs" in inspect_data and "diff_ids" in inspect_data["rootfs"]: - if len(inspect_data["rootfs"]["diff_ids"]) > 0: +if ("rootfs" in inspect_data + and "diff_ids" in inspect_data["rootfs"] + and len(inspect_data["rootfs"]["diff_ids"]) > 0):🧰 Tools
🪛 Ruff (0.8.2)
198-199: Use a single
ifstatement instead of nestedifstatements(SIM102)
🛑 Comments failed to post (2)
src/snakemake_software_deployment_plugin_container/__init__.py (2)
85-92:
⚠️ Potential issueEnsure support for custom registries containing multiple colons.
At present,
_get_image_uri_and_tagraises errors for URIs that contain a port or additional segments (e.g.,registry.company.org:5000/myimage:tag). This limitation restricts valid URIs to strictly one or two colon-delimited parts. Consider a more robust parsing strategy that handles custom registry formats:-def _get_image_uri_and_tag(self) -> Iterable[str]: - parts = self.spec.image_uri.split(":") - if len(parts) > 2: - raise WorkflowError("Malformed image URI", self.spec.image_uri) - if len(parts) != 2: - parts += ["latest"] - return parts +def _get_image_uri_and_tag(self) -> Iterable[str]: + # Example: handle optional port and tag by splitting on the rightmost colon + if ":" not in self.spec.image_uri: + return [self.spec.image_uri, "latest"] + repo_tag_split = self.spec.image_uri.rsplit(":", 1) + if len(repo_tag_split) == 1: + # no tag, default to 'latest' + return [repo_tag_split[0], "latest"] + return repo_tag_splitAlso applies to: 118-118
159-179:
⚠️ Potential issueUse both the repository name and tag when inspecting images.
The code currently inspects images using only the repository name (e.g.,
rep[0]). If an explicit tag is specified (e.g.,alpine:3.17), inspection might fail without including it. Consider revising the call topm.inspect_imageto include both parts:-uri, tag = self._get_image_uri_and_tag() ... -full_image_id = pm.inspect_image(uri) +repo, tag = self._get_image_uri_and_tag() +full_image_id = pm.inspect_image(f"{repo}:{tag}")Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/snakemake_software_deployment_plugin_container/__init__.py (2)
180-215: Well-structured UDockerManager class with comprehensive error handling.The class properly handles image inspection and extraction of the hash identifier. The error handling covers multiple potential failure points.
Consider these improvements:
- Combine the nested if statements on lines 197-199 into a single condition:
- if "rootfs" in inspect_data and "diff_ids" in inspect_data["rootfs"]: - if len(inspect_data["rootfs"]["diff_ids"]) > 0: + if "rootfs" in inspect_data and "diff_ids" in inspect_data["rootfs"] and len(inspect_data["rootfs"]["diff_ids"]) > 0:
- Consider using a logger instead of print statements for better error handling:
- print(f"error: failed to extract hash for udocker image {image_id}: {e}") + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Failed to extract hash for udocker image {image_id}: {e}")🧰 Tools
🪛 Ruff (0.8.2)
197-198: Use a single
ifstatement instead of nestedifstatements(SIM102)
217-237: Well-implemented PodmanManager class for container inspection.The class properly extracts the image ID for Podman containers and includes comprehensive error handling for different failure scenarios.
Consider using a logger instead of print statements for more consistent error handling:
- print(f"error: failed to inspect image {image_id}: {e}") + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Failed to inspect image {image_id}: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/snakemake_software_deployment_plugin_container/__init__.py(6 hunks)tests/test_plugin.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest ...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake_software_deployment_plugin_container/__init__.pytests/test_plugin.py
🪛 Ruff (0.8.2)
src/snakemake_software_deployment_plugin_container/__init__.py
197-198: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (8)
tests/test_plugin.py (2)
57-73: Well-designed test for UDocker container software reporting.The test effectively verifies the container's software reporting functionality by:
- Getting the environment and decorating a test command
- Running the command to ensure the image is fetched
- Validating that the reported software has the expected name and version format
The assertions correctly check that the container name is "alpine" and that the version follows the expected format of "tag/hash".
110-126: Well-implemented test for Podman container software reporting.This test mirrors the UDocker test with appropriate comments explaining the purpose and expected format of the software report. The comments clarify that the version follows the format "tag/hash" (e.g., "latest/aded1e1a5b37").
Both tests ensure consistency in software reporting across different container backends.
src/snakemake_software_deployment_plugin_container/__init__.py (6)
5-7: Appropriate additions to imports.The added imports (
jsonand fullsubprocess) are necessary for the new functionality to parse container inspection results.
34-35: Good extraction of SOURCE_CACHE constant.Defining this path as a constant improves maintainability by centralizing the definition of the source cache location.
84-90: Well-implemented image URI parsing with proper error handling.The method correctly handles different URI formats and adds a default "latest" tag when none is specified. The error handling for malformed URIs is appropriate.
117-117: Improved shell command decoration using the new URI parsing method.The code now uses the helper method to properly handle image URIs and tags.
119-120: Refactored cache path handling using the new constant.This change improves consistency by using the centralized SOURCE_CACHE constant.
157-178: Well-implemented software reporting with container inspection.The method properly:
- Extracts the image URI and tag
- Creates a SoftwareReport with container details
- Uses the appropriate container manager to inspect the image
- Enhances the version information with the image hash
The TODOs indicate potential future improvements for encapsulating backend-specific logic.
We use a combination of container URI, tag and hash of the image used in the execution
Summary by CodeRabbit
New Features
Tests