feat: add --commit flag to deploy-workflow#127
Open
ntnn19 wants to merge 1 commit into
Open
Conversation
snakedeploy deploy-workflow previously only allowed pinning a workflow deployment to a --tag or --branch, even though the underlying Snakemake github() helper also supports pinning to an exact commit via commit=. This meant users wanting full reproducibility (pinning to a specific commit rather than a potentially moving branch, or a repo with no tags) had to manually edit the generated Snakefile after deployment. This adds a --commit option to the deploy-workflow subcommand: - --commit can be used standalone, or combined with --branch/--tag. - When given, it takes precedence for both the local checkout performed during deployment and the ref written into the generated module's github(...) call (commit="<sha>" instead of tag=/branch=). - The local/Gitlab providers were updated accordingly, and a new test case was added to tests/test_client.sh. - Docs in workflow_deployment.rst updated to document the new flag.
Contributor
📝 WalkthroughWalkthroughWorkflow deployment now accepts an optional commit SHA, prioritizes it for checkout and generated module declarations, documents its precedence with branch and tag options, and adds an integration test for the pinned declaration. ChangesCommit-pinned deployment
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as deploy-workflow CLI
participant Deployer as WorkflowDeployer
participant Repository as Git repository
participant Provider as Github provider
CLI->>Deployer: pass commit, branch, and tag
Deployer->>Repository: checkout commit
Deployer->>Provider: request source declaration with commit
Provider-->>Deployer: return commit-pinned declaration
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
ntnn19
changed the title
ntnn19
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_client.sh (1)
54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest checkout precedence, not only declaration rendering.
The current assertion would pass even if checkout still used the branch or skipped commit checkout. Add a commit-only invocation and verify a copied file or focused unit test reflects the requested commit.
🤖 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/test_client.sh` around lines 54 - 55, The deploy-workflow test currently verifies only that the commit is rendered in the Snakefile, not that checkout uses it. Extend the test around the existing runTest invocations with a commit-only deployment and assert that a copied file or focused unit test matches the requested commit, ensuring commit checkout takes precedence over the branch.
🤖 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 `@snakedeploy/client.py`:
- Around line 92-98: The precedence documentation must state that commit
overrides branch and tag for both checkout and generated declarations. Update
the --commit help text in snakedeploy/client.py lines 92-98 and the public API
docstring in snakedeploy/deploy.py lines 283-284 to describe this precedence
consistently.
In `@snakedeploy/deploy.py`:
- Around line 261-262: Preserve positional API compatibility by placing the
existing force parameter before the newly added commit parameter in both
deploy() and WorkflowDeployer.__init__. Update snakedeploy/deploy.py lines
261-262 and 21-24 respectively; keep all other parameter behavior unchanged.
In `@snakedeploy/providers.py`:
- Around line 77-80: Ensure the Local provider does not silently ignore a
requested commit: either implement commit-aware checkout in Local.checkout or
explicitly reject --commit for Local before deployment proceeds. Update the
related get_source_file_declaration flow only as needed to preserve the
requested snapshot behavior.
In `@tests/test_client.sh`:
- Around line 54-55: Quote the new shell expansions in the deploy-workflow test:
update the commit argument to use "${commit_sha}" and the Snakefile path passed
to grep to use "${dest}/workflow/Snakefile", preserving the existing runTest
behavior.
---
Nitpick comments:
In `@tests/test_client.sh`:
- Around line 54-55: The deploy-workflow test currently verifies only that the
commit is rendered in the Snakefile, not that checkout uses it. Extend the test
around the existing runTest invocations with a commit-only deployment and assert
that a copied file or focused unit test matches the requested commit, ensuring
commit checkout takes precedence over the branch.
🪄 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: 9d53391b-09b9-4175-99a4-903703d0f657
📒 Files selected for processing (5)
docs/workflow_users/workflow_deployment.rstsnakedeploy/client.pysnakedeploy/deploy.pysnakedeploy/providers.pytests/test_client.sh
Comment on lines
+92
to
+98
| deploy_workflow_parser.add_argument( | ||
| "--commit", | ||
| help="Git commit (SHA) to deploy from and pin the resulting module to. " | ||
| "Can be combined with --branch or --tag to determine which ref to " | ||
| "clone/checkout locally, while pinning the generated Snakefile module " | ||
| "declaration to this exact commit.", | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make precedence documentation consistent with implementation. When commit is supplied, it takes precedence over branch/tag for checkout and generated declarations.
snakedeploy/client.py#L92-L98: update the CLI help text.snakedeploy/deploy.py#L283-L284: update the public API docstring.
📍 Affects 2 files
snakedeploy/client.py#L92-L98(this comment)snakedeploy/deploy.py#L283-L284
🤖 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 `@snakedeploy/client.py` around lines 92 - 98, The precedence documentation
must state that commit overrides branch and tag for both checkout and generated
declarations. Update the --commit help text in snakedeploy/client.py lines 92-98
and the public API docstring in snakedeploy/deploy.py lines 283-284 to describe
this precedence consistently.
Comment on lines
+261
to
262
| commit: Optional[str] = None, | ||
| force=False, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve positional compatibility when adding commit. Existing callers can silently bind their force argument to the new commit parameter.
snakedeploy/deploy.py#L261-L262: keepforcebeforecommitin the publicdeploy()signature.snakedeploy/deploy.py#L21-L24: apply the same ordering toWorkflowDeployer.__init__.
📍 Affects 1 file
snakedeploy/deploy.py#L261-L262(this comment)snakedeploy/deploy.py#L21-L24
🤖 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 `@snakedeploy/deploy.py` around lines 261 - 262, Preserve positional API
compatibility by placing the existing force parameter before the newly added
commit parameter in both deploy() and WorkflowDeployer.__init__. Update
snakedeploy/deploy.py lines 261-262 and 21-24 respectively; keep all other
parameter behavior unchanged.
Comment on lines
+77
to
80
| def get_source_file_declaration( | ||
| self, path: str, tag: str, branch: str, commit: Optional[str] = None | ||
| ): | ||
| relative_path = path.replace(self.source_url, "").strip(os.sep) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not silently ignore --commit for local repositories.
repo_clone invokes provider.checkout(..., commit), but Local.checkout() is a no-op and this declaration cannot encode the commit. A local deployment therefore copies the current working tree rather than the requested snapshot. Either implement commit checkout for Git-backed local sources or reject --commit for the Local provider.
🤖 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 `@snakedeploy/providers.py` around lines 77 - 80, Ensure the Local provider
does not silently ignore a requested commit: either implement commit-aware
checkout in Local.checkout or explicitly reject --commit for Local before
deployment proceeds. Update the related get_source_file_declaration flow only as
needed to preserve the requested snapshot behavior.
Comment on lines
+54
to
+55
| runTest 0 $output snakedeploy deploy-workflow "${repo}" "${dest}" --branch master --commit ${commit_sha} --name dna-seq-commit | ||
| runTest 0 $output grep "commit=\"${commit_sha}\"" ${dest}/workflow/Snakefile |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote the new shell variables.
commit_sha and the Snakefile path are expanded unquoted, allowing word splitting or glob expansion. Use "${commit_sha}" and "${dest}/workflow/Snakefile".
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 54-54: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 55-55: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 55-55: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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/test_client.sh` around lines 54 - 55, Quote the new shell expansions in
the deploy-workflow test: update the commit argument to use "${commit_sha}" and
the Snakefile path passed to grep to use "${dest}/workflow/Snakefile",
preserving the existing runTest behavior.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
snakedeploy deploy-workflow previously only allowed pinning a workflow deployment to a --tag or --branch, even though the underlying Snakemake github() helper also supports pinning to an exact commit via commit=. This meant users wanting full reproducibility (pinning to a specific commit rather than a potentially moving branch, or a repo with no tags) had to manually edit the generated Snakefile after deployment.
This adds a --commit option to the deploy-workflow subcommand:
LLM usage disclosure: LLM tools were used to assist in generating parts of the code and PR description. The resulting changes were manually reviewed, edited, and tested by the author before submission.
Summary by CodeRabbit
New Features
--commitoption.Documentation