feat: add profile directory and license file to deployment #75
Conversation
📝 WalkthroughWalkthroughThis update adds a new entry to the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant D as WorkflowDeployer
participant R as Repository
participant Dest as Destination
C->>D: deploy()
D->>D: deploy_profile()
alt "Profile directory exists"
D->>R: Check "profile" folder exists
R-->>D: Folder found
D->>Dest: Copy profile contents
else "Profile directory missing"
D->>D: Log warning and return indicator
end
D->>D: deploy_license()
D->>R: Search for license files using glob patterns
alt "License file found"
D->>Dest: Copy license file
else "License file not found"
D->>D: Return absence indicator
end
D->>C: Return deployment result
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
snakedeploy/deploy.py (3)
78-97: Well-implemented profile deployment functionality.The
deploy_profilemethod follows the same structure asdeploy_config, providing consistent behavior. However, the returnedno_profilevariable is assigned in thedeploymethod but never used.Consider either using the
no_profilevalue (perhaps in the post-instructions template) or removing the assignment in thedeploymethod:- no_profile = self.deploy_profile() + self.deploy_profile()
98-127: License deployment implementation looks good but has minor issues.The license deployment functionality works correctly, searching for license files with various naming patterns and copying the first match to the destination.
Three suggestions to improve this implementation:
- Simplify the conditional logic as suggested by the static analyzer:
- if license_file is None: - no_license = True - else: - no_license = not license_file.exists() + no_license = True if license_file is None else not license_file.exists()
Similar to
no_profile, theno_licensevariable is assigned but never used in thedeploymethod.Consider adding a log message when no license file is found, similar to how warnings are provided for missing config and profile directories.
🧰 Tools
🪛 Ruff (0.8.2)
116-119: Use ternary operator
no_license = True if license_file is None else not license_file.exists()instead ofif-else-blockReplace
if-else-block withno_license = True if license_file is None else not license_file.exists()(SIM108)
150-154: License and profile deployment integration.Good integration of the new deployment functions in the main
deploymethod.As noted earlier, the returned variables
no_profileandno_licenseare assigned but never used. Either use these variables (perhaps in the post-instructions template) or remove the assignments.🧰 Tools
🪛 Ruff (0.8.2)
151-151: Local variable
no_profileis assigned to but never usedRemove assignment to unused variable
no_profile(F841)
154-154: Local variable
no_licenseis assigned to but never usedRemove assignment to unused variable
no_license(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)snakedeploy/deploy.py(5 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.
snakedeploy/deploy.py
🪛 Ruff (0.8.2)
snakedeploy/deploy.py
116-119: Use ternary operator no_license = True if license_file is None else not license_file.exists() instead of if-else-block
Replace if-else-block with no_license = True if license_file is None else not license_file.exists()
(SIM108)
151-151: Local variable no_profile is assigned to but never used
Remove assignment to unused variable no_profile
(F841)
154-154: Local variable no_license is assigned to but never used
Remove assignment to unused variable no_license
(F841)
🪛 GitHub Actions: CI
snakedeploy/deploy.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Please run 'black' to fix code style issues in this file.
🔇 Additional comments (5)
.gitignore (1)
3-3: Good addition to ignore the build directory.Adding the
builddirectory to.gitignoreis appropriate as it prevents build artifacts from being tracked in version control.snakedeploy/deploy.py (4)
1-1: Appropriate import for license file discovery.The addition of the
globmodule is necessary for pattern matching when searching for license files.🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Please run 'black' to fix code style issues in this file.
47-49: Good addition of profile property.The new
profileproperty follows the same pattern as the existingsnakefileandconfigproperties, maintaining consistency in the codebase.
179-182: Consistent error handling for profile directory.The check for an existing profile directory follows the same pattern as the checks for the Snakefile and config directory, maintaining consistency in the codebase.
1-230:Details
❓ Verification inconclusive
Format code with Black to address pipeline failure.
The CI pipeline is failing due to Black formatting issues.
🏁 Script executed:
#!/bin/bash # Run black in check mode to identify formatting issues black --check snakedeploy/deploy.pyLength of output: 133
Action: Ensure Black is installed and run Black formatting on the file.
It appears that running the Black check failed because the Black command isn’t available in the current environment. Please confirm that Black is installed in your CI/development environment (for example, by running
pip install black) and then re-run the formatting check. Once installed, run:black snakedeploy/deploy.pyThis should reformat the file according to Black’s standards and resolve the CI pipeline failure. After applying these changes, please verify manually that the formatting issues are resolved.
🧰 Tools
🪛 Ruff (0.8.2)
116-119: Use ternary operator
no_license = True if license_file is None else not license_file.exists()instead ofif-else-blockReplace
if-else-block withno_license = True if license_file is None else not license_file.exists()(SIM108)
151-151: Local variable
no_profileis assigned to but never usedRemove assignment to unused variable
no_profile(F841)
154-154: Local variable
no_licenseis assigned to but never usedRemove assignment to unused variable
no_license(F841)
🪛 GitHub Actions: CI
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Please run 'black' to fix code style issues in this file.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
snakedeploy/deploy.py (3)
128-133:⚠️ Potential issueRemove duplicate and incomplete license code block
This code block appears to be a duplicate of the
deploy_licensefunctionality and looks incomplete. It should be removed as it's likely a merge artifact.- @property - def repo_clone(self): - if self._cloned is None: - logger.info("Writing license") - shutil.copy(license_file, self.dest_path) - return no_license
159-164:⚠️ Potential issueRemove duplicate deploy_license call
The
deploy_licensemethod is already called at line 154, so this second call is redundant and should be removed.- # Copy license if it exists - self.deploy_license() - - # Inspect repository to find existing snakefile - self.deploy_snakefile(self.repo_clone, name)
184-190:⚠️ Potential issueRemove duplicate method definition
This appears to be a duplicate partial definition of
deploy_snakefilethat contains code from thecheckmethod. It should be removed as it's likely a merge artifact.- def deploy_snakefile(self, tmpdir: str, name: str): - """ - if self.profile.exists() and not self.force: - raise UserError( - f"{self.profile} already exists, aborting (use --force to overwrite)" - ) -
🧹 Nitpick comments (1)
snakedeploy/deploy.py (1)
98-127: License deployment implementation looks good, with minor improvement suggestionsThe
deploy_licensemethod properly searches for license files with various naming patterns and copies them when found. A couple of improvements could be made:
- The code could have better error handling for when no licenses are found
- The if-else block for determining
no_licensecould be simplified- license_file = Path(licenses[0]) if len(licenses) != 0 else None - if license_file is None: - no_license = True - else: - no_license = not license_file.exists() + license_file = Path(licenses[0]) if licenses else None + no_license = True if license_file is None else not license_file.exists()Also, the empty
if no_license: passblock at line 121-122 is unnecessary and could be removed.🧰 Tools
🪛 Ruff (0.8.2)
123-126: Use ternary operator
no_license = True if license_file is None else not license_file.exists()instead ofif-else-blockReplace
if-else-block withno_license = True if license_file is None else not license_file.exists()(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snakedeploy/deploy.py(5 hunks)setup.py(0 hunks)snakedeploy/deploy.py(4 hunks)
💤 Files with no reviewable changes (1)
- setup.py
🚧 Files skipped from review as they are similar to previous changes (1)
- snakedeploy/deploy.py
🧰 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.
snakedeploy/deploy.py
🪛 Ruff (0.8.2)
snakedeploy/deploy.py
123-126: Use ternary operator no_license = True if license_file is None else not license_file.exists() instead of if-else-block
Replace if-else-block with no_license = True if license_file is None else not license_file.exists()
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: testing (3.9)
- GitHub Check: testing (3.8)
🔇 Additional comments (4)
snakedeploy/deploy.py (4)
47-49: LGTM: New profile property addedThe new property
profilecorrectly returns the path to the profile directory in the destination.
78-97: LGTM: Well-implemented profile deployment functionalityThe
deploy_profilemethod follows the same pattern as the existingdeploy_configmethod, making it consistent with the codebase. The implementation correctly checks for a profile directory in the cloned repository and handles both cases appropriately.
179-182: LGTM: Proper check for existing profile directoryThe additional check for the profile directory existence in the
checkmethod is correctly implemented, following the same pattern as the existing config check.
166-170:Details
❓ Verification inconclusive
Update template render to include new variables
The template rendering doesn't include the newly added
no_profileandno_licensevariables, which might be needed for proper instructions.
🏁 Script executed:
#!/bin/bash # Check if post-instructions.txt.jinja template references no_profile or no_license variables grep -r "no_profile\|no_license" $(find . -name "post-instructions.txt.jinja")Length of output: 130
Action Required: Align Template Variables with Template Usage
The current implementation insnakedeploy/deploy.py(lines 166–170) passes the newno_profileandno_licensevariables to the post-instructions template. However, a grep search onsnakedeploy/templates/post-instructions.txt.jinjadid not reveal any references to these variables.
- If the intention is for the template to use these variables to adjust its output, please update
snakedeploy/templates/post-instructions.txt.jinjaaccordingly.- Conversely, if these variables are not needed, consider removing them from the render call in
snakedeploy/deploy.pyfor consistency.Please verify that the changes align with the intended behavior of your deployment instructions.
johanneskoester
left a comment
johanneskoester
left a comment
There was a problem hiding this comment.
Great, thanks a lot! Just one thing, the place for workflow specific profiles is the subfolder profiles not profile. I have added corresponding changes below.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
snakedeploy/deploy.py (1)
98-134: Use a ternary operator to simplify the assignment of ‘no_license’.
As suggested by the static analysis tool, you can replace theif-elseblock with a conditional expression for cleaner code.- if license_file is None: - no_license = True - else: - no_license = not license_file.exists() + no_license = True if license_file is None else not license_file.exists()🧰 Tools
🪛 Ruff (0.8.2)
123-126: Use ternary operator
no_license = True if license_file is None else not license_file.exists()instead ofif-else-blockReplace
if-else-block withno_license = True if license_file is None else not license_file.exists()(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakedeploy/deploy.py(5 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.
snakedeploy/deploy.py
🪛 Ruff (0.8.2)
snakedeploy/deploy.py
123-126: Use ternary operator no_license = True if license_file is None else not license_file.exists() instead of if-else-block
Replace if-else-block with no_license = True if license_file is None else not license_file.exists()
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: testing (3.9)
- GitHub Check: testing (3.8)
🔇 Additional comments (4)
snakedeploy/deploy.py (4)
47-50: Great addition of the 'profiles' property.
It aligns well with the existing structure, ensuring consistent referencing and usage throughout the deployment logic.
157-159: No issues found with callingdeploy_profile().
Everything looks consistent here.
160-162: No issues found with callingdeploy_license().
It correctly integrates license deployment.
186-190: Logic to prevent overwriting profiles without the ‘force’ flag looks correct.
The condition preserves existing profiles unless explicitly overridden.
Fix the missed self.profiles reference. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
snakedeploy/deploy.py (1)
1-236: 💡 Verification agent❓ Verification inconclusive
Run Black to fix formatting issues.
The pipeline failures indicate that Black formatting check failed for this file.
🏁 Script executed:
#!/bin/bash # Format the file with Black to fix code style issues black snakedeploy/deploy.pyLength of output: 125
Action Required: Install Black for Code Formatting
The pipeline error shows that the
blackcommand is not found, which means it's not installed or accessible in your current environment. Please ensure that Black is added as a dependency (for example, viapip install black) in your project's environment or CI/CD pipeline. Once installed, re-run the formatting command to automatically fix code style issues.
- Confirm Black is installed and available in your environment.
- Update CI/CD settings if necessary to ensure Black runs and checks are correctly applied.
🧰 Tools
🪛 Ruff (0.8.2)
122-125: Use ternary operator
no_license = True if license_file is None else not license_file.exists()instead ofif-else-blockReplace
if-else-block withno_license = True if license_file is None else not license_file.exists()(SIM108)
🪛 GitHub Actions: CI
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Please run 'black' to fix code style issues in this file.
🧹 Nitpick comments (1)
snakedeploy/deploy.py (1)
97-132: Simplify the license file existence check.The license deployment functionality looks good, but the check logic can be slightly simplified.
- license_file = Path(licenses[0]) if len(licenses) != 0 else None - if license_file is None: - no_license = True - else: - no_license = not license_file.exists() + license_file = Path(licenses[0]) if len(licenses) != 0 else None + no_license = True if license_file is None else not license_file.exists()🧰 Tools
🪛 Ruff (0.8.2)
122-125: Use ternary operator
no_license = True if license_file is None else not license_file.exists()instead ofif-else-blockReplace
if-else-block withno_license = True if license_file is None else not license_file.exists()(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakedeploy/deploy.py(5 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.
snakedeploy/deploy.py
🪛 Ruff (0.8.2)
snakedeploy/deploy.py
122-125: Use ternary operator no_license = True if license_file is None else not license_file.exists() instead of if-else-block
Replace if-else-block with no_license = True if license_file is None else not license_file.exists()
(SIM108)
🪛 GitHub Actions: CI
snakedeploy/deploy.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Please run 'black' to fix code style issues in this file.
🔇 Additional comments (4)
snakedeploy/deploy.py (4)
47-49: LGTM: Profiles property looks good.The property implementation correctly returns the path to the profiles directory.
78-96: LGTM: Profile deployment is well implemented.The profile deployment method follows the same pattern as the existing
deploy_configmethod, making the code consistent and maintainable. It properly handles the case when no profile directory exists and provides appropriate logging.
156-160: LGTM: Deploy method updates look good.The changes to the deploy method integrate the new profile and license deployment functionality appropriately.
185-188: LGTM: Consistent check for profiles directory.The addition to the check method follows the same pattern as the existing checks for other directories, maintaining code consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snakedeploy/deploy.py (1)
98-134: License deployment implementation is good with minor suggestionsThe implementation successfully handles multiple license file formats and copies the license to the destination. However, there are a few improvements that could be made:
- The ternary operator could simplify lines 123-126
- The empty
passstatement at line 129 is unnecessary- If multiple license files are found, only the first one is used. This might be worth mentioning in a comment.
- license_file = Path(licenses[0]) if len(licenses) != 0 else None - if license_file is None: - no_license = True - else: - no_license = not license_file.exists() + # If multiple license files are found, use the first one + license_file = Path(licenses[0]) if len(licenses) != 0 else None + no_license = True if license_file is None else not license_file.exists() - if no_license: - pass - else: + if not no_license: logger.info("Writing license") shutil.copy(license_file, self.dest_path)🧰 Tools
🪛 Ruff (0.8.2)
123-126: Use ternary operator
no_license = True if license_file is None else not license_file.exists()instead ofif-else-blockReplace
if-else-block withno_license = True if license_file is None else not license_file.exists()(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakedeploy/deploy.py(5 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.
snakedeploy/deploy.py
🪛 Ruff (0.8.2)
snakedeploy/deploy.py
123-126: Use ternary operator no_license = True if license_file is None else not license_file.exists() instead of if-else-block
Replace if-else-block with no_license = True if license_file is None else not license_file.exists()
(SIM108)
🔇 Additional comments (4)
snakedeploy/deploy.py (4)
47-50: Looks good!The
profilesproperty correctly follows the pattern established by the existingsnakefileandconfigproperties.
78-97: Implementation looks correct and consistentThe
deploy_profilemethod follows the same pattern as the existingdeploy_configmethod, making the code more maintainable. It properly checks for profile directory existence, provides appropriate logging, and returns a boolean to indicate profile absence.
157-162: Proper integration of new functionalityThe new calls to
deploy_profileanddeploy_licenseare well-placed in thedeploymethod and correctly reference issue #64 in the comment.
186-189: Check for profiles directory existence is consistentThis addition to the
checkmethod ensures that deployment won't overwrite an existing profiles directory unless explicitly forced, maintaining consistency with the checks for Snakefile and config directories.
🤖 I have created a release *beep* *boop* --- ## [0.11.0](v0.10.4...v0.11.0) (2025-03-14) ### Features * add profile directory and license file to deployment ([#75](#75)) ([f52f7ef](f52f7ef)) ### Bug Fixes * consider config.yml as configuration filename ([#85](#85)) ([6a8fd0d](6a8fd0d)) --- 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>
This PR implements #64 and also deploys a license if it's present in the repository.
As licenses usually state that they shall be included in all copies of a software this is a nice feature to have.
Summary by CodeRabbit