Debian Improvements #732
Debian Improvements #732
Conversation
English ReadME
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (4)
pkg/deb/DEBIAN/postrm (1)
1-3: Add documentation and conform to Debian PolicyThe script lacks documentation about its purpose and usage. Additionally, it should handle all package removal scenarios as per Debian Policy Manual.
Add these comments at the beginning of the script:
#!/bin/sh -e +# postrm script for peerbanhelper +# +# See: https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html +# +# This script is called with the following arguments: +# purge: Remove all data and configuration +# remove: Remove the package +# upgrade: Upgrade to a newer version +# failed-upgrade: Failed upgrade attempt +# abort-install: Aborted installation +# abort-upgrade: Aborted upgrade +# disappear: Package replaced by anotherpkg/deb/DEBIAN/postinst (2)
1-2: Enhance script safety with additional shell flagsWhile
-eis good for failing fast, consider adding-uto catch undefined variables and-o pipefailto catch pipe failures.-#!/bin/sh -e +#!/bin/sh -eu +set -o pipefail
1-10: Consider adding operational safeguardsA few architectural considerations:
- Add logrotate configuration to prevent log directory overflow
- Validate configuration files before starting the service
- Consider adding a health check after service start
Would you like me to provide implementation examples for these improvements?
pkg/deb/DEBIAN/preinst (1)
1-2: Enhance shell script safety settingsConsider adding additional safety flags to prevent common shell script issues:
#!/bin/sh -e +set -u # exit on undefined variables +set -o pipefail # exit on pipe failures
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
pkg/deb/DEBIAN/conffiles(1 hunks)pkg/deb/DEBIAN/control(1 hunks)pkg/deb/DEBIAN/dirs(0 hunks)pkg/deb/DEBIAN/postinst(1 hunks)pkg/deb/DEBIAN/postrm(1 hunks)pkg/deb/DEBIAN/preinst(1 hunks)pkg/deb/DEBIAN/prerm(1 hunks)pkg/deb/etc/peerbanhelper/profile.yml(1 hunks)pkg/deb/usr/lib/systemd/system/peerbanhelper.service(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/deb/DEBIAN/dirs
✅ Files skipped from review due to trivial changes (2)
- pkg/deb/DEBIAN/conffiles
- pkg/deb/etc/peerbanhelper/profile.yml
🧰 Additional context used
📓 Learnings (1)
pkg/deb/usr/lib/systemd/system/peerbanhelper.service (1)
Learnt from: Ghost-chu
PR: PBH-BTN/PeerBanHelper#698
File: pkg/deb/usr/lib/systemd/system/peerbanhelper.service:0-0
Timestamp: 2024-11-12T05:44:34.213Z
Learning: In the PeerBanHelper project, avoid repeatedly suggesting the addition of security hardening options to the systemd service files if the user has declined them.
🔇 Additional comments (8)
pkg/deb/DEBIAN/prerm (1)
1-2: LGTM! Good error handling setup.
The use of -e flag ensures the script fails fast on errors, which is a good practice for package management scripts.
pkg/deb/DEBIAN/postrm (1)
1-1: LGTM: Appropriate shebang with error handling flag
The use of -e flag is good practice as it ensures the script fails fast on errors.
pkg/deb/DEBIAN/control (2)
3-3: Verify the necessity of Pre-Depends.
While adding adduser as a Pre-Depends aligns with the user management changes, consider if a regular Depends would suffice. Pre-Depends is a stronger requirement that can complicate package management and should only be used when absolutely necessary.
#!/bin/bash
# Description: Check if other similar packages use Pre-Depends for adduser
# and verify the installation scripts that require it
# Check for adduser in Pre-Depends of other packages
dpkg-query -W -f='${Package}: ${Pre-Depends}\n' | grep adduser
# Look for user creation in installation scripts
rg -l "useradd|adduser" "pkg/deb/DEBIAN/"3-4: Review dependency specifications.
The dependency configuration has some potential concerns:
- The Java runtime requirement of >=21 is quite restrictive as it's a recent version. Consider if supporting older LTS versions (like 17) would be beneficial for wider compatibility.
- Consider adding
adduserto regularDependsinstead ofPre-Dependsunless there's a critical installation ordering requirement.
✅ Verification successful
Let me check the preinst and postinst scripts to understand why adduser is in Pre-Depends.
The dependency specifications are appropriate for the application's requirements
The current dependency configuration is well-justified:
-
Java 21 requirement is appropriate as it's explicitly required by the project (defined in pom.xml), and this is a hard technical requirement, not just a compatibility choice.
-
Pre-Depends: adduseris correctly specified because:- The preinst script requires
addusercommand for potential user migration - The postinst script uses
adduserto create system user before other installation steps - This makes it a critical installation ordering requirement, validating the use of
Pre-Depends
- The preinst script requires
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Java runtime availability in Debian repositories
# Check available Java versions in Debian
curl -s https://packages.debian.org/search?keywords=java-runtime | grep -o "openjdk-[0-9]*-jre"
# Look for Java version requirements in the codebase
rg "java\.version|JAVA_VERSION"
Length of output: 478
Script:
#!/bin/bash
# Check the content of preinst and postinst scripts
cat pkg/deb/DEBIAN/preinst
echo "---"
cat pkg/deb/DEBIAN/postinst
Length of output: 980
pkg/deb/usr/lib/systemd/system/peerbanhelper.service (3)
4-7: LGTM: Service configuration follows best practices
The service is properly configured to run as a dedicated user, which is a security best practice.
9-12: LGTM: Service behavior configuration is well-defined
The service behavior configuration includes:
SuccessExitStatus=143: Correctly handles Java application termination signalsAmbientCapabilities=CAP_NET_ADMIN: Provides necessary network capabilitiesRestart=on-failure: Ensures service reliability while avoiding restart loops
7-8: Verify directory permissions
The service uses several directories that need specific permissions:
/usr/lib/peerbanhelper: Working directory/var/lib/peerbanhelper: Data directory/etc/peerbanhelper: Config directory/var/log/peerbanhelper: Logs directory
✅ Verification successful
Let me generate another verification script to check the complete installation scripts and user setup:
This will help us verify:
- Complete installation scripts to understand the full setup process
- User creation for the service
- Directory creation commands and their order
- Any missing permission settings
Directory permissions and ownership are properly configured
The installation scripts correctly handle directory permissions and ownership:
- System user
peerbanhelperis created with/var/lib/peerbanhelperas home /etc/peerbanhelper: owned by peerbanhelper:peerbanhelper/usr/lib/peerbanhelper: owned by root:root/var/log/peerbanhelper: owned by peerbanhelper:peerbanhelper/var/lib/peerbanhelper: implicitly owned by peerbanhelper as it's the user's home
The setup ensures proper security isolation with root owning the application binaries while the service user owns runtime directories.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the installation scripts properly set up directory permissions
# Check directory existence and permissions
echo "Checking directory permissions..."
for dir in "/usr/lib/peerbanhelper" "/var/lib/peerbanhelper" "/etc/peerbanhelper" "/var/log/peerbanhelper"; do
fd -t d "^${dir##*/}$" / -x stat -c "Directory: %n\nOwner: %U:%G\nPermissions: %A" {}
done
# Check if installation scripts handle permissions
echo -e "\nChecking installation scripts..."
rg -A 5 "chown|chmod" pkg/deb/DEBIAN/
Length of output: 2456
Script:
#!/bin/bash
# Check complete installation scripts and user setup
echo "Checking complete installation scripts..."
echo -e "\n=== preinst ==="
cat pkg/deb/DEBIAN/preinst
echo -e "\n=== postinst ==="
cat pkg/deb/DEBIAN/postinst
echo -e "\n=== Checking user creation ==="
rg -A 5 "useradd|adduser" pkg/deb/DEBIAN/
echo -e "\n=== Checking directory creation ==="
rg -A 5 "mkdir" pkg/deb/DEBIAN/
Length of output: 2913
pkg/deb/DEBIAN/preinst (1)
1-12: Verify file permissions and ownership in the codebase
Let's ensure proper file permissions are maintained throughout the codebase.
✅ Verification successful
File permissions and ownership are properly configured
The verification shows a consistent and secure approach to file permissions and ownership:
- The preinst script correctly handles migration of files with appropriate ownership
- The postinst script sets proper ownership with root for application files and peerbanhelper user for data directories
- The systemd service runs as the dedicated peerbanhelper user with minimal required capabilities
- All file permission changes are properly scoped to system installation directories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any hard-coded permissions or ownership settings in other files
rg -l "chmod|chown" .
# Check for other preinst/postinst scripts that might conflict
fd -t f "(pre|post)(inst|rm)" .
# Check for systemd service files that might reference these paths
fd -t f "\.service$" .
Length of output: 419
Script:
#!/bin/bash
# Check the content of related files for permission and ownership settings
echo "=== postinst ==="
cat ./pkg/deb/DEBIAN/postinst
echo -e "\n=== systemd service file ==="
cat ./pkg/deb/usr/lib/systemd/system/peerbanhelper.service
echo -e "\n=== build script permissions ==="
cat ./pkg/synopkg/build-script.sh | grep -E "chmod|chown"
Length of output: 1270
| systemctl daemon-reload | ||
| systemctl stop peerbanhelper.service |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add error handling for systemd operations in both prerm and postinst scripts
The verification confirms that both installation scripts (prerm and postinst) lack proper error handling for systemd operations. The package appears to be systemd-only, with no alternative init system support files present. The suggested changes should be applied to both scripts:
prerm: Apply the suggested error handling as mentioned in the review.postinst: Add similar error handling:
#!/bin/sh -e
getent passwd peerbanhelper > /dev/null || adduser --quiet --system --group --home /var/lib/peerbanhelper peerbanhelper
chown -R peerbanhelper: /etc/peerbanhelper
chown -R root: /usr/lib/peerbanhelper
mkdir /var/log/peerbanhelper
chown peerbanhelper: /var/log/peerbanhelper
-systemctl daemon-reload
-systemctl start peerbanhelper.service
+# Check if systemd is running
+if [ -d /run/systemd/system ]; then
+ systemctl daemon-reload || true
+ systemctl start peerbanhelper.service || true
+else
+ echo "Warning: systemd not detected, skipping service management"
+fi🔗 Analysis chain
Add error handling for systemd-specific operations.
The script assumes systemd is present and running, which might not always be true. Consider adding checks and graceful fallbacks.
Here's a suggested implementation with better error handling:
#!/bin/sh -e
-systemctl daemon-reload
-systemctl stop peerbanhelper.service
+# Check if systemd is running
+if [ -d /run/systemd/system ]; then
+ # Stop service if it's running
+ if systemctl is-active --quiet peerbanhelper.service; then
+ systemctl stop peerbanhelper.service || true
+ fi
+ systemctl daemon-reload || true
+else
+ echo "Warning: systemd not detected, skipping service management"
+fiThis implementation:
- Checks if systemd is running before attempting systemd operations
- Only attempts to stop the service if it's actually running
- Uses
|| trueto prevent script failure if commands fail - Provides feedback when systemd is not available
Let's verify the service configuration:
The verification results show that:
- The service file exists and is properly configured
systemctlcommands are used in bothprermandpostinstscripts- The service requires special capabilities (
CAP_NET_ADMIN) and runs as a dedicated user
Let's verify the presence of similar error handling in the postinst script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify service configuration and related files
# Check if service file exists and its contents
fd -t f "peerbanhelper.service" -x cat {}
# Check for other related systemd unit files
fd -t f "peerbanhelper*.service"
# Look for any systemctl commands in other installation scripts
rg "systemctl" pkg/
Length of output: 880
Script:
#!/bin/bash
# Check the content of postinst script
cat pkg/deb/DEBIAN/postinst
# Check if there are any other init system related files
fd -t f "init|upstart|openrc" pkg/
Length of output: 417
| @@ -0,0 +1,3 @@ | |||
| #!/bin/sh -e | |||
|
|
|||
| [ $1 = purge ] && deluser --quiet peerbanhelper && rm -rf /var/lib/peerbanhelper || true | |||
There was a problem hiding this comment.
Improve error handling and safety checks
The current implementation has several potential issues:
- Suppressing all errors with
|| truecould hide important failures - No checks if user/directory exist before removal
- Using
-rfwithout safeguards is risky
Consider this safer implementation:
-[ $1 = purge ] && deluser --quiet peerbanhelper && rm -rf /var/lib/peerbanhelper || true
+case "$1" in
+ purge)
+ # Remove peerbanhelper user if exists
+ if id "peerbanhelper" >/dev/null 2>&1; then
+ if ! deluser --quiet peerbanhelper; then
+ echo "Warning: Failed to remove peerbanhelper user" >&2
+ fi
+ fi
+
+ # Remove data directory if exists
+ if [ -d "/var/lib/peerbanhelper" ]; then
+ if ! rm -rf "/var/lib/peerbanhelper"; then
+ echo "Warning: Failed to remove /var/lib/peerbanhelper directory" >&2
+ exit 1
+ fi
+ fi
+ ;;
+ remove|upgrade|failed-upgrade|abort-install|abort-upgrade|disappear)
+ # No action needed for other removal cases
+ ;;
+ *)
+ echo "postrm called with unknown argument '$1'" >&2
+ exit 1
+ ;;
+esac📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [ $1 = purge ] && deluser --quiet peerbanhelper && rm -rf /var/lib/peerbanhelper || true | |
| case "$1" in | |
| purge) | |
| # Remove peerbanhelper user if exists | |
| if id "peerbanhelper" >/dev/null 2>&1; then | |
| if ! deluser --quiet peerbanhelper; then | |
| echo "Warning: Failed to remove peerbanhelper user" >&2 | |
| fi | |
| fi | |
| # Remove data directory if exists | |
| if [ -d "/var/lib/peerbanhelper" ]; then | |
| if ! rm -rf "/var/lib/peerbanhelper"; then | |
| echo "Warning: Failed to remove /var/lib/peerbanhelper directory" >&2 | |
| exit 1 | |
| fi | |
| fi | |
| ;; | |
| remove|upgrade|failed-upgrade|abort-install|abort-upgrade|disappear) | |
| # No action needed for other removal cases | |
| ;; | |
| *) | |
| echo "postrm called with unknown argument '$1'" >&2 | |
| exit 1 | |
| ;; | |
| esac |
| #!/bin/sh -e | ||
|
|
||
| [ $1 = purge ] && deluser --quiet peerbanhelper && rm -rf /var/lib/peerbanhelper || true |
There was a problem hiding this comment.
💡 Codebase verification
Service files are not properly cleaned up during package removal
The package has systemd service files and service-related operations, but the postrm script doesn't clean up:
/usr/lib/systemd/system/peerbanhelper.service- service file remains after package removal- No
systemctl disablebefore removal - No removal of service files in
/usr/lib/systemd/system/
🔗 Analysis chain
Verify systemd service cleanup
Ensure that systemd-related files are properly cleaned up during package removal.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for systemd service files that might need cleanup
# Check if there are any systemd service files
rg -l "peerbanhelper" "/lib/systemd/system/" "/etc/systemd/system/"
# Check if there are any remaining systemd runtime files
rg -l "peerbanhelper" "/run/systemd/system/"
Length of output: 289
Script:
#!/bin/bash
# Let's check the repository for any systemd service files and their cleanup
# First, find any systemd service files in the repository
fd -e service
# Check for any systemd-related cleanup in package scripts
rg -l "systemctl|systemd" pkg/deb/
# Look for service definitions or systemd mentions in the codebase
rg -l "\.service"
# Check package scripts for service-related operations
cat pkg/deb/DEBIAN/{preinst,postinst,prerm,postrm} 2>/dev/null
Length of output: 1686
| #!/bin/sh -e | ||
|
|
||
| getent passwd peerbanhelper > /dev/null || adduser --quiet --system --group --home /var/lib/peerbanhelper peerbanhelper |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and explicit umask setting
While the user creation logic is correct, consider:
- Adding explicit error handling for adduser
- Setting a restrictive umask before user creation
+umask 022
+
getent passwd peerbanhelper > /dev/null || adduser --quiet --system --group --home /var/lib/peerbanhelper peerbanhelper || {
+ echo "Failed to create peerbanhelper user" >&2
+ exit 1
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getent passwd peerbanhelper > /dev/null || adduser --quiet --system --group --home /var/lib/peerbanhelper peerbanhelper | |
| umask 022 | |
| getent passwd peerbanhelper > /dev/null || adduser --quiet --system --group --home /var/lib/peerbanhelper peerbanhelper || { | |
| echo "Failed to create peerbanhelper user" >&2 | |
| exit 1 | |
| } |
| systemctl start peerbanhelper.service |
There was a problem hiding this comment.
Improve service management robustness
The service management needs better error handling and should enable the service for system startup.
Apply these changes:
-systemctl daemon-reload
-systemctl start peerbanhelper.service
+systemctl daemon-reload || {
+ echo "Failed to reload systemd daemon" >&2
+ exit 1
+}
+
+systemctl enable peerbanhelper.service || {
+ echo "Failed to enable peerbanhelper service" >&2
+ exit 1
+}
+
+systemctl start peerbanhelper.service || {
+ echo "Failed to start peerbanhelper service" >&2
+ exit 1
+}
+
+# Verify service is running
+systemctl is-active --quiet peerbanhelper.service || {
+ echo "Service failed to start properly" >&2
+ exit 1
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| systemctl daemon-reload | |
| systemctl start peerbanhelper.service | |
| systemctl daemon-reload || { | |
| echo "Failed to reload systemd daemon" >&2 | |
| exit 1 | |
| } | |
| systemctl enable peerbanhelper.service || { | |
| echo "Failed to enable peerbanhelper service" >&2 | |
| exit 1 | |
| } | |
| systemctl start peerbanhelper.service || { | |
| echo "Failed to start peerbanhelper service" >&2 | |
| exit 1 | |
| } | |
| # Verify service is running | |
| systemctl is-active --quiet peerbanhelper.service || { | |
| echo "Service failed to start properly" >&2 | |
| exit 1 | |
| } |
| WorkingDirectory=/etc/peerbanhelper | ||
| ExecStart=/usr/bin/java -Dpbh.release=debian -Dpbh.datadir=/etc/peerbanhelper -Xmx512M -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps -jar /usr/lib/peerbanhelper/PeerBanHelper.jar | ||
| WorkingDirectory=/usr/lib/peerbanhelper | ||
| ExecStart=/usr/bin/java -Dpbh.release=debian -Dpbh.datadir=/var/lib/peerbanhelper -Dpbh.configdir=/etc/peerbanhelper -Dpbh.logsdir=/var/log/peerbanhelper -Dpbh.log.level=WARN -Xmx512M -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps -jar PeerBanHelper.jar |
There was a problem hiding this comment.
Fix: Use absolute path for jar file
The jar file path has been changed from absolute (/usr/lib/peerbanhelper/PeerBanHelper.jar) to relative (PeerBanHelper.jar). While this might work due to WorkingDirectory, it's better to use absolute paths in systemd unit files for clarity and reliability.
-ExecStart=/usr/bin/java -Dpbh.release=debian -Dpbh.datadir=/var/lib/peerbanhelper -Dpbh.configdir=/etc/peerbanhelper -Dpbh.logsdir=/var/log/peerbanhelper -Dpbh.log.level=WARN -Xmx512M -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps -jar PeerBanHelper.jar
+ExecStart=/usr/bin/java -Dpbh.release=debian -Dpbh.datadir=/var/lib/peerbanhelper -Dpbh.configdir=/etc/peerbanhelper -Dpbh.logsdir=/var/log/peerbanhelper -Dpbh.log.level=WARN -Xmx512M -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps -jar /usr/lib/peerbanhelper/PeerBanHelper.jar📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ExecStart=/usr/bin/java -Dpbh.release=debian -Dpbh.datadir=/var/lib/peerbanhelper -Dpbh.configdir=/etc/peerbanhelper -Dpbh.logsdir=/var/log/peerbanhelper -Dpbh.log.level=WARN -Xmx512M -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps -jar PeerBanHelper.jar | |
| ExecStart=/usr/bin/java -Dpbh.release=debian -Dpbh.datadir=/var/lib/peerbanhelper -Dpbh.configdir=/etc/peerbanhelper -Dpbh.logsdir=/var/log/peerbanhelper -Dpbh.log.level=WARN -Xmx512M -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps -jar /usr/lib/peerbanhelper/PeerBanHelper.jar |
| deluser --quiet peerbanhelper | ||
| adduser --quiet --system --group --home /var/lib/peerbanhelper peerbanhelper |
There was a problem hiding this comment.
Add error handling for user management operations
The --quiet flag suppresses important error messages. Consider removing it and adding proper error handling.
-deluser --quiet peerbanhelper
-adduser --quiet --system --group --home /var/lib/peerbanhelper peerbanhelper
+if ! deluser peerbanhelper; then
+ echo "Error: Failed to remove existing peerbanhelper user" >&2
+ exit 1
+fi
+if ! adduser --system --group --home /var/lib/peerbanhelper peerbanhelper; then
+ echo "Error: Failed to create peerbanhelper user" >&2
+ exit 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deluser --quiet peerbanhelper | |
| adduser --quiet --system --group --home /var/lib/peerbanhelper peerbanhelper | |
| if ! deluser peerbanhelper; then | |
| echo "Error: Failed to remove existing peerbanhelper user" >&2 | |
| exit 1 | |
| fi | |
| if ! adduser --system --group --home /var/lib/peerbanhelper peerbanhelper; then | |
| echo "Error: Failed to create peerbanhelper user" >&2 | |
| exit 1 | |
| fi |
| find /etc/peerbanhelper -mindepth 1 -maxdepth 1 -type d -exec mv {} /var/lib/peerbanhelper \; | ||
| fi |
There was a problem hiding this comment.
Improve safety of directory migration
The find command could potentially move unintended directories. Add validation and error handling.
-find /etc/peerbanhelper -mindepth 1 -maxdepth 1 -type d -exec mv {} /var/lib/peerbanhelper \;
+# Move remaining directories with validation
+while IFS= read -r dir; do
+ dir_name=$(basename "$dir")
+ case "$dir_name" in
+ config|data|logs)
+ continue # Skip already handled directories
+ ;;
+ *)
+ if ! mv "$dir" /var/lib/peerbanhelper/; then
+ echo "Error: Failed to move directory $dir" >&2
+ exit 1
+ fi
+ ;;
+ esac
+done < <(find /etc/peerbanhelper -mindepth 1 -maxdepth 1 -type d)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| find /etc/peerbanhelper -mindepth 1 -maxdepth 1 -type d -exec mv {} /var/lib/peerbanhelper \; | |
| fi | |
| # Move remaining directories with validation | |
| while IFS= read -r dir; do | |
| dir_name=$(basename "$dir") | |
| case "$dir_name" in | |
| config|data|logs) | |
| continue # Skip already handled directories | |
| ;; | |
| *) | |
| if ! mv "$dir" /var/lib/peerbanhelper/; then | |
| echo "Error: Failed to move directory $dir" >&2 | |
| exit 1 | |
| fi | |
| ;; | |
| esac | |
| done < <(find /etc/peerbanhelper -mindepth 1 -maxdepth 1 -type d) | |
| fi |
| # migrate <= 7.1.2 | ||
| if getent passwd peerbanhelper | grep --quiet /nonexistent; then |
There was a problem hiding this comment.
Implement explicit version checking
The current version detection relies on checking the user's home directory, which is an indirect and potentially unreliable method. Consider implementing explicit version checking.
Example implementation:
# migrate <= 7.1.2
+if [ -f /var/lib/dpkg/status ]; then
+ OLD_VERSION=$(dpkg-query -W -f='${Version}' peerbanhelper 2>/dev/null || echo "0.0.0")
+ if dpkg --compare-versions "$OLD_VERSION" le "7.1.2"; then
+ if getent passwd peerbanhelper | grep --quiet /nonexistent; then
-if getent passwd peerbanhelper | grep --quiet /nonexistent; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # migrate <= 7.1.2 | |
| if getent passwd peerbanhelper | grep --quiet /nonexistent; then | |
| # migrate <= 7.1.2 | |
| if [ -f /var/lib/dpkg/status ]; then | |
| OLD_VERSION=$(dpkg-query -W -f='${Version}' peerbanhelper 2>/dev/null || echo "0.0.0") | |
| if dpkg --compare-versions "$OLD_VERSION" le "7.1.2"; then | |
| if getent passwd peerbanhelper | grep --quiet /nonexistent; then |
| chown -R peerbanhelper: /etc/peerbanhelper | ||
| mv /etc/peerbanhelper/config/config.yml /etc/peerbanhelper/config/profile.yml /etc/peerbanhelper | ||
| rm -rf /etc/peerbanhelper/config /etc/peerbanhelper/data | ||
| mv /etc/peerbanhelper/logs /var/log/peerbanhelper | ||
| find /etc/peerbanhelper -mindepth 1 -maxdepth 1 -type d -exec mv {} /var/lib/peerbanhelper \; |
There was a problem hiding this comment.
Add safety checks and backups for file operations
The file operations lack error handling and backups. This could lead to data loss or incomplete migration.
+# Create backup
+BACKUP_DIR="/etc/peerbanhelper.backup.$(date +%Y%m%d_%H%M%S)"
+if ! cp -a /etc/peerbanhelper "$BACKUP_DIR"; then
+ echo "Error: Failed to create backup" >&2
+ exit 1
+fi
+# Ensure target directories exist
+mkdir -p /var/log/peerbanhelper /var/lib/peerbanhelper
# Change ownership
-chown -R peerbanhelper: /etc/peerbanhelper
+if ! chown -R peerbanhelper: /etc/peerbanhelper; then
+ echo "Error: Failed to change ownership" >&2
+ exit 1
+fi
# Move configuration files
-mv /etc/peerbanhelper/config/config.yml /etc/peerbanhelper/config/profile.yml /etc/peerbanhelper
+for file in config.yml profile.yml; do
+ if [ -f "/etc/peerbanhelper/config/$file" ]; then
+ if ! mv "/etc/peerbanhelper/config/$file" /etc/peerbanhelper/; then
+ echo "Error: Failed to move $file" >&2
+ exit 1
+ fi
+ fi
+done
# Cleanup old directories
-rm -rf /etc/peerbanhelper/config /etc/peerbanhelper/data
-mv /etc/peerbanhelper/logs /var/log/peerbanhelper
+if [ -d /etc/peerbanhelper/logs ]; then
+ if ! mv /etc/peerbanhelper/logs/* /var/log/peerbanhelper/; then
+ echo "Error: Failed to move logs" >&2
+ exit 1
+ fi
+fi
+
+# Remove old directories only if they exist
+for dir in config data logs; do
+ if [ -d "/etc/peerbanhelper/$dir" ]; then
+ rm -rf "/etc/peerbanhelper/$dir"
+ fi
+done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| chown -R peerbanhelper: /etc/peerbanhelper | |
| mv /etc/peerbanhelper/config/config.yml /etc/peerbanhelper/config/profile.yml /etc/peerbanhelper | |
| rm -rf /etc/peerbanhelper/config /etc/peerbanhelper/data | |
| mv /etc/peerbanhelper/logs /var/log/peerbanhelper | |
| find /etc/peerbanhelper -mindepth 1 -maxdepth 1 -type d -exec mv {} /var/lib/peerbanhelper \; | |
| # Create backup | |
| BACKUP_DIR="/etc/peerbanhelper.backup.$(date +%Y%m%d_%H%M%S)" | |
| if ! cp -a /etc/peerbanhelper "$BACKUP_DIR"; then | |
| echo "Error: Failed to create backup" >&2 | |
| exit 1 | |
| fi | |
| # Ensure target directories exist | |
| mkdir -p /var/log/peerbanhelper /var/lib/peerbanhelper | |
| # Change ownership | |
| if ! chown -R peerbanhelper: /etc/peerbanhelper; then | |
| echo "Error: Failed to change ownership" >&2 | |
| exit 1 | |
| fi | |
| # Move configuration files | |
| for file in config.yml profile.yml; do | |
| if [ -f "/etc/peerbanhelper/config/$file" ]; then | |
| if ! mv "/etc/peerbanhelper/config/$file" /etc/peerbanhelper/; then | |
| echo "Error: Failed to move $file" >&2 | |
| exit 1 | |
| fi | |
| fi | |
| done | |
| # Cleanup old directories | |
| if [ -d /etc/peerbanhelper/logs ]; then | |
| if ! mv /etc/peerbanhelper/logs/* /var/log/peerbanhelper/; then | |
| echo "Error: Failed to move logs" >&2 | |
| exit 1 | |
| fi | |
| fi | |
| # Remove old directories only if they exist | |
| for dir in config data logs; do | |
| if [ -d "/etc/peerbanhelper/$dir" ]; then | |
| rm -rf "/etc/peerbanhelper/$dir" | |
| fi | |
| done |
| [Service] | ||
| User=peerbanhelper | ||
| WorkingDirectory=/etc/peerbanhelper | ||
| ExecStart=/usr/bin/java -Dpbh.release=debian -Dpbh.datadir=/etc/peerbanhelper -Xmx512M -XX:+UseG1GC -XX:+UseStringDeduplication -XX:+ShrinkHeapInSteps -jar /usr/lib/peerbanhelper/PeerBanHelper.jar | ||
| WorkingDirectory=/usr/lib/peerbanhelper |
There was a problem hiding this comment.
I think the wd should be the $HOME of user peerbanhelper, which means /var/lib/peerbanhelper. Anyway, it doesn't matter
|
Thanks for your effort! |
|
It seems the ci failed on the |
|
I'm not sure it's possible to change file permissions from GitHub's Web UI. Can you do that yourself? |
Continuation of #698.
Summary by CodeRabbit
New Features
peerbanhelper, enhancing organization.postrmscript for improved package removal processes.prermscript to manage service stopping before package removal.Improvements
Bug Fixes
Documentation
profile.ymlconfiguration file.