feat: Allow creation of PRs upon wrapper updates #111
Conversation
📝 WalkthroughWalkthroughAdds a centralized PR helper module ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (client.py)
participant Updater as Update Function (conda / wrappers)
participant PR as PR Class (snakedeploy.prs)
participant GitHub as GitHub API
CLI->>Updater: call with create_prs, per_snakefile_prs, entity_regex, pr_add_label
Updater->>Updater: process items (envs or snakefiles)
alt create_prs enabled
Note right of Updater: initialize PR(s) per config
Updater->>PR: add_file(path, content, is_updated, msg) [xN]
PR->>PR: validate entity_regex (optional) and collect files
PR->>GitHub: ensure branch exists (create if needed)
PR->>GitHub: update_file/create_file for each file
PR->>GitHub: check existing PR(s)
alt no existing PR
PR->>GitHub: create PR
alt label provided
PR->>GitHub: add label
end
end
PR-->>Updater: return created PR info
end
Updater-->>CLI: finish (logs/results)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snakedeploy/snakemake_wrappers.py (1)
138-195: Fix PR creation: add modified files before creating PRs
pr.create()currently sees an emptyself.fileslist, so it returns immediately — no commits, no branch, no PR. On the aggregated path, reusing the samePRobject and callingcreate()inside the loop will try to re-update already committed files on the second snakefile, which GitHub rejects with a 422. Please stage the modified Snakefile in the PR before callingcreate(), and only invokecreate()per iteration when we’re in the per-Snakefile flow.with open(snakefile, "r") as infile: snakefile_content = infile.read() + original_content = snakefile_content @@ snakefile_content = re.sub( "(?P<def>(meta_)?wrapper:\n?\s*)(?P<quote>['\"])(?P<spec>.+)(?P=quote)", update_spec, snakefile_content, ) with open(snakefile, "w") as outfile: outfile.write(snakefile_content) - - if create_prs: - assert pr is not None - pr.create() + if create_prs and snakefile_content != original_content: + assert pr is not None + pr.add_file( + snakefile, + snakefile_content, + is_updated=True, + msg=f"perf: autobump wrappers in {snakefile}.", + ) + if create_prs and per_snakefile_prs: + assert pr is not None + pr.create()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
snakedeploy/client.py(4 hunks)snakedeploy/conda.py(4 hunks)snakedeploy/prs.py(1 hunks)snakedeploy/snakemake_wrappers.py(3 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.pysnakedeploy/client.pysnakedeploy/conda.pysnakedeploy/prs.py
🧬 Code graph analysis (4)
snakedeploy/snakemake_wrappers.py (2)
snakedeploy/exceptions.py (1)
UserError(43-44)snakedeploy/prs.py (3)
PR(26-122)get_repo(15-20)create(61-122)
snakedeploy/client.py (2)
snakedeploy/conda.py (1)
update_conda_envs(43-62)snakedeploy/snakemake_wrappers.py (1)
update_snakemake_wrappers(98-195)
snakedeploy/conda.py (2)
snakedeploy/exceptions.py (1)
UserError(43-44)snakedeploy/prs.py (2)
PR(26-122)get_repo(15-20)
snakedeploy/prs.py (2)
snakedeploy/exceptions.py (1)
UserError(43-44)snakedeploy/logger.py (1)
info(104-105)
🪛 Ruff (0.14.2)
snakedeploy/snakemake_wrappers.py
112-112: Avoid specifying long messages outside the exception class
(TRY003)
116-118: Avoid specifying long messages outside the exception class
(TRY003)
snakedeploy/prs.py
48-48: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Use raise without specifying exception name
Remove exception name
(TRY201)
88-88: Use raise without specifying exception name
Remove exception name
(TRY201)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakedeploy/prs.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/prs.py
🧬 Code graph analysis (1)
snakedeploy/prs.py (3)
snakedeploy/providers.py (1)
Github(81-113)snakedeploy/exceptions.py (1)
UserError(43-44)snakedeploy/logger.py (1)
info(104-105)
🪛 Ruff (0.14.2)
snakedeploy/prs.py
48-48: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Use raise without specifying exception name
Remove exception name
(TRY201)
88-88: Use raise without specifying exception name
Remove exception name
(TRY201)
⏰ 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: testing
| pr_exists = any( | ||
| pr.head.label.split(":", 1)[1] == self.branch | ||
| for pr in self.repo.get_pulls(state="open", base=self.base_ref) | ||
| ) |
There was a problem hiding this comment.
Don't treat fork PR branches as ours.
Line 108 currently compares only the branch name derived from pr.head.label. For forked PRs this string looks like "forkowner:branch", so any fork that happens to use the same branch name as our automation makes us think a PR already exists and we skip creation. That blocks our automated PRs for common branch names. Please ensure we also match the head repository before concluding a PR exists.
- pr_exists = any(
- pr.head.label.split(":", 1)[1] == self.branch
- for pr in self.repo.get_pulls(state="open", base=self.base_ref)
- )
+ repo_full_name = self.repo.full_name
+ pr_exists = any(
+ pr.head.repo
+ and pr.head.repo.full_name == repo_full_name
+ and pr.head.ref == self.branch
+ for pr in self.repo.get_pulls(state="open", base=self.base_ref)
+ )📝 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.
| pr_exists = any( | |
| pr.head.label.split(":", 1)[1] == self.branch | |
| for pr in self.repo.get_pulls(state="open", base=self.base_ref) | |
| ) | |
| repo_full_name = self.repo.full_name | |
| pr_exists = any( | |
| pr.head.repo | |
| and pr.head.repo.full_name == repo_full_name | |
| and pr.head.ref == self.branch | |
| for pr in self.repo.get_pulls(state="open", base=self.base_ref) | |
| ) |
🤖 Prompt for AI Agents
In snakedeploy/prs.py around lines 108 to 111, the existence check only compares
the branch name from pr.head.label and thereby treats forks with the same branch
name as our branches; update the predicate to require both the head repo
identity and the branch match (for example compare pr.head.repo.full_name or
pr.head.repo.owner.login and pr.head.ref against self.repo.full_name/self.owner
and self.branch), and guard for pr.head.repo possibly being None before
accessing its attributes so deleted/unknown repos don't raise.
🤖 I have created a release *beep* *boop* --- ## [0.15.0](v0.14.0...v0.15.0) (2025-10-31) ### Features * Allow creation of PRs upon wrapper updates ([#111](#111)) ([6f7ed82](6f7ed82)) ### Bug Fixes * scheduler template ([#110](#110)) ([d7c8907](d7c8907)) --- 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