feat(software): add OpenMediaVault installation module#751
Conversation
WalkthroughThe module_omv.sh script was updated to add i386 to supported architectures, rename the title to "OpenMediaVault", and introduce public constants and a cleanup helper for OMV installation. It adds pre-installation checks (existing OMV, desktop environments, container runtimes), validates architecture and Debian codenames, saves/restores resolv.conf, forces IPv4 and disables translations for apt, fetches/configures the OMV GPG key and APT source, installs prerequisites and OMV with guarded dpkg options, verifies installation, optionally runs omv-confdbadm populate, and expands the remove path to stop services and remove repo/GPG artifacts. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
bot
added
02
github-actions
bot
added
size/medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@tools/modules/software/module_omv.sh`:
- Around line 59-64: The architecture validation accepts i386 via the regex in
the arch variable check (arch="$(dpkg --print-architecture)" and if [[ ! ${arch}
=~ ^(amd64|arm64|armhf|i386)$ ]]) but the module metadata key
["module_omv,arch"] only lists "amd64 arm64 armhf"; make them consistent by
either adding "i386" to the metadata value for ["module_omv,arch"] or removing
"i386" from the regex in the validation so both the metadata and the arch check
agree.
- Around line 40-45: The current desktop detection uses "dpkg -l | grep -Eqw
..." which can match removed/config-file-only packages and descriptions; update
the check to only consider actually installed packages by first filtering dpkg
output for the "ii" installed state (or use dpkg-query -W
-f='${Status}\t${Package}\n') and then grep for the desktop package names
(gdm3|sddm|lxdm|xdm|lightdm|slim|wdm); modify the block that contains dpkg -l
and grep -Eqw so it filters for installed packages before matching and keep the
same error messages/return 1 behavior.
- Around line 118-124: The prerequisite package installs currently run without
checking failures; wrap the two pkg_install invocations (the one installing
"postfix wget gnupg" and the one installing "openmediavault-keyring") with the
same error-guard pattern used for the main OMV install: check the exit status of
pkg_install (or use set -e and conditional logging) and on failure log an error
via the same logger (or echo) and exit non-zero to stop further installation;
also remove the redundant --yes flag since pkg_install already supplies -y.
Ensure you reference the exact pkg_install calls in the module_omv.sh script
when adding these guards.
- Around line 86-97: The install creates /etc/apt/apt.conf.d/99no-translations
but error paths only remove "${forceIpv4}", leaving the APT config behind; add a
cleanup helper (e.g. _omv_install_cleanup) that removes both "${forceIpv4}" and
/etc/apt/apt.conf.d/99no-translations, define it in the same script, and replace
every error-path call of rm -f "${forceIpv4}" (the branches after pkg_update and
other returns) with a call to _omv_install_cleanup before returning to ensure
the temporary APT configs are always removed.
🧹 Nitpick comments (5)
tools/modules/software/module_omv.sh (5)
53-57: LXC detection via/proc/1/cgroupis unreliable on cgroup v2 (Debian 12+ default).On cgroup v2 systems,
/proc/1/cgrouptypically contains only0::/and won't matchmachine-lxc. Consider usingsystemd-detect-virtwhich works reliably on both cgroup v1 and v2:Proposed fix
# Check for LXC - if grep -q 'machine-lxc' /proc/1/cgroup 2>/dev/null; then + if systemd-detect-virt -c -q 2>/dev/null; then echo "ERROR: LXC detected. OMV does not work in LXC!" return 1 fiNote:
systemd-detect-virt -c -qdetects any container environment (LXC, Docker, systemd-nspawn, etc.), which would also let you consolidate the Docker check above. If you want to keep them separate for distinct error messages, usesystemd-detect-virt -cand check the output string.
100-105:curlfailure in the pipeline may not be caught.In
curl ... | gpg --dearmor -o, Bash reports only the last command's exit code by default. Ifcurlfails butgpgexits 0 (e.g., produces an empty file from no input on some versions), the error goes undetected. Consider downloading to a temp file first, then converting:Proposed fix
echo "Adding GPG key for OpenMediaVault..." - if ! curl --max-time 60 -4 -fsSL "${omvKeyUrl}" | \ - gpg --dearmor -o "${omvKey}"; then + local keyTmp + keyTmp="$(mktemp)" + if ! curl --max-time 60 -4 -fsSL "${omvKeyUrl}" -o "${keyTmp}"; then + echo "ERROR: Failed to download GPG key." + rm -f "${keyTmp}" + _omv_install_cleanup + return 1 + fi + if ! gpg --dearmor -o "${omvKey}" < "${keyTmp}"; then echo "ERROR: Failed to add GPG key." - rm -f "${forceIpv4}" + rm -f "${keyTmp}" + _omv_install_cleanup return 1 fi + rm -f "${keyTmp}"
145-153:pingtests ICMP reachability, not just DNS resolution.The comment says "DNS validation" but
pingwill also fail if the host blocks ICMP, triggering an unnecessaryresolv.confrestore. For a pure DNS check, prefer:- if ! ping -4 -q -c2 -W2 omv-extras.org >/dev/null 2>&1; then + if ! getent hosts omv-extras.org >/dev/null 2>&1; then
getent hostsperforms only name resolution without requiring ICMP.
155-166:omv-confdbadm populatefailure is silently ignored.If
omv-confdbadm populatefails (line 161), the user sees a success message but OMV may be in a broken state. Consider at least logging a warning:Proposed fix
if command -v omv-confdbadm >/dev/null 2>&1; then echo "Initializing OMV configuration..." - omv-confdbadm populate + if ! omv-confdbadm populate; then + echo "WARNING: OMV configuration initialization failed. You may need to run 'omv-confdbadm populate' manually." + fi fi
168-201: Remove path looks solid — good service-stop-before-removal pattern.One minor gap: if
pkg_remove openmediavaultfails (line 182), the function still reports success at line 200. Consider checking the return code:Proposed fix
# Remove the package - DEBIAN_FRONTEND=noninteractive pkg_remove openmediavault + if ! DEBIAN_FRONTEND=noninteractive pkg_remove openmediavault; then + echo "WARNING: Package removal may have been incomplete." + fi
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@tools/modules/software/module_omv.sh`:
- Around line 106-111: The gpg --dearmor step can fail if the target file
${omvKey} already exists; update the block around omvKeyUrl and omvKey (used by
the curl | gpg pipeline) to ensure gpg will overwrite or the file is removed
before writing: either add the --yes option to gpg --dearmor (e.g., gpg
--dearmor --yes -o "${omvKey}") or explicitly unlink/rm "${omvKey}" if present
before running curl | gpg, keeping the same error handling that calls
_omv_install_cleanup and returns 1 on failure.
- Around line 164-172: The DNS check currently uses ping (ICMP) which can be
blocked; replace the ping-based test with a DNS-only lookup using getent hosts
(e.g., run getent hosts omv-extras.org and check its exit status) so it
exercises NSS/resolv.conf rather than ICMP; keep the same warning/restore logic
that checks the ${resolvTmp} file and copies it back to /etc/resolv.conf when
the lookup fails, and update the echo messages to reflect a DNS lookup failure
instead of ICMP/ping.
- Around line 200-208: The script calls DEBIAN_FRONTEND=noninteractive
pkg_remove openmediavault but does not check its exit status before continuing
to remove the OMV repo and GPG key; update the removal sequence so that after
invoking pkg_remove (symbol: pkg_remove) you check its exit code ($?) and if
non-zero abort/exit with a non-zero status and an error log (e.g., via echo or
logger) instead of proceeding to remove omvSources and the key; only perform the
repository/file cleanup (symbols: omvSources, the GPG key removal block) when
pkg_remove succeeds, ensuring you surface the original failure message to aid
recovery.
- Around line 59-63: Replace the brittle grep check against /proc/1/cgroup with
a call to systemd-detect-virt --container: detect_container=$(command -v
systemd-detect-virt >/dev/null && systemd-detect-virt --container || true); if
detect_container == "lxc" then echo error and return 1; if detect_container ==
"docker" handle per the existing Docker logic (merge your current Docker check
into this branch). Also keep a fallback: if systemd-detect-virt is unavailable,
preserve the original grep 'machine-lxc' test. Update the script so the new
logic replaces the grep block and merges Docker detection into a single
container-detection block.
🧹 Nitpick comments (1)
tools/modules/software/module_omv.sh (1)
31-34: Cleanup helper doesn't cover all artifacts, leading to repeated inline cleanup.After the repo and GPG key are created, every error path calls
_omv_install_cleanupandrm -f "${omvSources}" "${omvKey}"(lines 121, 130, 139, 152, 160). This repetition is error-prone — a future error path could easily forget the extrarm. Consider making the helper idempotently remove all install artifacts:Proposed fix
_omv_install_cleanup() { - rm -f "${forceIpv4}" "${noTrans}" + rm -f "${forceIpv4}" "${noTrans}" "${omvSources}" "${omvKey}" "${resolvTmp}" }Then replace all the compound cleanup sequences (e.g., lines 120-121) with a single
_omv_install_cleanupcall.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tools/modules/software/module_omv.sh`:
- Around line 178-182: The script currently runs omv-confdbadm populate without
checking its exit code, so failures are silently ignored; update the block that
echoes "Initializing OMV configuration..." and calls omv-confdbadm populate to
capture its exit status and handle errors: after running omv-confdbadm populate
(the command referenced), if its exit code is non-zero print a clear
error/warning to stderr (or via process logger) including the command name and
exit status and then exit or return a non-zero status so the caller sees
failure; if it succeeds, continue as now. Ensure you change only the populate
invocation/error handling (the echo/omv-confdbadm populate sequence).
- Around line 184-186: The echo that prints the web UI URL should defensively
resolve a single valid IP instead of blindly using hostname -I; compute a
variable (e.g., web_ip) by taking the first token from hostname -I, fall back to
hostname -i or an ip utility if empty, and finally default to "localhost" when
no address is found, then use that variable in the echo that references ${title}
(the two lines echo "=== ${title} installation..." and the URL). Preserve the
existing return 0 behavior after printing.
igorpecovnik
added
Work in progress
1 participant
Summary
Changes to OMV Module (
tools/modules/software/module_omv.sh)Pre-installation Checks
Installation Improvements
Removal Enhancements
Status Command
Test Plan