feat(build): Push toolkit bundle as OCI zstd artifact#112
Conversation
📝 WalkthroughWalkthroughThe action now supports Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant cache-downloader.py
participant regctl
participant zstd
participant tar
Workflow->>cache-downloader.py: TOOLKIT_DOWNLOAD_METHOD=oci-zstd
cache-downloader.py->>regctl: artifact get toolkit bundle ref
regctl->>zstd: compressed payload
zstd->>tar: decompressed archive
tar-->>cache-downloader.py: extracted toolkit files
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Comment |
|
📦 Python package built successfully!
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/actions/push-oci-artifact/action.yml (1)
131-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated annotation-building and file-parsing logic across both publish steps.
Lines 144-154 and 166-187 largely re-implement the
manifest_annotationsconstruction and file-descriptor parsing already present in the image-volume step (lines 64-87). Any future fix (e.g., descriptor edge cases) now needs to be applied in two places.♻️ Suggested direction
Extract the shared annotation-building loop (and possibly descriptor parsing) into a small reusable script sourced by both steps, or a separate composite step that writes annotations/args to a temp file consumed by both branches.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/push-oci-artifact/action.yml around lines 131 - 196, The direct-files branch in the push step is duplicating the same annotation assembly and file-descriptor parsing already implemented in the image-volume branch. Refactor the shared logic out of the inline bash in the Push OCI direct file artifact step into a reusable helper/script or composite step, and have both publish_mode branches consume it so fixes to descriptor handling or annotation rules only need to be made once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/actions/push-oci-artifact/action.yml:
- Around line 131-196: The direct-files branch in the push step is duplicating
the same annotation assembly and file-descriptor parsing already implemented in
the image-volume branch. Refactor the shared logic out of the inline bash in the
Push OCI direct file artifact step into a reusable helper/script or composite
step, and have both publish_mode branches consume it so fixes to descriptor
handling or annotation rules only need to be made once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24f43dc9-a809-4d3c-88e8-c9e959c869ba
📒 Files selected for processing (2)
.github/actions/push-oci-artifact/action.yml.github/workflows/cd.yml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #112 +/- ##
=======================================
Coverage 74.42% 74.42%
=======================================
Files 95 95
Lines 5704 5704
Branches 850 850
=======================================
Hits 4245 4245
Misses 1182 1182
Partials 277 277
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
🚀 Review App Deployment Started
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/unit/test_cache_downloader.py (1)
177-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test method annotations/docstring.
Use
-> Noneon the new tests, and add a short docstring fortest_build_toolkit_bundle_ref.As per coding guidelines,
**/*.py: “Use explicit type hints for function parameters and return values” and “Use docstrings for all functions/classes”.Also applies to: 243-244
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_cache_downloader.py` around lines 177 - 183, The new test methods in test_cache_downloader need explicit return type hints and a short docstring to match the Python coding guidelines. Update test_build_toolkit_bundle_ref and the other affected test(s) in this file (including the ones around test_build_toolkit_bundle_ref and the later occurrences noted in the review) to use -> None, and add a concise docstring describing each test’s purpose. Keep the changes localized to the test method definitions so they remain easy to find by their test_* names.Source: Coding guidelines
dockerfiles/cache-downloader/Dockerfile (1)
5-5: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the regctl source image by digest.
v0.11.5is a mutable tag. Use a digest-pinned reference for reproducible, safer builds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dockerfiles/cache-downloader/Dockerfile` at line 5, The regctl source image reference in the Dockerfile is still tag-based and mutable, so update the COPY --from=... source to use a digest-pinned regclient/regctl image instead of v0.11.5. Keep the existing regctl copy step, but replace the source image reference with a fixed digest to make the build reproducible and safer.dockerfiles/cache-downloader/cache-downloader.py (1)
17-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the new APIs with project Python style.
New/changed functions need docstrings and explicit
-> Nonewhere applicable. Line 132 also keeps path handling split acrossos.path/os.makedirs; preferPathconsistently.As per coding guidelines,
**/*.py: “Use type hints consistently”, “Use docstrings for all functions/classes”, and “Use pathlib.Path for file path operations instead of os.path”.Also applies to: 69-74, 123-141, 166-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dockerfiles/cache-downloader/cache-downloader.py` around lines 17 - 36, The new/updated functions in cache-downloader.py need to follow project Python style by adding docstrings to each function, making return types explicit with -> None where the function does not return a value, and keeping type hints consistent across the module. Update the relevant helpers such as build_toolkit_bundle_ref, _read_stderr, and download_dependency_from_s3 accordingly, and replace mixed os.path/os.makedirs path handling with pathlib.Path throughout the path-related code, including the sections around version path and directory creation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dockerfiles/cache-downloader/cache-downloader.py`:
- Around line 257-269: The cache-downloader flow currently defers validation
until after cleanup, so an invalid TOOLKIT_DOWNLOAD_METHOD can trigger
cleanup_old_versions() before failing, and submit_downloading() can still be
called with a missing toolkit_index_bucket_name in S3 mode. Validate the backend
configuration first in the cache-downloader entry path before calling
cleanup_old_versions() or submit_downloading(), using the existing
download_method, oci_repository, and toolkit_index_bucket_name values to reject
unsupported methods and missing S3 bucket configuration early.
- Around line 52-57: The pipeline in cache-downloader.py is leaving stderr pipes
unread while aws/regctl/zstd and tar run, which can block noisy subprocesses and
hang the download flow. Update the subprocess handling in the download path and
the OCI path so stderr is drained concurrently or redirected away from PIPE
during execution, then preserve the collected stderr for error reporting in the
existing failure branches around the aws_process/tar_process logic and the OCI
equivalents.
---
Nitpick comments:
In `@dockerfiles/cache-downloader/cache-downloader.py`:
- Around line 17-36: The new/updated functions in cache-downloader.py need to
follow project Python style by adding docstrings to each function, making return
types explicit with -> None where the function does not return a value, and
keeping type hints consistent across the module. Update the relevant helpers
such as build_toolkit_bundle_ref, _read_stderr, and download_dependency_from_s3
accordingly, and replace mixed os.path/os.makedirs path handling with
pathlib.Path throughout the path-related code, including the sections around
version path and directory creation.
In `@dockerfiles/cache-downloader/Dockerfile`:
- Line 5: The regctl source image reference in the Dockerfile is still tag-based
and mutable, so update the COPY --from=... source to use a digest-pinned
regclient/regctl image instead of v0.11.5. Keep the existing regctl copy step,
but replace the source image reference with a fixed digest to make the build
reproducible and safer.
In `@tests/unit/test_cache_downloader.py`:
- Around line 177-183: The new test methods in test_cache_downloader need
explicit return type hints and a short docstring to match the Python coding
guidelines. Update test_build_toolkit_bundle_ref and the other affected test(s)
in this file (including the ones around test_build_toolkit_bundle_ref and the
later occurrences noted in the review) to use -> None, and add a concise
docstring describing each test’s purpose. Keep the changes localized to the test
method definitions so they remain easy to find by their test_* names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9135739c-e84b-42c0-bfb4-4a3c5e3e3572
📒 Files selected for processing (3)
dockerfiles/cache-downloader/Dockerfiledockerfiles/cache-downloader/cache-downloader.pytests/unit/test_cache_downloader.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dockerfiles/cache-downloader/cache-downloader.py (1)
77-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a specific exception type. Bare
Exception(here and in the OCI path at Lines 135-146) hampers targeted handling.Proposed fix
- raise Exception( + raise RuntimeError( f"Error downloading {release_name} (aws s3 command failed): aws stderr: {aws_process_stderr_bytes}, tar stderr: {tar_process_stderr_bytes}" )As per coding guidelines: "Use appropriate exception types for error handling".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dockerfiles/cache-downloader/cache-downloader.py` around lines 77 - 84, Replace the broad Exception raises in the cache downloader error paths with a more specific exception type so callers can handle download failures distinctly. Update the AWS download branch and the OCI path error handling in the same downloader flow to throw a targeted runtime/error class from the existing download logic instead of bare Exception, keeping the same failure messages and using the relevant error-handling blocks in the cache downloader routine.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dockerfiles/cache-downloader/cache-downloader.py`:
- Around line 26-30: Update validate_download_config to use Optional[str] for
the nullable parameters instead of str | None, since this module must remain
importable on Python 3.9 and should follow the existing Optional[T] typing rule.
Adjust the annotations on toolkit_index_bucket_name and oci_repository in
validate_download_config accordingly, and keep the rest of the function behavior
unchanged.
---
Nitpick comments:
In `@dockerfiles/cache-downloader/cache-downloader.py`:
- Around line 77-84: Replace the broad Exception raises in the cache downloader
error paths with a more specific exception type so callers can handle download
failures distinctly. Update the AWS download branch and the OCI path error
handling in the same downloader flow to throw a targeted runtime/error class
from the existing download logic instead of bare Exception, keeping the same
failure messages and using the relevant error-handling blocks in the cache
downloader routine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83b01aab-f3ab-4623-8c89-46fb0d03ac51
📒 Files selected for processing (2)
dockerfiles/cache-downloader/cache-downloader.pytests/unit/test_cache_downloader.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/test_cache_downloader.py
2 participants
Summary by CodeRabbit
image-volume(default) anddirect-files.tar+zstdOCI artifact, and enable toolkit downloads via OCI zstd in addition to S3.direct-files: enforcespath:media-typeformat, requires relative existing files, prevents duplicate basenames, errors when no files are provided, and verifies the pushed manifest.