-
Notifications
You must be signed in to change notification settings - Fork 56
updatePanel script Cleanup #169
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
✅ Deploy Preview for pelica ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughRefactors Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant S as static/updatePanel.sh
participant FS as Filesystem
participant DB as Database
participant C as Composer
U->>S: invoke updatePanel.sh
S->>FS: ensure/create backup dir
alt backup dir fail
S-->>U: echo error + exit 1
end
S-->>U: detect DB_CONNECTION & DB_DATABASE (trim/ default sqlite)
alt sqlite
S-->>U: echo normalized DB filename
end
S->>FS: backup .env, storage assets, DB file(s)
alt backup fail
S-->>U: echo failure + exit 1
end
S->>FS: extract panel tarball
alt extract fail
S-->>U: echo failure + exit 1
end
S->>C: run composer install (post-restore, non-interactive)
alt composer fails
S-->>U: echo failure + exit 1
end
S->>FS: create storage symlink(s), set permissions/ownership
S->>DB: run migrations and seeders
alt migration fail
S-->>U: echo failure + exit 1
else success
S-->>U: "Panel Updated!" + exit 0
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
static/updatePanel.sh (2)
69-90: Fix sqlite backup path constructionWhen
DB_DATABASEalready contains a path (e.g.database/database.sqliteor an absolute/var/www/.../database.sqlite), the currentcp -a "$install_dir/database/$db_database"builds an invalid source path by prepending yet anotherdatabase/. The backup then aborts, which breaks the whole update for a default Laravel sqlite setup. Please resolve by normalizing the sqlite path before copying (detect absolute vs. relative) and generate the backup filename frombasename "$db_database".- cp -a "$install_dir/database/$db_database" "$backup_dir/$db_database.backup" + sqlite_path="$db_database" + if [[ "$sqlite_path" != /* ]]; then + sqlite_path="$install_dir/$sqlite_path" + fi + sqlite_filename=$(basename "$sqlite_path") + + cp -a "$sqlite_path" "$backup_dir/$sqlite_filename.backup"Be sure to reuse the same normalized variables later when restoring the database.
157-164: Restore sqlite using normalized pathAfter adjusting the backup step to normalize the sqlite path, the restore phase also needs to use the same resolved path/filename. Right now it still assumes
$install_dir/database/$db_database, so it will continue to fail for values that include a path. Mirror the backup fix here:-if [ -f "$backup_dir/$db_database.backup" ]; then - echo "Restoring sqlite database" - cp -a "$backup_dir/$db_database.backup" "$install_dir/database/$db_database" +if [ -n "$sqlite_filename" ] && [ -f "$backup_dir/$sqlite_filename.backup" ]; then + echo "Restoring sqlite database" + cp -a "$backup_dir/$sqlite_filename.backup" "$sqlite_path"Ensure
sqlite_path/sqlite_filenameare exported from the sqlite branch so they’re available here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
static/updatePanel.sh(5 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
static/updatePanel.sh
[warning] 166-166: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
Boy132
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.
Can't test it on an actual panel but code wise LGTM.
test using
sudo bash -c "$(curl -fsSL deploy-preview-169--pelica.netlify.app/updatePanel.sh)"Summary by CodeRabbit
New Features
Bug Fixes