feat: update wrappers to their specific latest version instead of the global latest version of the wrapper repo #105
Conversation
|
Warning Rate limit exceeded@johanneskoester has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as snakedeploy.client
participant Updater as update_snakemake_wrappers
participant Repo as WrapperRepo
participant Git as snakemake-wrappers (git)
User->>CLI: run update-snakemake-wrappers [snakefiles]
CLI->>Updater: update_snakemake_wrappers(snakefiles)
rect rgba(220,235,245,0.5)
Updater->>Repo: __enter__() — clone + sparse-checkout init
Repo->>Git: fetch sparse patterns & refs
Git-->>Repo: repo ready
end
loop per snakefile
Updater->>Updater: parse wrapper specs
alt spec is non-URL / unpinned
Updater->>Repo: get_wrapper_version(spec)
Repo->>Git: query tags for wrapper path
Git-->>Repo: latest tag or none
Repo-->>Updater: version | None
alt version found
Updater->>Updater: replace spec with tag and write file
else
Updater->>Updater: log warning (version unknown)
end
else external URL or already pinned
Updater->>Updater: no change
end
end
rect rgba(235,245,220,0.5)
Updater->>Repo: __exit__() — cleanup temp repo
end
Updater-->>CLI: return/update results
CLI-->>User: output logs/results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
snakedeploy/snakemake_wrappers.py (1)
12-43: Consider clarifying the function name and docstring.The function returns the earliest tag that contains the latest changes to the directory, not the "latest git tag". While the logic is correct for finding the appropriate wrapper version (the first release that included the wrapper's current state), the naming could be clearer.
Consider renaming to
get_first_git_tag_containing_pathor updating the docstring to clarify: "Get the earliest git tag containing the latest commit that changed the given directory."Alternatively, if you prefer to keep the current name, update the docstring:
-def get_latest_git_tag(path: Path, repo: Path) -> str | None: - """Get the latest git tag of any file in the given directory or below. - Thereby ignore later git tags outside of the given directory. +def get_latest_git_tag(path: Path, repo: Path) -> str | None: + """Get the earliest git tag containing the latest commit that changed files + in the given directory or below. This identifies the first release version + that included the directory's current state. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakedeploy/client.py(2 hunks)snakedeploy/snakemake_wrappers.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.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.
Files:
snakedeploy/client.pysnakedeploy/snakemake_wrappers.py
🧬 Code graph analysis (2)
snakedeploy/client.py (1)
snakedeploy/snakemake_wrappers.py (1)
update_snakemake_wrappers(96-141)
snakedeploy/snakemake_wrappers.py (2)
snakedeploy/providers.py (2)
Github(81-113)name(87-88)snakedeploy/logger.py (2)
info(104-105)warning(107-108)
🪛 GitHub Actions: CI
snakedeploy/snakemake_wrappers.py
[error] 9-9: Ruff check failed: F401 [*] github.Github imported but unused. Remove unused import: github.Github.
🪛 Ruff (0.13.3)
snakedeploy/snakemake_wrappers.py
19-19: subprocess call: check for execution of untrusted input
(S603)
20-20: Starting a process with a partial executable path
(S607)
30-30: subprocess call: check for execution of untrusted input
(S603)
31-31: Starting a process with a partial executable path
(S607)
58-65: Starting a process with a partial executable path
(S607)
70-70: Starting a process with a partial executable path
(S607)
74-74: Starting a process with a partial executable path
(S607)
75-75: subprocess call: check for execution of untrusted input
(S603)
76-77: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
81-81: Starting a process with a partial executable path
(S607)
🔇 Additional comments (5)
snakedeploy/client.py (2)
220-221: LGTM!The updated help text accurately reflects the new per-wrapper version resolution behavior.
315-315: LGTM!The simplified function call correctly matches the updated signature of
update_snakemake_wrapperswhich no longer requires agit_refparameter.snakedeploy/snakemake_wrappers.py (3)
46-51: LGTM!The sparse checkout patterns correctly target wrapper files, environment definitions, and test Snakefiles.
53-93: LGTM!The
WrapperRepocontext manager correctly:
- Clones the repository with sparse checkout for efficiency
- Configures sparse checkout patterns to fetch only necessary files
- Provides per-wrapper version resolution via
get_wrapper_version- Cleans up the temporary directory on exit
99-141: LGTM!The refactored implementation correctly:
- Uses the
WrapperRepocontext manager for per-wrapper version resolution- Handles both local wrapper specs and URL-based specs appropriately
- Provides informative logging for updates, warnings, and no-ops
- Preserves the wrapper spec format when updating versions
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
snakedeploy/snakemake_wrappers.py (1)
95-95: Update the outdated docstring.The docstring still references "the given git ref" parameter which no longer exists after removing the git_ref parameter.
Apply this diff:
- """Set all snakemake wrappers to the given git ref (e.g. tag or branch).""" + """Update all snakemake wrappers to their specific latest versions."""
🧹 Nitpick comments (2)
snakedeploy/snakemake_wrappers.py (2)
72-78: Remove redundant sparse-checkout disable.Line 72 disables sparse-checkout, but line 73-78 immediately reconfigures it with
set --no-cone. Thedisablecall is unnecessary and can be removed.Apply this diff:
sp.run( ["git", "config", "core.sparseCheckoutCone", "false"], cwd=self.tmpdir.name, check=True, ) - sp.run(["git", "sparse-checkout", "disable"], cwd=self.tmpdir.name, check=True) sp.run( ["git", "sparse-checkout", "set", "--no-cone"] + list(get_sparse_checkout_patterns()),
73-78: Consider using iterable unpacking for cleaner list concatenation.As per coding guidelines
Apply this diff:
sp.run( - ["git", "sparse-checkout", "set", "--no-cone"] - + list(get_sparse_checkout_patterns()), + ["git", "sparse-checkout", "set", "--no-cone", *get_sparse_checkout_patterns()], cwd=self.tmpdir.name, check=True, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakedeploy/snakemake_wrappers.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.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.
Files:
snakedeploy/snakemake_wrappers.py
🧬 Code graph analysis (1)
snakedeploy/snakemake_wrappers.py (1)
snakedeploy/logger.py (2)
info(104-105)warning(107-108)
🪛 GitHub Actions: CI
snakedeploy/snakemake_wrappers.py
[error] 99-99: FileNotFoundError: [Errno 2] No such file or directory: 'v1.4.0' while attempting to update snakemake wrappers. The command 'snakedeploy update-snakemake-wrappers --git-ref v1.4.0 /tmp/tmp.uoMP0OBFWz/test-snakefile.smk' failed with exit code 1.
🪛 Ruff (0.13.3)
snakedeploy/snakemake_wrappers.py
17-17: subprocess call: check for execution of untrusted input
(S603)
18-18: Starting a process with a partial executable path
(S607)
28-28: subprocess call: check for execution of untrusted input
(S603)
29-29: Starting a process with a partial executable path
(S607)
56-63: Starting a process with a partial executable path
(S607)
68-68: Starting a process with a partial executable path
(S607)
72-72: Starting a process with a partial executable path
(S607)
73-73: subprocess call: check for execution of untrusted input
(S603)
74-75: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
79-79: Starting a process with a partial executable path
(S607)
🔇 Additional comments (1)
snakedeploy/snakemake_wrappers.py (1)
44-48: LGTM!The sparse checkout patterns correctly cover wrapper files, environment files, and test Snakefiles needed for version resolution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snakedeploy/snakemake_wrappers.py (1)
73-78: Consider using iterable unpacking for cleaner code.The list concatenation works correctly but can be simplified using iterable unpacking.
Apply this diff:
sp.run( - ["git", "sparse-checkout", "set", "--no-cone"] - + list(get_sparse_checkout_patterns()), + ["git", "sparse-checkout", "set", "--no-cone", *get_sparse_checkout_patterns()], cwd=self.tmpdir.name, check=True, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakedeploy/snakemake_wrappers.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.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.
Files:
snakedeploy/snakemake_wrappers.py
🧠 Learnings (1)
📚 Learning: 2025-10-07T06:27:30.749Z
Learnt from: johanneskoester
PR: snakemake/snakedeploy#105
File: snakedeploy/snakemake_wrappers.py:27-41
Timestamp: 2025-10-07T06:27:30.749Z
Learning: In `snakedeploy/snakemake_wrappers.py`, the `get_latest_git_tag()` function intentionally returns the earliest (not latest) tag that contains the last commit affecting a given path. This represents the minimum version where a wrapper reached its current state. The ascending sort order (`--sort creatordate`) and returning `tags[0]` is the correct behavior.
Applied to files:
snakedeploy/snakemake_wrappers.py
🧬 Code graph analysis (1)
snakedeploy/snakemake_wrappers.py (1)
snakedeploy/logger.py (2)
info(104-105)warning(107-108)
🪛 Ruff (0.13.3)
snakedeploy/snakemake_wrappers.py
17-17: subprocess call: check for execution of untrusted input
(S603)
18-18: Starting a process with a partial executable path
(S607)
28-28: subprocess call: check for execution of untrusted input
(S603)
29-29: Starting a process with a partial executable path
(S607)
56-63: Starting a process with a partial executable path
(S607)
68-68: Starting a process with a partial executable path
(S607)
72-72: Starting a process with a partial executable path
(S607)
73-73: subprocess call: check for execution of untrusted input
(S603)
74-75: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
79-79: Starting a process with a partial executable path
(S607)
🔇 Additional comments (3)
snakedeploy/snakemake_wrappers.py (3)
10-48: LGTM! Helper functions are well-implemented.The
get_latest_git_tagfunction correctly implements the logic to find the earliest tag containing the last commit affecting a path, which represents the minimum version where a wrapper reached its current state. Theget_sparse_checkout_patternsfunction generates appropriate patterns for sparse checkout of wrapper files and environment files.Based on learnings: The ascending sort order and returning
tags[0]is the correct behavior.
51-91: LGTM! Context manager properly handles resource lifecycle.The
WrapperRepoclass correctly implements a context manager for managing a temporary sparse checkout of the snakemake-wrappers repository. The sparse checkout workflow (clone without checkout, configure, then populate with read-tree) is properly implemented, and resource cleanup is guaranteed via the__exit__method.
94-148: LGTM! Comprehensive error handling and logging.The
update_snakemake_wrappersfunction is well-implemented with proper error handling for malformed wrapper specs (lines 106-113) and cases where the latest version cannot be determined (lines 116-121). The regex pattern correctly matches bothwrapper:andmeta_wrapper:specifications with appropriate quote handling via backreference.The logging appropriately uses
infofor successful updates andwarningfor issues, providing clear feedback to users about what changes were made or why certain specs were left unchanged.
🤖 I have created a release *beep* *boop* --- ## [0.14.0](v0.13.0...v0.14.0) (2025-10-07) ### Features * update wrappers to their specific latest version instead of the global latest version of the wrapper repo ([#105](#105)) ([a5d3bf8](a5d3bf8)) ### Bug Fixes * update pixi envs for building ([#104](#104)) ([a4aabc9](a4aabc9)), closes [#102](#102) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit