Update update script to use quoted install directory #185
Conversation
✅ Deploy Preview for pelica ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)(omitted — changes are to a single shell script and do not introduce multi-component sequential interactions) Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
static/updatePanel.sh (1)
186-192: Consider quoting variables for robustness.Both
chmod_commandandchown_commandcould benefit from proper quoting to handle installation directories with spaces or special characters. For example:-chmod_command="chmod -R 755 $install_dir/storage/* $install_dir/bootstrap/cache" +chmod_command="chmod -R 755 \"$install_dir\"/storage/* \"$install_dir\"/bootstrap/cache"Note: This is a pre-existing pattern throughout the script and not specific to this PR's changes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
static/updatePanel.sh
🔇 Additional comments (1)
static/updatePanel.sh (1)
186-186: LGTM! Change correctly implements the PR objective.The use of
$install_dirprefixes ensures that the chmod command printed at lines 203-204 will work correctly regardless of the user's current working directory when they run it manually.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
static/updatePanel.sh (1)
166-166: Quote the directory variable to handle paths with spaces.The
cdcommand should quote$install_dirto prevent failures when the path contains spaces. This is inconsistent with the quoting improvements elsewhere in the script.🔎 Proposed fix
-cd $install_dir +cd "$install_dir"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
static/updatePanel.sh
🔇 Additional comments (1)
static/updatePanel.sh (1)
186-186: LGTM! Change achieves the PR objective.The chmod command now uses explicit
$install_dir-prefixed paths with proper quoting. When printed to users (lines 190, 203), the command will work correctly regardless of their current working directory, preventing the support requests mentioned in the PR description.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
static/updatePanel.sh (1)
186-197: LGTM: Achieves PR objective - commands now use absolute paths.The chmod and chown commands now correctly include
"$install_dir"prefixes, ensuring the printed commands (lines 190, 196) work regardless of the user's current working directory. This directly addresses the PR's goal of preventing support requests from users running commands in the wrong directory.The use of
evalis acceptable here but could be refactored to avoid it in the future by executing commands directly or using arrays.💡 Optional refactor to avoid eval
-chmod_command="chmod -R 755 \"$install_dir\"/storage/* \"$install_dir\"/bootstrap/cache" -eval $chmod_command +chmod -R 755 "$install_dir"/storage/* "$install_dir"/bootstrap/cache if [ $? -ne 0 ]; then echo "Failed to run chmod, Please run the following commands manually:" - echo "sudo $chmod_command" + echo "sudo chmod -R 755 \"$install_dir\"/storage/* \"$install_dir\"/bootstrap/cache" fi -chown_command="chown -R $owner:$group \"$install_dir\"" -eval $chown_command +chown -R "$owner:$group" "$install_dir" if [ $? -ne 0 ]; then echo "Failed to run chown, Please run the following commands manually:" - echo "sudo $chown_command" + echo "sudo chown -R $owner:$group \"$install_dir\"" fiNote: You would also need to update lines 203-204 to print the literal commands instead of variables.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
static/updatePanel.sh
🧰 Additional context used
🪛 Shellcheck (0.11.0)
static/updatePanel.sh
[warning] 166-166: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (4)
static/updatePanel.sh (4)
116-116: LGTM: Proper quoting in user-facing prompt.The quoted
"$install_dir"correctly handles paths with spaces or special characters in the user-facing message.
123-128: LGTM: Safe deletion approach with proper exclusions.The find-based deletion is safer than wildcards and properly excludes the backup and plugins directories. The exclusion of 'panel.tar.gz' is harmless but may be unnecessary since line 101 downloads it to the current working directory, not
$install_dir.
136-139: LGTM: Good cleanup with appropriate error handling.Removing the tarball after extraction is good housekeeping. The non-fatal error handling (continuing rather than aborting) is appropriate for this cleanup step.
202-204: LGTM: Clear remediation instructions with absolute paths.The final reminder messages now display commands with absolute paths, ensuring users can copy-paste and run them successfully from any directory. This is a good UX improvement that should reduce support requests.
AndyIsHereBoi
changed the title
rmartinoscar
left a comment
rmartinoscar
left a comment
There was a problem hiding this comment.
Thanks for your contribution ! 👍
|
I was thinking about this too but not sure if it should be added here as well, i already added quotes in the other printed messages so i figured i might as well ask. Is it worth editing the message that says "Deleted all files and folders in "$install_dir" except the backup folder." to "... except the backup and plugins folders." |
No don't worry i will rework that part to also include icons/{server,egg} & plugins with also cleaner backup process. |
2 participants
While the script will already be in the
$install_dirfolder, the chmod command is also displayed at the end of the script and printed out to the user. If the user is not already in their install directory when running the last 2 recommended commands, the chmod will fail. This is mainly to prevent any unnecessary help requests in the Discord when it could be an easy fix.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.