feat: update reset_payram_environment steps for updater removal process#34
Open
aryaman-payram wants to merge 3 commits intomainfrom
Open
feat: update reset_payram_environment steps for updater removal process#34aryaman-payram wants to merge 3 commits intomainfrom
aryaman-payram wants to merge 3 commits intomainfrom
Conversation
aryaman-payram
requested review from
BuddhaSource and
iam-joey
and removed request for
iam-joey
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Summary by CodeRabbit
WalkthroughThe PayRam reset function was expanded from six to seven steps. A new Step 6 removes the PayRam Updater (remote uninstall script or manual fallback), and PayRam data directory deletion was moved to the final Step 7 after certificate and cron cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as setup_payram.sh
participant GitHub as GitHub/raw
participant OS as System (systemd / launchd)
participant FS as Filesystem
Script->>Script: start reset_payram_environment()
Script->>Script: run Steps 1–5 (existing cleanup)
Script->>GitHub: download & run setup_payram_updater.sh (--uninstall --yes --remove-backups)
alt remote uninstall succeeds
GitHub-->>Script: script runs and removes updater artifacts
Script->>OS: ensure service stopped/disabled
Script->>FS: updater files & backups removed
else remote uninstall fails
Script->>OS: detect and stop/disable updater (systemctl or launchctl)
Script->>FS: delete updater binary, unit/plist, data/backups, configs
Script->>FS: verify remaining updater artifacts
end
Script->>FS: Step 7 — remove PayRam data directory
Script->>Script: finish reset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@setup_payram.sh`:
- Around line 2411-2435: The cleanup conditional currently only checks for
/usr/local/bin/payram-updater and sets updater_removed, so leftover artifacts
like /usr/local/bin/payram-updater-launcher, /etc/payram/updater.env,
/Library/LaunchDaemons/com.payram.updater.plist,
/etc/systemd/system/payram-updater.service, /var/lib/payram(-updater) or backups
can be missed; modify the logic around updater_removed and the initial if-block
to detect any known updater artifacts (check existence of
/usr/local/bin/payram-updater, /usr/local/bin/payram-updater-launcher,
/Library/LaunchDaemons/com.payram.updater.plist,
/etc/systemd/system/payram-updater.service, /var/lib/payram*,
/var/lib/payram/backups, /etc/payram/updater.env) and enter the removal sequence
if any exist, and only print the yellow "not found" message when none of those
paths exist; ensure the same removal commands (service stop/unload, rm -f, rm
-rf, rmdir) run when any artifact is present and set updater_removed=true when
any removal was attempted.
- Around line 2406-2407: The curl pipeline invoking the external updater
uninstall (variable updater_uninstall_url and the line starting with "if curl
-fsSL") needs connection and overall time bounds to avoid hanging; update that
curl invocation to include the same flags used elsewhere (--connect-timeout 10
--max-time 60) so the command becomes curl -fsSL --connect-timeout 10 --max-time
60 "$updater_uninstall_url" 2>/dev/null | bash -s -- --uninstall --yes
--remove-backups 2>/dev/null, preserving the existing redirects and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
setup_payram.sh (2)
2429-2431: macOSlaunchctl unloadis deprecated but acceptable for compatibility.
launchctl unloadis deprecated on macOS 10.10+ in favor oflaunchctl bootout, but it still functions. The|| trueensures the script continues gracefully. For broad macOS version compatibility, this approach is acceptable.♻️ Optional: Use modern launchctl syntax on newer macOS
elif [[ -f /Library/LaunchDaemons/com.payram.updater.plist ]]; then - launchctl unload /Library/LaunchDaemons/com.payram.updater.plist 2>/dev/null || true + # bootout is preferred on macOS 10.10+; unload is deprecated but still works + launchctl bootout system /Library/LaunchDaemons/com.payram.updater.plist 2>/dev/null \ + || launchctl unload /Library/LaunchDaemons/com.payram.updater.plist 2>/dev/null \ + || true rm -f /Library/LaunchDaemons/com.payram.updater.plist 2>/dev/null || true fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_payram.sh` around lines 2429 - 2431, Replace the deprecated "launchctl unload /Library/LaunchDaemons/com.payram.updater.plist 2>/dev/null || true" with logic that prefers the modern "launchctl bootout" when supported: detect whether "launchctl bootout" is available (or check macOS version), call "launchctl bootout system /Library/LaunchDaemons/com.payram.updater.plist" when supported, otherwise fall back to "launchctl unload /Library/LaunchDaemons/com.payram.updater.plist"; keep the same error suppression "2>/dev/null || true" and the following "rm -f /Library/LaunchDaemons/com.payram.updater.plist 2>/dev/null || true" unchanged.
2407-2407: Consider logging curl errors for easier debugging.The timeouts have been correctly added. However, redirecting stderr to
/dev/nullhides network errors that could help diagnose why the remote uninstall script failed. Consider logging to$LOG_FILEfor better debugging while still keeping the fallback behavior.♻️ Optional: Log errors instead of discarding
- if curl -fsSL --connect-timeout 10 --max-time 60 "$updater_uninstall_url" 2>/dev/null | bash -s -- --uninstall --yes --remove-backups 2>/dev/null; then + if curl -fsSL --connect-timeout 10 --max-time 60 "$updater_uninstall_url" 2>>"$LOG_FILE" | bash -s -- --uninstall --yes --remove-backups 2>>"$LOG_FILE"; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_payram.sh` at line 2407, Replace the stderr redirections that discard errors with redirections that append errors to the log file so network/script errors are preserved; specifically, change the curl call that uses updater_uninstall_url from "curl -fsSL --connect-timeout 10 --max-time 60 \"$updater_uninstall_url\" 2>/dev/null | bash -s -- --uninstall --yes --remove-backups 2>/dev/null" to redirect curl stderr and the piped bash stderr to "$LOG_FILE" (e.g., curl ... 2>>"$LOG_FILE" | bash -s -- --uninstall --yes --remove-backups 2>>"$LOG_FILE"), ensuring LOG_FILE is defined and used consistently so errors are logged while keeping the same success/failure behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@setup_payram.sh`:
- Around line 2429-2431: Replace the deprecated "launchctl unload
/Library/LaunchDaemons/com.payram.updater.plist 2>/dev/null || true" with logic
that prefers the modern "launchctl bootout" when supported: detect whether
"launchctl bootout" is available (or check macOS version), call "launchctl
bootout system /Library/LaunchDaemons/com.payram.updater.plist" when supported,
otherwise fall back to "launchctl unload
/Library/LaunchDaemons/com.payram.updater.plist"; keep the same error
suppression "2>/dev/null || true" and the following "rm -f
/Library/LaunchDaemons/com.payram.updater.plist 2>/dev/null || true" unchanged.
- Line 2407: Replace the stderr redirections that discard errors with
redirections that append errors to the log file so network/script errors are
preserved; specifically, change the curl call that uses updater_uninstall_url
from "curl -fsSL --connect-timeout 10 --max-time 60 \"$updater_uninstall_url\"
2>/dev/null | bash -s -- --uninstall --yes --remove-backups 2>/dev/null" to
redirect curl stderr and the piped bash stderr to "$LOG_FILE" (e.g., curl ...
2>>"$LOG_FILE" | bash -s -- --uninstall --yes --remove-backups 2>>"$LOG_FILE"),
ensuring LOG_FILE is defined and used consistently so errors are logged while
keeping the same success/failure behavior.
sam6230i
approved these changes
Apr 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.