Refine workflow #106
Refine workflow #106
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
staging/validate_staged_modules.sh (1)
57-121: Expand helpers list parsing and localize variables; align with earlier preference for comprehensive validation.
- helpers may be comma- or space-delimited; current regex only accounts for commas.
- Localize variables to avoid side effects.
This keeps the fail-fast posture you wanted for module configs.
_check_conf() { # Check for required fields in <modulename>.conf - local REQUIRED_CONF_FIELDS=(feature options helpers description parent group contributor port) - local file="$1" - local failed=0 - local failed_fields=() + local REQUIRED_CONF_FIELDS=(feature options helpers description parent group contributor port) + local file="$1" + local failed=0 + local failed_fields=() … - # Check for feature= line - local feature - feature="$(grep -E "^feature=" "$file" | cut -d= -f2- | xargs)" + # feature value (first occurrence) + local feature + feature="$(grep -Em1 "^feature=" "$file" | cut -d= -f2- | xargs)" … - local value - value="$(grep -E "^$field=" "$file" | cut -d= -f2- | xargs)" + local value + value="$(grep -Em1 "^$field=" "$file" | cut -d= -f2- | xargs)" … - helpers) - # Check for _about_<feature> function in CSV or space-separated helpers list - if [[ -n $feature && ! $value =~ (^|,)_about_${feature}(,|$) ]]; then + helpers) + # Accept comma and/or whitespace delimiters; require _about_<feature> + # shellcheck disable=SC2076 # intentional regex match in [[ =~ ]] + if [[ -n $feature && ! $value =~ (^|[[:space:],])_about_${feature}([[:space:],]|$) ]]; then failed=1 failed_fields+=("helpers must have at least (_about_$feature)") fi ;;If you’d prefer to avoid multiple grep passes and align with set -euo, I can rewrite this to read the .conf once and parse with awk.
🧹 Nitpick comments (35)
workflow/50_array_to_json.sh (4)
241-241: Disable copy: confirm no remaining consumers of modules_browsers/modules_metadata.jsonCommenting out this copy looks aligned with removing the legacy UI metadata path. Please verify that nothing (tests, CI jobs, docs, or local tooling) still reads from modules_browsers/modules_metadata.json. If there are downstream consumers, add a short-lived deprecation shim or update them in the same PR to avoid breakage. As a minor UX improvement, consider emitting an explicit notice instead of a silent comment.
Apply this diff to make the deprecation clear at runtime:
- #cp "$ROOT_DIR/lib/armbian-config/config.jobs.json" "$ROOT_DIR/modules_browsers/modules_metadata.json" + # Deprecated: modules_browsers/modules_metadata.json is no longer produced. + echo "[array_to_json] Skipping deprecated copy to modules_browsers/modules_metadata.json" >&2
240-240: Avoid double pretty-printing through jqarray_to_json already pretty-prints via jq (Line 205). Piping its output again into jq is redundant work. Trim one jq stage to speed up the workflow slightly and reduce process spawning.
- array_to_json | jq --indent 4 . > "$ROOT_DIR/lib/armbian-config/config.jobs.json" + array_to_json > "$ROOT_DIR/lib/armbian-config/config.jobs.json"
243-243: Harden copy to docs by ensuring target directory existsIf docs/ doesn’t exist (fresh clone or CI job), cp will fail. Create the directory first.
+ mkdir -p "$ROOT_DIR/docs" cp "$ROOT_DIR/src/module_browser.html" "$ROOT_DIR/docs/index.html"
1-10: Optional: add defensive bash options for safer failuresConsider adopting a stricter shell stance to catch errors early (only if you’re comfortable with the stricter behavior across all call paths):
#!/usr/bin/env bash +set -euo pipefail +IFS=$'\n\t'Note: set -u may require defaulting all possibly-unset vars (you already guard
$1 with $ {1:-}).staging/web_images.conf (3)
8-12: Placeholders are OK in staging; add content before promotion.Per your staging model, empty description/extend_desc are acceptable now. Before promotion, document scope (e.g., where images are sourced, naming rules, supported formats) so downstream tooling and docs generators remain coherent.
I can draft a first-pass description/extend_desc from the implementation in staging/web_images.sh if you’d like.
24-34: Pre-fill baseline platform metadata when ready.When this moves out of staging, align with the common defaults you’ve used elsewhere:
- contributor=@Tearran
- arch=all
- require_os=linux
- require_kernel=4.19+
-contributor= -arch= -require_os= -require_kernel= +contributor=@Tearran +arch=all +require_os=linux +require_kernel=4.19+
14-15: Keep staging configoptions=in sync with[options]entriesVerified via the provided script that the following staging config files have an empty top-level
options=declaration but non-empty[options]blocks, which can lead to drift when loaders enumerate available commands:
- staging/promote_staged_module.conf
- staging/validate_staged_modules.conf
- staging/web_images.conf
In each of these files, update the
options=line to list all option names (comma-separated) defined under its[options]section. For example, in staging/web_images.conf:-options= +options=help @@ [options] help=Show help for this moduleApply the same pattern for the other two files, replacing the empty
options=with the exact keys declared under their respective[options]blocks. This ensures consistency for any loader logic that relies on the top-leveloptions=field.staging/promote_staged_module.conf (2)
8-12: Staging placeholders acknowledged; document before promotion.Leave as-is for now. Add a clear description and extended description before moving to src/ so validators and autogenerated docs don’t produce empty sections.
Happy to propose text once the CLI surface is finalized.
14-15: Mirror help in the options key.Align
options=with the[options]section to avoid metadata consumers missing the declared commands.-options= +options=help @@ [options] help=Show help for this moduleAlso applies to: 45-46
staging/validate_staged_modules.conf (3)
8-12: Placeholders acceptable in staging; complete before promotion.Fill these descriptions to make the validator self-documenting and to support doc generation.
14-15: Synchronize options across metadata surfaces.As with other modules, declare
options=helpalongside the[options]entry.-options= +options=help @@ [options] help=Show help for this moduleAlso applies to: 45-46
38-41: Include required helpers in module metadataThe validator keys off the
helperslist and will fail if both the main and help functions aren’t declared.
- File: staging/validate_staged_modules.conf (lines 38–41): update the
helpersfield to reference the functions confirmed in staging/validate_staged_modules.sh.-helpers= +helpers=_validate_staged_modules_main,_about_validate_staged_modulesstaging/validate_staged_modules.sh (5)
17-24: Scope nullglob to this function and remove stray comment artifact.
- shopt is shell-global; enable it on entry and restore it before returning to avoid surprising downstream behavior.
- The stray commented token “#Sexit 1” looks like a typo from a prior edit.
Apply:
_validate_staged_modules_main() { -shopt -s nullglob # <--- Add this! + # limit globbing side-effects to this function + shopt -s nullglob local failed=0 local shfiles=(./staging/*.sh) if [ ${#shfiles[@]} -eq 0 ]; then echo "No modules found in ./staging/" - #Sexit 1 + # Intentionally non-fatal for an empty staging area fi … -} + # restore global shell option state + shopt -u nullglob +}
25-32: Declare modname local and quote consistently.modname is function-scoped. Declare it local to avoid leaking into the global namespace in scripts that source this file.
- for shfile in "${shfiles[@]}"; do - modname="$(basename "$shfile" .sh)" + for shfile in "${shfiles[@]}"; do + local modname + modname="$(basename "$shfile" .sh)" echo "==> Checking module: $modname" _check_sh "./staging/$modname.sh" || failed=1 _check_conf "./staging/$modname.conf" || failed=1 _check_duplicate_anywhere "$modname" || failed=1 echo done
41-55: Make vars local and accept both “function f() { … }” and “f() { … }” styles.Good intent. Minor polish: mark variables local for cleanliness. The grep already handles both styles; keep it.
_check_sh() { - file="$1" - modname="$(basename "$file" .sh)" + local file="$1" + local modname + modname="$(basename "$file" .sh)" if [ ! -f "$file" ]; then echo "MISSING: $file" return 1 fi # Check for _about_<modname>() function if ! grep -Eq "^(function[[:space:]]+)?_about_${modname}[[:space:]]*\(\)[[:space:]]*\{" "$file"; then echo "FAIL: $file missing _about_${modname}()" return 1 fi
136-158: Duplicate scan: fix unreachable branch and align code with comment (scan docs/ too).
- The else branch can’t be reached because find only returns existing files.
- The header says “src/ and docs/” but you only scan src/.
-# Check for duplicates in src/ and docs/ (excluding ./staging) +# Check for duplicates in src/ and docs/ (excluding ./staging) _check_duplicate_anywhere() { local modname="$1" - local found=0 - for dir in ./src ; do + local found=0 + for dir in ./src ./docs ; do for ext in .sh .conf; do # Find all matches, ignoring ./staging - while IFS= read -r file; do - # Skip if nothing found or file is in ./staging - [[ -z "$file" ]] && continue + while IFS= read -r file; do + # Skip files in ./staging [[ "$file" == ./staging/* ]] && continue - # FAIL if file exists outside staging - if [ -f "$file" ]; then - echo "FAIL: Duplicate found in $dir: $file" - found=1 - else - echo "OK: No duplicate found in $dir: $file" - fi + # Any hit from find is a duplicate + echo "FAIL: Duplicate found in $dir: $file" + found=1 done < <(find "$dir" -type f -name "$modname$ext") done done return $found }
160-186: Help mentions commands not implemented by the dispatcher.validate_staged_modules currently only supports “help” and the main validation path. Drop placeholder commands or implement them to avoid confusing UX.
Commands: - test - Run a basic test of the validate_staged_modules module - foo - Example 'foo' operation (replace with real command) - bar - Example 'bar' operation (replace with real command) help - Show this help messageAlternatively, extend validate_staged_modules() to route test/foo/bar.
staging/promote_staged_module.sh (4)
17-25: Enable nullglob and handle empty staging gracefully.Without nullglob, the literal pattern ./staging/*.sh is iterated once. Enable nullglob and provide a clear message when nothing is staged.
_promote_staged_module_main() { - for sh_file in ./staging/*.sh; do + shopt -s nullglob + local had_files=0 + for sh_file in ./staging/*.sh; do + had_files=1 [[ -f "$sh_file" ]] || continue base_name="$(basename "$sh_file" .sh)" conf_file="./staging/${base_name}.conf" if [[ -f "$conf_file" ]]; thenAnd at the end of the function:
- done + done + if [[ $had_files -eq 0 ]]; then + echo "No staged modules found in ./staging/" + fi
56-66: Orphan check is good; restore nullglob and consider allowing known non-module files.The fail-fast stance is fine. Since we enabled nullglob earlier, restore it here. If you expect README or .gitkeep in staging, whitelist them.
if [[ -d "./staging" ]]; then if [[ -z "$(ls -A ./staging)" ]]; then echo "Removing empty ./staging directory." rmdir ./staging else echo "ERROR: Orphaned files left in ./staging after promotion!" ls -l ./staging exit 1 fi fi + shopt -u nullglob
69-96: Help mentions commands not implemented by the dispatcher.As-is, only “help” and the default promotion path are available. Trim placeholders or add routing for test/foo/bar.
Commands: - test - Run a basic test of the promote_staged_module module - foo - Example 'foo' operation (replace with real command) - bar - Example 'bar' operation (replace with real command) help - Show this help message
100-110: Self-test can trigger real moves; limit to help assertion to avoid side effects.Executing the script directly with no args will run promotion. That’s risky for ad-hoc invocations. Exit after the help assertion in the test harness.
help_output="$(promote_staged_module help)" echo "$help_output" | grep -q "Usage: promote_staged_module" || { echo "fail: Help output does not contain expected usage string" echo "test complete" exit 1 } - # --- end assertion --- - promote_staged_module "$@" + # --- end assertion --- + exit 0staging/web_images.sh (4)
46-59: Minor: scope shopt and keep it balanced.Enable nullglob just for the SVG scan and then disable it. You already disable it later; keeping it close improves readability.
-# Check if SVGs exist in the source directory -shopt -s nullglob -svg_files=("$SRC_DIR"/*.svg) + # Check if SVGs exist in the source directory + shopt -s nullglob + local -a svg_files=("$SRC_DIR"/*.svg) if [ ${#svg_files[@]} -eq 0 ]; then echo "Error: No SVG files found in '$SRC_DIR'." exit 1 fi -shopt -u nullglob + shopt -u nullglob
60-79: Use local vars and the resolved convert binary; improve error handling.-for svg in "${svg_files[@]}"; do + for svg in "${svg_files[@]}"; do # Extract the base filename without extension - base=$(basename "$svg" .svg) + local base + base=$(basename "$svg" .svg) # For each size, generate the PNG in the corresponding directory - for size in "${SIZES[@]}"; do - OUT_DIR="share/icons/hicolor/${size}x${size}" - mkdir -p "$OUT_DIR" - OUT_FILE="${OUT_DIR}/${base}.png" + for size in "${SIZES[@]}"; do + local OUT_DIR="share/icons/hicolor/${size}x${size}" + local OUT_FILE="${OUT_DIR}/${base}.png" + mkdir -p "$OUT_DIR" # Only generate if missing or source SVG is newer if [[ ! -f "$OUT_FILE" || "$svg" -nt "$OUT_FILE" ]]; then - convert -background none -resize ${size}x${size} "$svg" "$OUT_FILE" - if [ $? -eq 0 ]; then + # shellcheck disable=SC2086 # allow split for CONVERT_BIN + $CONVERT_BIN -background none -resize "${size}x${size}" "$svg" "$OUT_FILE" + if [ $? -eq 0 ]; then echo "Generated $OUT_FILE" else echo "Failed to convert $svg to $OUT_FILE" fi fi done done
6-15: Dispatcher only supports help/main; align help text.Same as other modules in this PR, the help advertises test/foo/bar but the router doesn’t implement them.
case "${1:-}" in help|-h|--help) _about_web_images ;; *) _web_images_main ;; esacAnd trim the placeholder commands from the help block (see separate diff below).
83-110: Trim placeholder commands from help to match current behavior.Commands: - test - Run a basic test of the web_images module - foo - Example 'foo' operation (replace with real command) - bar - Example 'bar' operation (replace with real command) help - Show this help message … Notes: - - Replace 'foo' and 'bar' with real commands for your module. - - All commands should accept '--help', '-h', or 'help' for details, if implemented. + - All commands should accept '--help', '-h', or 'help' for details, if implemented. - Intended for use with the config-v2 menu and scripting. - Keep this help message up to date if commands change.docs/README.md (2)
27-29: Replace hard tabs with spaces to satisfy markdownlint (MD010).The two lines flagged by markdownlint use hard tabs. Convert to spaces for consistency.
- - [webmin](./webmin.md) — Webmin setup and service control. - - [plexmediaserver](./plexmediaserver.md) — SDK/development tool to install, remove, or check the status of Plex Media Server via the official repository. Not intended for production use unless otherwise secured and configured. + - [webmin](./webmin.md) — Webmin setup and service control. + - [plexmediaserver](./plexmediaserver.md) — SDK/development tool to install, remove, or check the status of Plex Media Server via the official repository. Not intended for production use unless otherwise secured and configured.
5-5: Minor punctuation nit: consistency with other entries.Consider adding a trailing period to match tone of other descriptions, or remove from all for consistency. Current mix is acceptable; leaving as optional.
docs/webmin.md (2)
4-6: Tighten wording for clarity.Add commas for readability.
-Provides installation removal service management and status checks for the Webmin web-based system administration tool. Supports enabling/disabling on boot and verifying service state. +Provides installation, removal, service management, and status checks for the Webmin web-based system administration tool. Supports enabling/disabling on boot and verifying service state.
37-39: List formatting: add commas for parallel items.- - Supported Architectures: x86-64 arm64 armhf + - Supported Architectures: x86-64, arm64, armhfdocs/initialize_env.md (6)
1-2: Tighten intro and fix project name drift ("config-v2" vs "configng-v2").Line 2 likely intends to reference this repository. If the canonical name is "configng-v2", update to prevent confusion. Also add terminal punctuation.
Apply this diff:
-Detect and expose common environment variables for other config-v2 modules +Detect and expose common environment variables for other configng-v2 modules.
5-5: Use code spans for variables/paths and clarify opt-in flag.Wrap identifiers and paths in backticks for readability and consistency. This also highlights the opt-in flag.
Apply this diff:
-Provides helpers to detect system and project paths (BIN_ROOT, LIB_ROOT, WEB_ROOT, etc.), OS/release metadata (sourced from /etc/os-release and /etc/armbian-release), and runtime info (kernel, default network adapter, local IP/subnet). Intended to be sourced by callers; includes helpers to print or emit export lines suitable for eval. Keep side effects opt-in (INITIALIZE_AUTO) to avoid surprising behavior on source. +Provides helpers to detect system and project paths (`BIN_ROOT`, `LIB_ROOT`, `WEB_ROOT`, etc.), OS/release metadata (sourced from `/etc/os-release` and `/etc/armbian-release`), and runtime info (kernel, default network adapter, local IP/subnet). Intended to be sourced by callers; includes helpers to print or emit export lines suitable for `eval`. Keep side effects opt-in via `INITIALIZE_AUTO` to avoid surprising behavior on source.
8-20: Replace hard tabs with spaces in fenced block (markdownlint MD010).Tabs on Lines 12, 15, 18, and 19 trigger MD010. Convert to spaces for compliance and consistent rendering.
Apply this diff:
~~~bash Usage: initialize_env [help|-h|--help] About: - The 'initialize_env' module provides environment detection utilities. + The 'initialize_env' module provides environment detection utilities. Commands: - help - Show this help message. + help - Show this help message. Notes: - - To set variables in the current shell: source this file and call _initialize_env_vars - - When executed, this file's test entrypoint verifies the environment and prints variables. + - To set variables in the current shell: source this file and call _initialize_env_vars + - When executed, this file's test entrypoint verifies the environment and prints variables. ~~~
8-20: Consider adding concrete usage examples to reduce ambiguity.Since the module is intended to be sourced and has an opt-in flag, explicit examples help users avoid subshell mistakes.
Proposed snippet to add after the "Notes:" section in the same fenced block:
+Examples: + # Source and initialize in current shell + source ./src/core/initialize/initialize_env.sh + _initialize_env_vars + echo "$BIN_ROOT" + + # One-shot: print export lines and eval them (no side effects unless `INITIALIZE_AUTO=1`) + eval "$( + INITIALIZE_AUTO=0 ./src/core/initialize/initialize_env.sh --print-exports + )"
8-8: Standardize code fences (~~~ vs ```).Tildes are valid, but if the docs elsewhere use triple backticks, prefer consistency to reduce mixed styles.
Optional:
-~~~bash +```bash ... -~~~ +```
22-22: Strengthen the “autogenerated” notice to prevent drift.If this page is generated, add a clear “Do not edit” line and point to the generation command.
Apply this diff:
-- Autogenerated from `./src/core/initialize/initialize_env.conf` and `./src/core/initialize/initialize_env.sh` +- Autogenerated from `./src/core/initialize/initialize_env.conf` and `./src/core/initialize/initialize_env.sh`. +- Do not edit this file directly; regenerate via the docs build pipeline (e.g., `make docs` or `./staging/validate_staged_modules.sh --generate-docs`).If no standard command exists, I can help wire one up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
docs/README.md(2 hunks)docs/initialize_env.md(1 hunks)docs/modules_metadata.json(0 hunks)docs/plexmediaserver.md(1 hunks)docs/webmin.md(1 hunks)staging/promote_staged_module.conf(1 hunks)staging/promote_staged_module.sh(1 hunks)staging/setup_scafolding.conf(1 hunks)staging/validate_staged_modules.conf(1 hunks)staging/validate_staged_modules.sh(2 hunks)staging/web_images.conf(1 hunks)staging/web_images.sh(1 hunks)workflow/00_start_here.sh(0 hunks)workflow/50_array_to_json.sh(1 hunks)
💤 Files with no reviewable changes (2)
- workflow/00_start_here.sh
- docs/modules_metadata.json
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: Tearran
PR: Tearran/configng-v2#36
File: src/network/interface/network_manager.sh:46-53
Timestamp: 2025-06-23T18:54:21.952Z
Learning: Tearran prefers heredoc over echo/printf for multi-line templating work, especially for generating dynamic content from metadata. Heredoc provides better readability, easier maintenance, and is more forgiving for complex formatting than chained echo statements.
📚 Learning: 2025-07-16T17:31:03.723Z
Learnt from: Tearran
PR: Tearran/configng-v2#71
File: staging/start_here.sh:0-0
Timestamp: 2025-07-16T17:31:03.723Z
Learning: In configng-v2, scaffold templates are intentionally incomplete and should fail validation until developers complete all required fields. This fail-fast design enforces proper completion before module promotion. The staging validation mode only checks for duplicates, allowing incomplete scaffolds to exist in staging/ while preventing naming conflicts.
Applied to files:
staging/setup_scafolding.confstaging/validate_staged_modules.conf
📚 Learning: 2025-06-23T06:57:50.987Z
Learnt from: Tearran
PR: Tearran/configng-v2#35
File: staging/cockpit.sh:4-6
Timestamp: 2025-06-23T06:57:50.987Z
Learning: In the configng-v2 project, files in the staging/ directory are temporary locations for PR review. The relative path comments in these staging files should reflect the final destination path where the file will be moved before merging, not the current staging location.
Applied to files:
staging/setup_scafolding.conf
📚 Learning: 2025-07-24T05:52:42.317Z
Learnt from: Tearran
PR: Tearran/configng-v2#92
File: workflow/50_array_to_json.sh:236-236
Timestamp: 2025-07-24T05:52:42.317Z
Learning: In workflow/50_array_to_json.sh, core_options are intentionally excluded from the arrays list because they are internal/administrative commands not meant for TUI/GUI/UX interfacing. The JSON output is specifically for user-facing interfaces, so core options should remain separate from the user-facing system/network/software options.
Applied to files:
workflow/50_array_to_json.sh
📚 Learning: 2025-08-21T16:51:34.545Z
Learnt from: Tearran
PR: Tearran/configng-v2#105
File: public_html/json/images/logo.json:1-102
Timestamp: 2025-08-21T16:51:34.545Z
Learning: When reviewing generated JSON files in the public_html/json/ directory, issues with schema or asset paths should be addressed in the generating script (like web_kit.sh) rather than in the output files themselves or other unrelated scripts.
Applied to files:
workflow/50_array_to_json.sh
📚 Learning: 2025-06-18T05:04:30.676Z
Learnt from: Tearran
PR: Tearran/configng-v2#25
File: tools/04_promote_module.sh:30-38
Timestamp: 2025-06-18T05:04:30.676Z
Learning: In tools/04_promote_module.sh, the user wants comprehensive validation of configuration files before promotion, checking required fields like 'feature', 'helpers', 'description', and 'parent' rather than just 'parent'. This maintains fail-fast behavior while ensuring only properly configured modules are promoted.
Applied to files:
staging/promote_staged_module.confstaging/promote_staged_module.shstaging/validate_staged_modules.confstaging/validate_staged_modules.sh
📚 Learning: 2025-07-05T16:08:27.350Z
Learnt from: Tearran
PR: Tearran/configng-v2#58
File: tools/configng_v2.sh:25-27
Timestamp: 2025-07-05T16:08:27.350Z
Learning: In tools/configng_v2.sh, the staging code block (lines 23-31) is specifically for development and refining mini modules. The TRACE=true setting within this block is intentional for development purposes and would be stripped out if/when this becomes the main binary entry point for production. The staging folder and its associated code are temporary development tools, not production code.
Applied to files:
staging/promote_staged_module.confstaging/promote_staged_module.shstaging/validate_staged_modules.confstaging/validate_staged_modules.sh
📚 Learning: 2025-06-14T07:07:36.487Z
Learnt from: Tearran
PR: Tearran/configng-v2#11
File: staging/src_tests.sh:4-7
Timestamp: 2025-06-14T07:07:36.487Z
Learning: The file `staging/src_tests.sh` in the configng-v2 repository is a temporary test file created specifically to test the GitHub workflow `.github/workflows/02_github_workflows_staging.yml`, not a production module meant for implementation.
Applied to files:
staging/promote_staged_module.sh
📚 Learning: 2025-06-29T18:44:33.273Z
Learnt from: Tearran
PR: Tearran/configng-v2#52
File: tools/20_promote_module.sh:81-89
Timestamp: 2025-06-29T18:44:33.273Z
Learning: In tools/20_promote_module.sh, Tearran prefers fail-fast behavior when moving image files to ./docs/ - if the docs directory doesn't exist at promotion time, the script should fail rather than auto-create it, as this indicates an upstream workflow failure that needs to be addressed.
Applied to files:
staging/promote_staged_module.sh
📚 Learning: 2025-06-18T04:59:54.787Z
Learnt from: Tearran
PR: Tearran/configng-v2#25
File: tools/04_promote_module.sh:30-38
Timestamp: 2025-06-18T04:59:54.787Z
Learning: In tools/04_promote_module.sh, the documentation promotion logic that only moves .md files when the parent variable is non-empty is intentional fail-fast behavior, not a bug. This prevents compounded issues by ensuring docs are only moved when there's valid parent context from the .sh file processing loop.
Applied to files:
staging/promote_staged_module.sh
📚 Learning: 2025-07-23T18:29:39.709Z
Learnt from: Tearran
PR: Tearran/configng-v2#89
File: modules_browsers/modules_browser.html:470-476
Timestamp: 2025-07-23T18:29:39.709Z
Learning: Tearran's configng-v2 project has excellent implementation across most goals: 15/15 modules with matching .conf/.sh files, robust documentation automation (4 workflow scripts), existing scaffolding (20_promote_module.sh), strong frontend/backend separation (20 interface modules), and active JSON generation. The main verification needed is CLI compatibility between the new ID system (USE001, MAN001) and existing armbian/configng integration points.
Applied to files:
staging/promote_staged_module.shstaging/validate_staged_modules.sh
📚 Learning: 2025-06-17T20:56:24.185Z
Learnt from: Tearran
PR: Tearran/configng-v2#25
File: tools/03_promote_module.sh:10-11
Timestamp: 2025-06-17T20:56:24.185Z
Learning: The arrays `array_entries` and `group_counts` declared in `tools/03_promote_module.sh` are actually used by `tools/10_consolidate_module.sh`, indicating these scripts work together in the module management workflow.
Applied to files:
staging/promote_staged_module.sh
📚 Learning: 2025-06-18T04:04:02.296Z
Learnt from: Tearran
PR: Tearran/configng-v2#25
File: tools/02_validate_module.sh:57-78
Timestamp: 2025-06-18T04:04:02.296Z
Learning: In tools/02_validate_module.sh, Tearran prefers not to use the `|| status=1` pattern for exit code handling. He wants to leverage return values or handle status tracking in the helper functions themselves, maintaining strict bash compliance with `set -euo pipefail`.
Applied to files:
staging/validate_staged_modules.sh
📚 Learning: 2025-06-25T10:59:55.082Z
Learnt from: Tearran
PR: Tearran/configng-v2#44
File: staging/package.sh:26-28
Timestamp: 2025-06-25T10:59:55.082Z
Learning: For staging/package.sh in the configng-v2 project: The user prefers to keep working code unchanged rather than applying shellcheck suggestions that don't provide functional benefits in their specific target environment (Debian/Ubuntu with bash). The pkg_installed() function works correctly as implemented since it uses 2>/dev/null for error handling and doesn't depend on command exit status.
Applied to files:
staging/validate_staged_modules.sh
🧬 Code graph analysis (5)
docs/webmin.md (1)
src/software/management/webmin.sh (2)
_about_webmin(6-41)webmin(44-116)
docs/README.md (1)
src/software/management/webmin.sh (1)
_about_webmin(6-41)
staging/promote_staged_module.sh (3)
src/system/user/adjust_motd.sh (2)
_about_adjust_motd(83-116)adjust_motd(118-128)src/software/management/webmin.sh (2)
_about_webmin(6-41)webmin(44-116)src/core/interface/package.sh (2)
package(47-65)_about_package(67-96)
staging/validate_staged_modules.sh (2)
src/system/user/adjust_motd.sh (1)
_about_adjust_motd(83-116)src/core/interface/package.sh (1)
_about_package(67-96)
staging/web_images.sh (3)
src/software/management/webmin.sh (2)
_about_webmin(6-41)webmin(44-116)src/system/user/adjust_motd.sh (2)
_about_adjust_motd(83-116)adjust_motd(118-128)src/core/interface/package.sh (1)
_about_package(67-96)
🪛 LanguageTool
docs/initialize_env.md
[grammar] ~1-~1: There might be a mistake here.
Context: # initialize_env Detect and expose common environment var...
(QB_NEW_EN)
docs/webmin.md
[grammar] ~1-~1: There might be a mistake here.
Context: # webmin Webmin setup and service control. ## Ab...
(QB_NEW_EN)
[grammar] ~4-~4: There might be a mistake here.
Context: ...min setup and service control. ## About Provides installation removal service ma...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
docs/initialize_env.md
12-12: Hard tabs
Column: 1
(MD010, no-hard-tabs)
15-15: Hard tabs
Column: 1
(MD010, no-hard-tabs)
18-18: Hard tabs
Column: 1
(MD010, no-hard-tabs)
19-19: Hard tabs
Column: 1
(MD010, no-hard-tabs)
docs/plexmediaserver.md
12-12: Hard tabs
Column: 1
(MD010, no-hard-tabs)
13-13: Hard tabs
Column: 1
(MD010, no-hard-tabs)
14-14: Hard tabs
Column: 1
(MD010, no-hard-tabs)
15-15: Hard tabs
Column: 1
(MD010, no-hard-tabs)
19-19: Hard tabs
Column: 1
(MD010, no-hard-tabs)
20-20: Hard tabs
Column: 1
(MD010, no-hard-tabs)
21-21: Hard tabs
Column: 1
(MD010, no-hard-tabs)
22-22: Hard tabs
Column: 1
(MD010, no-hard-tabs)
25-25: Hard tabs
Column: 1
(MD010, no-hard-tabs)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
27-27: Hard tabs
Column: 1
(MD010, no-hard-tabs)
28-28: Hard tabs
Column: 1
(MD010, no-hard-tabs)
29-29: Hard tabs
Column: 1
(MD010, no-hard-tabs)
30-30: Hard tabs
Column: 1
(MD010, no-hard-tabs)
docs/README.md
27-27: Hard tabs
Column: 1
(MD010, no-hard-tabs)
29-29: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (13)
workflow/50_array_to_json.sh (1)
236-236: Arrays list correctly excludes core_optionsKeeping arrays limited to system_options, network_options, and software_options is consistent with the prior decision to exclude core_options from user-facing JSON. No change needed.
staging/setup_scafolding.conf (2)
11-11: Minor copy tweak is fine; keeps tone consistent.Switching
.confand.shfrom code style to plain text in extend_desc is harmless and reads well. No functional impact.
43-43: Helpers list already includes required help function — good.Presence of
_about_setup_scafoldingmatches the validation rule noted in-file. Nothing to change here.staging/web_images.conf (1)
17-22: Ensure all staging configs have definedparentandgroupbefore promotionSeveral staging configuration files still have empty
parentandgroupfields. It’s fine to leave these blank while iterating in staging, but you must populate both values prior to running the promotion pipeline so modules are placed correctly.Files missing both fields:
- staging/web_images.conf (line 17:
parent=, line 21:group=)- staging/validate_staged_modules.conf (line 17:
parent=, line 21:group=)- staging/promote_staged_module.conf (line 17:
parent=, line 21:group=)Before you promote, update each file’s:
parent– the name of the parent module or directorygroup– the group/tag used by the promotion script (see docs/readme.md)This will ensure the promotion script can route and place your modules without manual intervention.
docs/plexmediaserver.md (2)
12-16: Ignore MD010 here: keep hard tabs in markdown code blocksPer the project’s established style (see docs/net_render.md), hard tabs are the preferred formatting in markdown files. The MD010 warnings for these code-block lines can be safely ignored—no changes needed.
Likely an incorrect or invalid review comment.
33-33: Autogenerated source paths confirmedBoth referenced files exist at
src/software/management/plexmediaserver.confandsrc/software/management/plexmediaserver.sh, so the “Autogenerated from” note is accurate and no changes are needed.staging/promote_staged_module.conf (2)
17-22: Staging config blanks verified: parent/group fields correctly emptyI’ve confirmed that in staging/promote_staged_module.conf the
parent(line 17) andgroup(line 21) settings are intentionally left blank for staging. These will be populated during the promotion step to compute the final paths. No changes needed here.
38-41: Include Required Helper Functions in ConfigurationThe
helpersvariable instaging/promote_staged_module.confmust list both the main entrypoint and the help-message function to satisfy the validation logic (which fails if_about_promote_staged_moduleis omitted).• File: staging/promote_staged_module.conf
Lines: 38–41-helpers= +helpers=_promote_staged_module_main,_about_promote_staged_moduleThis ensures the validation note (“You must include the help message function _about_promote_staged_module; validation will fail if it is missing.”) is complied with.
⛔ Skipped due to learnings
Learnt from: Tearran PR: Tearran/configng-v2#25 File: tools/04_promote_module.sh:30-38 Timestamp: 2025-06-18T05:04:30.676Z Learning: In tools/04_promote_module.sh, the user wants comprehensive validation of configuration files before promotion, checking required fields like 'feature', 'helpers', 'description', and 'parent' rather than just 'parent'. This maintains fail-fast behavior while ensuring only properly configured modules are promoted.Learnt from: Tearran PR: Tearran/configng-v2#0 File: :0-0 Timestamp: 2025-07-10T19:23:27.837Z Learning: In the configng-v2 project, the scaffold template in tools/start_here.sh was updated to generate CSV format for configuration fields (arch, require_os, require_kernel, helpers, options) but existing .conf files still use space-separated format. The helpers validation in tools/10_validate_module.sh line 69 uses grep -qw which breaks with CSV format and needs to be updated to handle comma-separated values.staging/validate_staged_modules.sh (1)
191-201: Self-test looks good.The guard neatly asserts the help surface without side effects before delegating. This mirrors patterns in existing modules.
staging/web_images.sh (1)
112-124: Self-test is fine; low risk.The help assertion doesn’t mutate state and then delegates. After the earlier fix, nothing else is needed here.
docs/webmin.md (1)
1-43: Docs content matches code; autogenerated note is present.Cross-check against src/software/management/webmin.sh looks consistent.
docs/initialize_env.md (2)
1-23: Overall: Clear, minimal, and helpful doc; just needs polish.Good overview and intent, plus a self-test note. Address the small naming, formatting, and lint issues above and this is ship-ready.
18-19: Helper_initialize_env_varsverified – no changes needed in docs
- The
_initialize_env_varsfunction is defined insrc/core/initialize/initialize_env.shat line 23 and is invoked in multiple case branches (lines 14, 19, 87).- The documentation snippet in
docs/initialize_env.mdcorrectly references this entrypoint.
staging/promote_staged_module.sh
Outdated
| parent="$(grep '^parent=' "$conf_file" | head -n1 | cut -d= -f2- | xargs)" | ||
| group="$(grep '^group=' "$conf_file" | head -n1 | cut -d= -f2- | xargs)" | ||
|
|
||
| if [[ -z "$parent" ]]; then | ||
| echo "No parent= in $conf_file, skipping $sh_file" | ||
| continue | ||
| fi | ||
|
|
||
| if [[ -n "$group" ]]; then | ||
| dest_dir="./src/$parent/$group" | ||
| else | ||
| dest_dir="./src/$parent" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate required conf fields before moving; fail fast on unknown parent path.
Per earlier workflow preferences, promotion should only occur for well-formed configs and known destinations to prevent silent tree pollution (e.g., typos creating new directories).
- parent="$(grep '^parent=' "$conf_file" | head -n1 | cut -d= -f2- | xargs)"
- group="$(grep '^group=' "$conf_file" | head -n1 | cut -d= -f2- | xargs)"
+ parent="$(grep -Em1 '^parent=' "$conf_file" | cut -d= -f2- | xargs)"
+ group="$(grep -Em1 '^group=' "$conf_file" | cut -d= -f2- | xargs)"
+ # quick presence/format checks (feature/helpers/description/parent at minimum)
+ if ! grep -Eqm1 '^feature=' "$conf_file" \
+ || ! grep -Eqm1 '^helpers=' "$conf_file" \
+ || ! grep -Eqm1 '^description=' "$conf_file" \
+ || ! grep -Eqm1 '^parent=' "$conf_file"; then
+ echo "ERROR: $conf_file missing one or more required fields (feature/helpers/description/parent). Aborting."
+ exit 1
+ fi
…
- if [[ -n "$group" ]]; then
- dest_dir="./src/$parent/$group"
- else
- dest_dir="./src/$parent"
- fi
- mkdir -p "$dest_dir"
+ if [[ -n "$group" ]]; then
+ dest_dir="./src/$parent/$group"
+ else
+ dest_dir="./src/$parent"
+ fi
+ # Fail if parent path does not pre-exist (typo guard)
+ if [[ ! -d "./src/$parent" ]]; then
+ echo "ERROR: Destination './src/$parent' does not exist. Check 'parent=' in $conf_file."
+ exit 1
+ fi
+ mkdir -p "$dest_dir"If you prefer, we can invoke the validator first and abort on any failures:
validate_staged_modules || { echo "Validation failed; aborting promotion."; exit 1; }Say the word and I’ll wire this into the main path.
🤖 Prompt for AI Agents
In staging/promote_staged_module.sh around lines 23 to 35, the script currently
reads parent and group from the .conf and constructs a destination path but does
not validate the config fields or ensure the destination exists, allowing typos
to create new directories; update the logic to (1) validate required fields (at
minimum ensure parent is present and matches an expected pattern or list) and
abort immediately on failure, and (2) verify the computed dest_dir exists under
./src (or call an existing validate_staged_modules function and abort if it
fails) before moving files so promotion fails fast instead of silently creating
new tree entries.
| echo "Moving $sh_file and $conf_file to $dest_dir/" | ||
| mv "$sh_file" "$dest_dir/" | ||
| mv "$conf_file" "$dest_dir/" | ||
|
|
||
| # Move any matching image files to the module destination directory | ||
| for ext in png jpg jpeg gif svg; do | ||
| img_file="./staging/${base_name}.${ext}" | ||
| if [[ -f "$img_file" ]]; then | ||
| echo "Moving image: $img_file to $dest_dir/" | ||
| mv "$img_file" "$dest_dir/" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Protect against overwriting existing files.
mv will overwrite silently. Safer: bail if a target already exists to avoid accidental clobbering.
- echo "Moving $sh_file and $conf_file to $dest_dir/"
- mv "$sh_file" "$dest_dir/"
- mv "$conf_file" "$dest_dir/"
+ for f in "$sh_file" "$conf_file"; do
+ t="$dest_dir/$(basename "$f")"
+ if [[ -e "$t" ]]; then
+ echo "ERROR: Destination already contains $(basename "$f") at $dest_dir/. Aborting to prevent overwrite."
+ exit 1
+ fi
+ done
+ echo "Moving $sh_file and $conf_file to $dest_dir/"
+ mv "$sh_file" "$dest_dir/"
+ mv "$conf_file" "$dest_dir/"🤖 Prompt for AI Agents
In staging/promote_staged_module.sh around lines 38 to 49, mv is used and will
silently overwrite existing files; update the script to protect against
clobbering by checking for the existence of each destination file before moving
(for the .sh, .conf and each image extension), and if any target already exists
print an error and exit non-zero. Perform the existence check using a
conditional like [[ -e "$dest_dir/$filename" ]] prior to calling mv, so no moves
occur if a conflict is detected, and keep the quoting of paths when moving and
when reporting the error.
| # Comma-separated list of functions in this module (all functions except the main feature). | ||
| # NOTE: You must include the help message function _about_web_images; validation will fail if it is missing. | ||
| helpers= | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Declare helpers to meet the validation rule.
Metadata says _about_web_images is required. Add the helpers list when the functions are in place.
-helpers=
+helpers=_web_images_main,_about_web_imagesVerify the functions exist in the script:
🏁 Script executed:
#!/bin/bash
# Confirm expected functions for web_images exist
rg -nP '^\s*(web_images|_web_images_main|_about_web_images)\s*\(\)' staging/web_images.shLength of output: 157
Add required helpers to staging/web_images.conf
The script defines two helper functions—_web_images_main() and _about_web_images()—and validation requires listing all helpers (excluding the main feature web_images) including the help message function. Update the helpers setting as follows:
File – staging/web_images.conf (around lines 38–41)
-helpers=
+helpers=_web_images_main,_about_web_images📝 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.
| # Comma-separated list of functions in this module (all functions except the main feature). | |
| # NOTE: You must include the help message function _about_web_images; validation will fail if it is missing. | |
| helpers= | |
| # Comma-separated list of functions in this module (all functions except the main feature). | |
| # NOTE: You must include the help message function _about_web_images; validation will fail if it is missing. | |
| helpers=_web_images_main,_about_web_images |
🤖 Prompt for AI Agents
In staging/web_images.conf around lines 38 to 41, the helpers setting is empty
but must list all helper functions (excluding the main feature `web_images`) so
validation passes; update the comma-separated helpers value to include the two
helper functions used by the script by setting helpers to
_web_images_main,_about_web_images.
| _web_images_main() { | ||
| #!/bin/bash | ||
|
|
||
| # Directory containing SVGs (env override supported) | ||
| SRC_DIR="${SRC_DIR:-assets/images/logos}" | ||
| # List of desired sizes (env override supported) | ||
| SIZES=(${SIZES[@]:-16 32 48 64 128 256 512}) | ||
|
|
There was a problem hiding this comment.
Remove stray shebang inside function and fix SIZES parsing under set -u.
- A second shebang inside a function is a syntax error.
- ${SIZES[@]:-…} can misbehave when SIZES isn’t an array. Parse from string safely.
_web_images_main() {
-#!/bin/bash
-
-# Directory containing SVGs (env override supported)
-SRC_DIR="${SRC_DIR:-assets/images/logos}"
-# List of desired sizes (env override supported)
-SIZES=(${SIZES[@]:-16 32 48 64 128 256 512})
+ # Directory containing SVGs (env override supported)
+ local SRC_DIR="${SRC_DIR:-assets/images/logos}"
+ # List of desired sizes (env override supported)
+ local -a SIZES
+ if [[ -n "${SIZES:-}" ]]; then
+ # parse space/comma-separated values from env SIZES
+ IFS=', ' read -r -a SIZES <<< "${SIZES}"
+ else
+ SIZES=(16 32 48 64 128 256 512)
+ fiCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In staging/web_images.sh around lines 17 to 24, remove the stray "#!/bin/bash"
that is placed inside the _web_images_main function (shebangs belong at the top
of the file), and replace the brittle SIZES assignment so it safely supports set
-u and non-array inputs by defaulting to a space-separated string and then
populating an array from that string (e.g., use a default string like "16 32 48
64 128 256 512" and split it into an array or use read -a to populate SIZES),
ensuring the script works whether SIZES is unset, a string, or an array.
| # Check for ImageMagick's convert command | ||
| if ! command -v convert &> /dev/null; then | ||
| echo "Error: ImageMagick 'convert' command not found." | ||
| read -p "Would you like to install ImageMagick using 'sudo apt install imagemagick'? [Y/n] " yn | ||
| case "$yn" in | ||
| [Yy]* | "" ) | ||
| echo "Installing ImageMagick..." | ||
| sudo apt update && sudo apt install imagemagick | ||
| if ! command -v convert &> /dev/null; then | ||
| echo "Installation failed or 'convert' still not found. Exiting." | ||
| exit 1 | ||
| fi | ||
| ;; | ||
| * ) | ||
| echo "Cannot proceed without ImageMagick. Exiting." | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| fi |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Support IM7 “magick” CLI and avoid hanging in non-interactive runs.
- Prefer “magick convert” when convert isn’t present.
- Detect non-TTY and default to exit (or auto-install via an opt-in flag).
-# Check for ImageMagick's convert command
-if ! command -v convert &> /dev/null; then
- echo "Error: ImageMagick 'convert' command not found."
- read -p "Would you like to install ImageMagick using 'sudo apt install imagemagick'? [Y/n] " yn
- case "$yn" in
- [Yy]* | "" )
- echo "Installing ImageMagick..."
- sudo apt update && sudo apt install imagemagick
- if ! command -v convert &> /dev/null; then
- echo "Installation failed or 'convert' still not found. Exiting."
- exit 1
- fi
- ;;
- * )
- echo "Cannot proceed without ImageMagick. Exiting."
- exit 1
- ;;
- esac
-fi
+ # Resolve ImageMagick CLI
+ local CONVERT_BIN="convert"
+ if ! command -v "$CONVERT_BIN" >/dev/null 2>&1; then
+ # IM7 fallback
+ if command -v magick >/dev/null 2>&1; then
+ CONVERT_BIN="magick convert"
+ else
+ echo "Error: ImageMagick not found."
+ if [[ -t 0 && -t 1 ]]; then
+ read -r -p "Install ImageMagick now with 'sudo apt install imagemagick'? [Y/n] " yn
+ case "$yn" in
+ [Yy]* | "" )
+ echo "Installing ImageMagick..."
+ sudo apt update && sudo apt install -y imagemagick ;;
+ *) echo "Cannot proceed without ImageMagick. Exiting."; exit 1;;
+ esac
+ else
+ echo "Non-interactive session; set ALLOW_AUTO_INSTALL=1 to auto-install." >&2
+ [[ "${ALLOW_AUTO_INSTALL:-0}" == "1" ]] && { sudo apt update && sudo apt install -y imagemagick; } || exit 1
+ fi
+ fi
+ fiTo verify IM7 availability across your runners, I can script-check with which magick/convert in CI if you’d like.
🏁 Script executed:
#!/bin/bash
# Locate the web_images.sh script
find . -type f -name web_images.sh
# Show context around the ImageMagick convert check
rg -n 'command -v convert' -C3 staging/web_images.sh
# Check for any existing references to 'magick' in this script
rg -n 'magick' -C3 staging/web_images.shLength of output: 1236
Support IM7 “magick” CLI and handle non-interactive sessions
The current check in staging/web_images.sh only looks for convert, which will fail on ImageMagick 7-only installs (where the CLI is magick). It also prompts unconditionally, causing hangs in CI or other non-TTY environments. Please update as follows:
• Introduce a local CONVERT_BIN variable and, if convert is missing but magick exists, set it to magick convert.
• Detect interactive shells via [[ -t 0 && -t 1 ]] and prompt only then; in non-TTY sessions, exit by default (or auto-install if ALLOW_AUTO_INSTALL=1).
• Always pass -y to apt install for unattended installs.
Suggested replacement for lines 25–43:
-# Check for ImageMagick's convert command
-if ! command -v convert &> /dev/null; then
- echo "Error: ImageMagick 'convert' command not found."
- read -p "Would you like to install ImageMagick using 'sudo apt install imagemagick'? [Y/n] " yn
- case "$yn" in
- [Yy]* | "" )
- echo "Installing ImageMagick..."
- sudo apt update && sudo apt install imagemagick
- if ! command -v convert &> /dev/null; then
- echo "Installation failed or 'convert' still not found. Exiting."
- exit 1
- fi
- ;;
- * )
- echo "Cannot proceed without ImageMagick. Exiting."
- exit 1
- ;;
- esac
-fi
+# Resolve ImageMagick CLI
+local CONVERT_BIN="convert"
+if ! command -v "$CONVERT_BIN" >/dev/null 2>&1; then
+ # IM7 fallback
+ if command -v magick >/dev/null 2>&1; then
+ CONVERT_BIN="magick convert"
+ else
+ echo "Error: ImageMagick not found." >&2
+ if [[ -t 0 && -t 1 ]]; then
+ # interactive install prompt
+ read -r -p "Install ImageMagick now with 'sudo apt install imagemagick'? [Y/n] " yn
+ case "$yn" in
+ [Yy]* | "" )
+ echo "Installing ImageMagick..."
+ sudo apt update && sudo apt install -y imagemagick
+ ;;
+ * )
+ echo "Cannot proceed without ImageMagick. Exiting." >&2
+ exit 1
+ ;;
+ esac
+ else
+ # non-interactive: require explicit opt-in
+ echo "Non-interactive session; set ALLOW_AUTO_INSTALL=1 to auto-install ImageMagick." >&2
+ if [[ "${ALLOW_AUTO_INSTALL:-0}" == "1" ]]; then
+ sudo apt update && sudo apt install -y imagemagick
+ else
+ exit 1
+ fi
+ fi
+ # recheck after install
+ if ! command -v "$CONVERT_BIN" >/dev/null 2>&1; then
+ echo "Installation failed or CLI still not found. Exiting." >&2
+ exit 1
+ fi
+ fi
+fiLet me know if you’d like a quick CI script to verify presence of convert/magick across your runners.
📝 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.
| # Check for ImageMagick's convert command | |
| if ! command -v convert &> /dev/null; then | |
| echo "Error: ImageMagick 'convert' command not found." | |
| read -p "Would you like to install ImageMagick using 'sudo apt install imagemagick'? [Y/n] " yn | |
| case "$yn" in | |
| [Yy]* | "" ) | |
| echo "Installing ImageMagick..." | |
| sudo apt update && sudo apt install imagemagick | |
| if ! command -v convert &> /dev/null; then | |
| echo "Installation failed or 'convert' still not found. Exiting." | |
| exit 1 | |
| fi | |
| ;; | |
| * ) | |
| echo "Cannot proceed without ImageMagick. Exiting." | |
| exit 1 | |
| ;; | |
| esac | |
| fi | |
| # Resolve ImageMagick CLI | |
| local CONVERT_BIN="convert" | |
| if ! command -v "$CONVERT_BIN" >/dev/null 2>&1; then | |
| # IM7 fallback | |
| if command -v magick >/dev/null 2>&1; then | |
| CONVERT_BIN="magick convert" | |
| else | |
| echo "Error: ImageMagick not found." >&2 | |
| if [[ -t 0 && -t 1 ]]; then | |
| # interactive install prompt | |
| read -r -p "Install ImageMagick now with 'sudo apt install imagemagick'? [Y/n] " yn | |
| case "$yn" in | |
| [Yy]* | "" ) | |
| echo "Installing ImageMagick..." | |
| sudo apt update && sudo apt install -y imagemagick | |
| ;; | |
| * ) | |
| echo "Cannot proceed without ImageMagick. Exiting." >&2 | |
| exit 1 | |
| ;; | |
| esac | |
| else | |
| # non-interactive: require explicit opt-in | |
| echo "Non-interactive session; set ALLOW_AUTO_INSTALL=1 to auto-install ImageMagick." >&2 | |
| if [[ "${ALLOW_AUTO_INSTALL:-0}" == "1" ]]; then | |
| sudo apt update && sudo apt install -y imagemagick | |
| else | |
| exit 1 | |
| fi | |
| fi | |
| # recheck after install | |
| if ! command -v "$CONVERT_BIN" >/dev/null 2>&1; then | |
| echo "Installation failed or CLI still not found. Exiting." >&2 | |
| exit 1 | |
| fi | |
| fi | |
| fi |
🤖 Prompt for AI Agents
In staging/web_images.sh around lines 25 to 43, the script currently only checks
for the `convert` binary and always prompts the user, which breaks on
ImageMagick‑7 (whose CLI is `magick`) and in non‑interactive CI; replace this
block to define a local CONVERT_BIN variable: if `convert` exists use "convert",
else if `magick` exists set CONVERT_BIN to "magick convert"; detect interactive
sessions with [[ -t 0 && -t 1 ]] and only prompt when interactive, otherwise
exit unless ALLOW_AUTO_INSTALL=1 is set; when auto‑installing use `sudo apt
update && sudo apt install -y imagemagick`; after installation recheck
CONVERT_BIN and exit with error if still missing.
Refine workflow to follow staged module