Skip to content

Fix: add crontab to acme.sh install dependency checks #17

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.

Sign up for GitHub

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

Open
W0lfD wants to merge 2 commits into
base: main
Choose a base branch
from
+22 −2
Open

Fix: add crontab to acme.sh install dependency checks #17

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2aed4f9
ensure crontab for acme
W0lfD Apr 15, 2026
2a1a5e1
check acme.sh after installation
W0lfD Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions pasarguard.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,16 +321,36 @@ is_port_in_use() {
return 1
}

get_cron_package_name() {
if [[ "$OS" == "Ubuntu"* ]] || [[ "$OS" == "Debian"* ]]; then
echo "cron"
elif [[ "$OS" == "CentOS"* ]] || [[ "$OS" == "AlmaLinux"* ]] || [ "$OS" == "Fedora"* ] || [ "$OS" == "Arch Linux" ]; then
echo "cronie"
else
return 1
fi
}
Comment on lines +324 to +332
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Glob pattern won't work with single brackets; also missing openSUSE support.

Line 327: [ "$OS" == "Fedora"* ] uses single brackets which don't support glob matching. This condition will never match Fedora variants. The static analysis tool (Shellcheck SC2081) also flags this.

Additionally, openSUSE is supported in install_package() (line 207-209) but is not handled here, which will cause get_cron_package_name to return failure on openSUSE systems.

🐛 Proposed fix 
 get_cron_package_name() {
     if [[ "$OS" == "Ubuntu"* ]] || [[ "$OS" == "Debian"* ]]; then
         echo "cron"
-    elif [[ "$OS" == "CentOS"* ]] || [[ "$OS" == "AlmaLinux"* ]] || [ "$OS" == "Fedora"* ] || [ "$OS" == "Arch Linux" ]; then
+    elif [[ "$OS" == "CentOS"* ]] || [[ "$OS" == "AlmaLinux"* ]] || [[ "$OS" == "Fedora"* ]] || [[ "$OS" == "Arch Linux" ]]; then
         echo "cronie"
+    elif [[ "$OS" == "openSUSE"* ]]; then
+        echo "cronie"
     else
         return 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.

Suggested change
get_cron_package_name() {
if [[ "$OS" == "Ubuntu"* ]] || [[ "$OS" == "Debian"* ]]; then
echo "cron"
elif [[ "$OS" == "CentOS"* ]] || [[ "$OS" == "AlmaLinux"* ]] || [ "$OS" == "Fedora"* ] || [ "$OS" == "Arch Linux" ]; then
echo "cronie"
else
return 1
fi
}
get_cron_package_name() {
if [[ "$OS" == "Ubuntu"* ]] || [[ "$OS" == "Debian"* ]]; then
echo "cron"
elif [[ "$OS" == "CentOS"* ]] || [[ "$OS" == "AlmaLinux"* ]] || [[ "$OS" == "Fedora"* ]] || [[ "$OS" == "Arch Linux" ]]; then
echo "cronie"
elif [[ "$OS" == "openSUSE"* ]]; then
echo "cronie"
else
return 1
fi
}
🧰 Tools
🪛 Shellcheck (0.11.0)

[error] 327-327: [ .. ] can't match globs. Use [[ .. ]] or case statement.

(SC2081)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pasarguard.sh` around lines 324 - 332, get_cron_package_name currently mixes
single-bracket tests (which don't support globbing) and omits openSUSE; change
the conditional to use double-bracket tests for all OS pattern matches (e.g., [[
"$OS" == "Fedora"* ]]) so the glob suffix works, and add openSUSE to the branch
that returns "cronie" so it matches install_package's supported distro list;
update the elif to include CentOS, AlmaLinux, Fedora, openSUSE, and Arch Linux
using [[ ... ]] pattern matching and keep the else returning failure.


ensure_acme_dependencies() {
command -v socat >/dev/null 2>&1 || install_package socat
command -v openssl >/dev/null 2>&1 || install_package openssl

local cron_pkg
if ! command -v crontab >/dev/null 2>&1; then
cron_pkg="$(get_cron_package_name)"
install_package "$cron_pkg"
fi
Comment on lines +337 to +342
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Missing error handling when get_cron_package_name fails.

If get_cron_package_name returns failure (e.g., on an unsupported OS), cron_pkg will be empty, and install_package "" will be called with an empty argument, potentially causing unexpected behavior or confusing error messages.

🛡️ Proposed fix to add error handling 
     local cron_pkg
     if ! command -v crontab >/dev/null 2>&1; then
-        cron_pkg="$(get_cron_package_name)"
-        install_package "$cron_pkg"
+        if cron_pkg="$(get_cron_package_name)"; then
+            install_package "$cron_pkg"
+        else
+            colorized_echo yellow "Warning: Could not determine cron package for this OS. Please install crontab manually."
+        fi
     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.

Suggested change
local cron_pkg
if ! command -v crontab >/dev/null 2>&1; then
cron_pkg="$(get_cron_package_name)"
install_package "$cron_pkg"
fi
local cron_pkg
if ! command -v crontab >/dev/null 2>&1; then
if cron_pkg="$(get_cron_package_name)"; then
install_package "$cron_pkg"
else
colorized_echo yellow "Warning: Could not determine cron package for this OS. Please install crontab manually."
fi
fi
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pasarguard.sh` around lines 337 - 342, When calling get_cron_package_name
before install_package, check whether it succeeded and returned a non-empty
value (cron_pkg) and handle the failure path; if get_cron_package_name fails or
cron_pkg is empty, log an error and exit (or return a non-zero status) instead
of calling install_package with an empty argument. Update the block around
cron_pkg, get_cron_package_name and install_package to validate cron_pkg (and
propagate errors from get_cron_package_name) so install_package is only invoked
with a valid package name.

}

install_acme() {
colorized_echo blue "Installing acme.sh for SSL certificate management..."
if curl -s https://get.acme.sh | sh >/dev/null 2>&1; then
colorized_echo green "acme.sh installed successfully"
return 0
local acme_bin=""
acme_bin="$(get_acme_sh_binary)" || true
if [ -n "$acme_bin" ] && [ -x "$acme_bin" ]; then
colorized_echo green "acme.sh installed successfully"
return 0
fi
fi
colorized_echo red "Failed to install acme.sh"
return 1
Expand Down