Download OS installers and Kiwix apps - #126
Conversation
|
""" WalkthroughThis update introduces a refactor to the offline data retrieval process and enhances VM creation scripts for OpenWRT and Ubuntu Server. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TaskRunner
participant LocalEnv
participant ProxmoxSSH
User->>TaskRunner: Run get-offline-data
TaskRunner->>LocalEnv: Run get-offline-data-local (dotenv, scripts)
TaskRunner->>ProxmoxSSH: Run get-offline-data-proxmox (SSH, scripts)
sequenceDiagram
participant User
participant VMCreateScript
User->>VMCreateScript: Run script with [--download-only]
VMCreateScript->>VMCreateScript: Download installer image
alt --download-only flag set
VMCreateScript-->>User: Exit after download
else
VMCreateScript->>VMCreateScript: Create VM using downloaded image
VMCreateScript-->>User: VM created successfully
end
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
proxmox/create-ubuntu-server-vm.sh (1)
112-124: Harden argument parsing
Usingfor arg in "$@"combined withshiftcan lead to subtle edge-cases if you ever pass multiple parameters. Consider switching to awhile [[ $# -gt 0 ]]loop and handling$1directly, for example:- for arg in "$@"; do - case $arg in - --download-only) - DOWNLOAD_ONLY=true - shift - ;; - *) - echo "Unknown argument: ${arg}" - exit 1 - ;; - esac - done + while [[ $# -gt 0 ]]; do + case "$1" in + --download-only) + DOWNLOAD_ONLY=true + ;; + *) + echo "Unknown argument: $1" >&2 + exit 1 + ;; + esac + shift + doneThis pattern is more idiomatic and guarantees each argument is consumed exactly once.
proxmox/create-openwrt-vm.sh (2)
18-28: Ensure working directory clarity
Currently the OpenWRT image lands in the script’s CWD (root’s home by default). To make artifact paths predictable, consider parameterizing orcd-ing into a designated directory (e.g.,/var/lib/vz/template/iso) and verifying it exists before download.
59-71: Harden argument parsing
As in the Ubuntu script, swappingfor arg in "$@"for awhile [[ $# -gt 0 ]]loop that shifts$1makes flag handling more predictable:while [[ $# -gt 0 ]]; do case "$1" in --download-only) DOWNLOAD_ONLY=true ;; *) echo "Unknown argument: $1" >&2; exit 1 ;; esac shift doneTaskfile.yaml (1)
142-151: Remote Proxmox offline data retrieval
Leveraging SSH+--download-onlyto pull ISOs/images is an elegant reuse of your VM scripts. TheTODOcomments for additional ISOs should be tracked—would you like help creating corresponding issues or backlog items?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Taskfile.yaml(2 hunks)proxmox/create-openwrt-vm.sh(1 hunks)proxmox/create-ubuntu-server-vm.sh(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
proxmox/create-ubuntu-server-vm.sh (1)
proxmox/create-openwrt-vm.sh (2)
download_installer(18-28)create_vm(30-57)
proxmox/create-openwrt-vm.sh (1)
proxmox/create-ubuntu-server-vm.sh (2)
download_installer(78-86)create_vm(88-109)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (8)
proxmox/create-ubuntu-server-vm.sh (3)
30-31: Introduce download-only flag
TheDOWNLOAD_ONLYflag defaults tofalse, enabling downstream logic to skip VM creation when desired. This cleanly ties into your Taskfile’s--download-onlyworkflow.
107-108: Echo moved insidecreate_vm
Logging success immediately after theqm createand configuration steps provides clear feedback that VM provisioning has completed.
127-133: Conditional VM creation in main logic
The flow—download first, exit early on--download-only, otherwise callcreate_vm—is clear and fault-tolerant. It cleanly separates download from provisioning.proxmox/create-openwrt-vm.sh (3)
15-17: Introduce download-only flag
AddingDOWNLOAD_ONLY=falsebrings parity with the Ubuntu script and enables offline-data tasks to fetch ISOs/images without spinning up VMs.
30-57: VM creation logic is solid
The steps for copying, unpacking, resizing, importing, and cleaning up the disk image are correctly ordered and comprehensive. Usingrealpathfor the import source is a nice touch.
74-80: Conditional VM creation in main logic
Exiting immediately after download whenDOWNLOAD_ONLYis set ensures no unintended VMs are provisioned—exactly the desired behavior for offline data tasks.Taskfile.yaml (2)
125-130: Splitget-offline-datainto local and remote subtasks
Orchestratingget-offline-data-localandget-offline-data-proxmoxsequentially clarifies responsibilities and leverages the new download-only flags. This improves task readability and maintainability.
131-141: Local offline data task
Theget-offline-data-localtask cleanly loads the proper dotenv files and invokes the ollama and kiwix scripts. Make sure variables like$STORAGE_KIWIXare documented in your.envfiles and validated before use.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/get-offline-data-file.sh (4)
4-9: Refine argument validation messaging
- Update the comment to mention all three parameters (directory, URL, pattern).
- Redirect error messages to stderr using
echo >&2.
Suggested diff:-# Validate that both URL and pattern are provided as parameters +# Validate that target directory, URL, and pattern are provided if [[ $# -ne 3 ]]; then - echo "Error: You must provide a target directory, a URL, and a pattern as parameters." + echo >&2 "Error: You must provide a target directory, a URL, and a pattern." echo "Usage: $0 <directory-path> <url> <pattern>" exit 1 fi
16-21: Improve directory existence check feedback
Redirect the error message to stderr for clarity:-if [[ ! -d "$TARGET_DIR" ]]; then - echo "Error: The specified directory '$TARGET_DIR' does not exist." - exit 1 -fi +if [[ ! -d "$TARGET_DIR" ]]; then + echo >&2 "Error: The specified directory '$TARGET_DIR' does not exist." + exit 1 +fi
40-44: Consider grep portability and regex robustness
Relying ongrep -oP(PCRE lookbehind) may not work on all systems. As an alternative, you could use a portablegrep -Eandsedpipeline:FILES=$(echo "$HTML" \ | grep -Eo 'href="[^"]*'"$pattern" \ | sed -E 's|href="([^"]+)".*|\1|' \ | sort -r)Or detect PCRE support at runtime and fallback.
42-50: Use version-aware sorting for filenames
A lexicographicalsort -rmay misorder versioned filenames. Usesort -Vrfor natural version sorting:-FILES=$(echo "$HTML" | grep -oP "(?<=href=\")${pattern}" | sort -r) +FILES=$(echo "$HTML" | grep -oP "(?<=href=\")${pattern}" | sort -Vr)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Taskfile.yaml(1 hunks)scripts/get-offline-data-file.sh(1 hunks)scripts/get-offline-data-kiwix.sh(0 hunks)
💤 Files with no reviewable changes (1)
- scripts/get-offline-data-kiwix.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- Taskfile.yaml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (6)
scripts/get-offline-data-file.sh (6)
1-2: Good use of strict mode
The shebang andset -euo pipefailensure reliable error handling and portability.
11-15: Parameter assignment is clear
Capturing positional arguments into named variables improves readability and maintainability.
22-25: Approve directory change
Changing into the target directory early ensures subsequent operations run in the correct context.
56-63: File existence check is solid
Skipping download when the latest file already exists prevents unnecessary network traffic.
65-73: Downloading logic is correct
Usingwget --show-progressfor visibility is helpful. Error handling on download failure is properly implemented.
75-77: Invoke download function and propagate exit code
Withset -e, the script will exit ifdownload_latest_filereturns a non-zero status, aligning with intended behavior.
ec739ac to
23baab5
Compare
Summary by CodeRabbit
New Features
Improvements