Add kit directory validation during installation#353
Conversation
Review Summary by QodoAdd kit directory validation to installation script
WalkthroughsDescription• Adds validation for kit directories during installation • Checks template files for missing kit paths specified in upload_extra • Displays formatted warning with missing kit details and guidance • Handles both array and string format kit specifications Diagramflowchart LR
A["Installation Complete"] --> B["Validate Kit Directories"]
B --> C["Parse Template Files"]
C --> D["Extract upload_extra Values"]
D --> E["Check Path Existence"]
E --> F{Missing Kits?}
F -->|Yes| G["Display Warning Message"]
F -->|No| H["Continue"]
G --> H
File Changes1. bin/install.sh
|
Code Review by Qodo
1. /home/uploads warning missing
|
|
This relates to RPOPC-700 |
sayalibhavsar
added
the
group_review_lgtm
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # Validate kit directories specified in templates | ||
| echo "" | ||
| echo "Validating kit directories..." | ||
|
|
There was a problem hiding this comment.
1. /home/uploads warning missing 📎 Requirement gap ⛯ Reliability
The install script changes add warnings for missing kit paths but do not check whether /home/uploads exists and emit the required non-blocking warning text. This fails the specified installation-time visibility requirement for missing /home/uploads.
Agent Prompt
## Issue description
The install script must warn (without failing) if `/home/uploads` is missing, and the warning text must match the compliance requirement exactly.
## Issue Context
Current PR adds kit-path validation warnings, but does not implement the required `/home/uploads` presence check and exact warning string.
## Fix Focus Areas
- bin/install.sh[212-215]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
PullHero ReviewProvider: generativelanguage.googleapis.com Model: gemini-2.0-flash Okay, I've reviewed the changes to Overall Assessment: The changes look good overall. This addresses a common issue where Zathras would be configured to use kits that don't exist, leading to confusing errors later on. The warning message is clear and actionable. Detailed Feedback:
Specific Comments:
Suggestions:
Revised Code (incorporating suggestion 1): --- a/bin/install.sh
+++ b/bin/install.sh
@@ -212,6 +212,15 @@
echo ""
echo "Validating kit directories..."
+# Check if yq is installed
+if ! command -v yq &> /dev/null; then
+ echo ""
+ echo "WARNING: yq is not installed. Kit directory validation will be skipped."
+ echo "Please install yq (version 2.10.0) to enable kit validation."
+ echo ""
+ exit 0 # Skip the rest of the validation
+fi
+
missing_kits=()
config_dir=$(readlink -f "config")
Voting Directive: Vote: +1 Vote: +1 |
PullHero ReviewProvider: generativelanguage.googleapis.com Model: gemini-2.0-flash Okay, I've reviewed the changes to Here's a breakdown of my analysis: Positive Aspects:
Potential Concerns and Suggestions:
Refactored Code Snippet (Illustrative): # Check if upload_extra is a valid path
check_path_exists() {
local template_name="$1"
local path="$2"
if [[ "$path" != "none" ]] && [[ -n "$path" ]]; then
if [[ ! -e "$path" ]]; then
missing_kits+=("$template_name:$path")
fi
fi
}
# Process upload_extra value
process_upload_extra() {
local template_file="$1"
local template_name="$2"
local upload_extra="$3"
local is_array
is_array=$(yq -r '.upload_extra | type' "$template_file" 2>/dev/null || echo "null")
if [[ "$is_array" == "array" ]]; then
# It's an array, process each element
while IFS= read -r path; do
check_path_exists "$template_name" "$path"
done < <(yq -r '.upload_extra[]' "$template_file" 2>/dev/null)
else
# It's a string, split by spaces
IFS=' ' read -ra paths <<< "$upload_extra"
for path in "${paths[@]}"; do
check_path_exists "$template_name" "$path"
done
fi
}
for template_file in config/*_template.yml; do
# Skip if no files match the pattern
[ -e "$template_file" ] || continue
template_name=$(basename "$template_file")
# Extract upload_extra value using yq
upload_extra=$(yq -r '.upload_extra // "none"'
**Vote**: 0
**[PullHero](https://github.com/pullhero/)** |
3 participants
Description
This PR adds inline validation to install.sh that checks for missing kit files/directories specified in config templates and displays clear warnings during installation.
Before/After Comparison
Before: Installation completed silently even if required kits were missing, causing wrappers to fail later without clear indication why.
After: Installation displays a formatted warning listing all missing kits with their paths and guidance on how to resolve the issue.
Documentation Check
No documentation updates needed as this is a non-blocking warning feature that enhances existing installation behavior without changing user workflows.
Clerical Stuff
This closes #323
Relates to JIRA: RPOPC-700