-
-
Notifications
You must be signed in to change notification settings - Fork 292
Re-arrange folder structure #2424
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
Reviewer's GuideRefactors script organization by introducing a dedicated .scripts directory with a new set_permissions helper script and a small cleanup in run_script.sh, adding permission-setting logic with safety checks and a test hook. Sequence diagram for invoking the new set_permissions script via test_set_permissionssequenceDiagram
actor User
participant Shell
participant test_set_permissions
participant run_script
participant set_permissions
User ->> Shell: execute DockSTARTer tests
Shell ->> test_set_permissions: call function
test_set_permissions ->> run_script: run_script set_permissions
run_script ->> set_permissions: source and execute function
set_permissions ->> set_permissions: validate CH_PATH safety
set_permissions ->> set_permissions: optionally chown and chmod CH_PATH
set_permissions ->> set_permissions: ensure SCRIPTNAME is executable
set_permissions -->> run_script: return
run_script -->> test_set_permissions: return
test_set_permissions -->> Shell: return
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - I've left some high level feedback:
- The new
set_permissionshelper is placed under.scripts, butrun_scriptstill looks only inscripts/, sotest_set_permissionscallingrun_script 'set_permissions'will fail unless you either move the script intoscripts/or updaterun_scriptto also search.scripts. - Having
set_permissionsunconditionally invokesudomakes it harder to reuse in non-interactive or constrained environments; consider honoring an existingSUDO/DS_SUDOwrapper or adding a way to disablesudofor callers that already run with appropriate privileges.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `set_permissions` helper is placed under `.scripts`, but `run_script` still looks only in `scripts/`, so `test_set_permissions` calling `run_script 'set_permissions'` will fail unless you either move the script into `scripts/` or update `run_script` to also search `.scripts`.
- Having `set_permissions` unconditionally invoke `sudo` makes it harder to reuse in non-interactive or constrained environments; consider honoring an existing `SUDO`/`DS_SUDO` wrapper or adding a way to disable `sudo` for callers that already run with appropriate privileges.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Pull request
Purpose
Describe the problem or feature in addition to a link to the issues.
Approach
How does this change address the problem?
Open Questions and Pre-Merge TODOs
Check all boxes as they are completed
Learning
Describe the research stage
Links to blog posts, patterns, libraries or addons used to solve this problem
Requirements
Check all boxes as they are completed
Summary by Sourcery
Add a dedicated helper for safely setting file ownership and permissions and integrate it with the existing script runner.
New Features:
Enhancements: