-
Notifications
You must be signed in to change notification settings - Fork 13
feat: when deploying profiles also add non standard directory paths #108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughUpdates adjust deploy_profile to search profiles in two locations with added logging; other changes reformat assertions and dictionary key spacing without altering behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Deploy as deploy_profile
participant FS as Filesystem
User->>Deploy: invoke deploy_profile(...)
Deploy->>FS: check repo_root/profiles/
alt profiles found at root
Deploy->>FS: copy profiles to target (dirs_exist_ok=force)
Deploy-->>User: log source path + contribution info
else profiles not at root
Deploy->>FS: check workflow/profiles/
alt profiles found in workflow
Deploy->>FS: copy profiles to target (dirs_exist_ok=force)
Deploy-->>User: log source path + contribution info
else not found anywhere
Deploy-->>User: warn: no profiles directory found
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snakedeploy/deploy.py (1)
91-97: Consider clarifying which locations were checked in the warning.When no profile directory is found at either location, the warning message doesn't specify which paths were checked. This could leave users uncertain about where to place their profiles.
Apply this diff to improve clarity:
no_profile = not profile_dir.exists() if no_profile: logger.warning( - "No profile directory found in source repository. " + "No profile directory found in source repository " + "(checked 'profiles' and 'workflow/profiles'). " "Please check whether the source repository really does not " "need or provide any profiles." )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (3)
snakedeploy/conda.py(1 hunks)snakedeploy/deploy.py(2 hunks)versioneer.py(2 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/deploy.pyversioneer.pysnakedeploy/conda.py
🧬 Code graph analysis (1)
snakedeploy/deploy.py (1)
snakedeploy/logger.py (1)
info(104-105)
🔇 Additional comments (5)
snakedeploy/conda.py (1)
130-132: LGTM!The assertion logic is unchanged; this is a pure formatting refactor.
snakedeploy/deploy.py (2)
84-90: LGTM!The two-step profile directory lookup (root first, then workflow subdirectory) correctly implements the feature described in the PR objectives. The fallback mechanism is sound and the code silently handles the non-standard directory as intended.
99-106: LGTM!The informative logging about the source path and the encouragement to contribute adapted profiles back to the original repository are valuable additions for users.
versioneer.py (2)
420-422: LGTM!The dictionary key assignment formatting is changed without altering behavior.
1459-1461: LGTM!The assertion logic is unchanged; this is a pure formatting refactor.
johanneskoester
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm. Snakedeploy is designed to reward sticking to the standards. I am hesitant to break with that rule.
|
Yes, I understand that. It was an attempt. I first overlooked that the feature has already been merged. And then felt, it is probably worth contemplating. Got ahead of myself. |
This PR merely adds another directory to deploy when deploying profiles. Yes, this is a non-standard approach, but snakedeploy will silently ignore this. I recommend doing so to avoid dissappointment from users.
Summary by CodeRabbit
New Features
Refactor
Style