Add kit directory validation during installation#352
Closed
sayalibhavsar wants to merge 1 commit intoredhat-performance:mainfrom
Closed
Add kit directory validation during installation#352sayalibhavsar wants to merge 1 commit intoredhat-performance:mainfrom
sayalibhavsar wants to merge 1 commit intoredhat-performance:mainfrom
Conversation
Review Summary by QodoAdd kit directory validation during installation
WalkthroughsDescription• Adds inline validation to check for missing kit directories during installation • Parses template files to extract upload_extra paths using yq • Displays formatted warning with missing kit locations and resolution guidance • Handles both array and string formats for upload_extra configuration Diagramflowchart LR
A["Installation Complete"] --> B["Parse Config Templates"]
B --> C["Extract upload_extra Paths"]
C --> D["Check Path Existence"]
D --> E{Missing Kits?}
E -->|Yes| F["Display Formatted Warning"]
E -->|No| G["Continue"]
F --> G
File Changes1. bin/install.sh
|
Code Review by Qodo
1. /home/uploads check missing
|
sayalibhavsar
added
the
group_review_lgtm
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+212
to
+280
| # Validate kit directories specified in templates | ||
| echo "" | ||
| echo "Validating kit directories..." | ||
|
|
||
| missing_kits=() | ||
| config_dir=$(readlink -f "config") | ||
|
|
||
| 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"' "$template_file" 2>/dev/null || echo "none") | ||
|
|
||
| # Skip if upload_extra is 'none', empty, or null | ||
| if [[ "$upload_extra" == "none" ]] || [[ -z "$upload_extra" ]] || [[ "$upload_extra" == "null" ]]; then | ||
| continue | ||
| fi | ||
|
|
||
| # Check if upload_extra is an array or a string | ||
| 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 | ||
| if [[ "$path" != "none" ]] && [[ -n "$path" ]]; then | ||
| if [[ ! -e "$path" ]]; then | ||
| missing_kits+=("$template_name:$path") | ||
| fi | ||
| fi | ||
| 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 | ||
| if [[ "$path" != "none" ]] && [[ -n "$path" ]]; then | ||
| if [[ ! -e "$path" ]]; then | ||
| missing_kits+=("$template_name:$path") | ||
| fi | ||
| fi | ||
| done | ||
| fi | ||
| done | ||
|
|
||
| # Display warning if there are missing kits | ||
| if [ ${#missing_kits[@]} -gt 0 ]; then | ||
| echo "" | ||
| echo "======================================================================" | ||
| echo "WARNING: Missing Kit Directories" | ||
| echo "======================================================================" | ||
| echo "" | ||
| echo "The following kit files/directories specified in templates do not exist:" | ||
| echo "" | ||
| for entry in "${missing_kits[@]}"; do | ||
| template=$(echo "$entry" | cut -d':' -f1) | ||
| path=$(echo "$entry" | cut -d':' -f2-) | ||
| echo " - $template: $path" | ||
| done | ||
| echo "" | ||
| echo "These kits are required for certain benchmarks to run properly." | ||
| echo "Wrappers depending on these kits may fail until they are provided." | ||
| echo "" | ||
| echo "Please ensure the required kits are placed at the specified locations" | ||
| echo "or update the template files in $config_dir to reflect the correct paths." | ||
| echo "======================================================================" | ||
| echo "" | ||
| fi |
There was a problem hiding this comment.
1. /home/uploads check missing 📎 Requirement gap ⛯ Reliability
The installer changes add kit-path validation but do not check whether /home/uploads exists or emit the required non-blocking warning message. This fails the required installer visibility behavior when /home/uploads is missing.
Agent Prompt
## Issue description
The installer must warn (non-blocking) when `/home/uploads` is missing, but the current changes only validate kit paths from templates and do not implement the required `/home/uploads` warning.
## Issue Context
Compliance requires the installer to check for `/home/uploads` and print the message: `Warning: /home/uploads directory not found. Some wrappers may fail until it is created.` (or functionally equivalent) without aborting installation.
## Fix Focus Areas
- bin/install.sh[209-285]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+225
to
+226
| # Extract upload_extra value using yq | ||
| upload_extra=$(yq -r '.upload_extra // "none"' "$template_file" 2>/dev/null || echo "none") |
There was a problem hiding this comment.
2. Yq not on path 🐞 Bug ⛯ Reliability
The validation calls yq immediately after installing it via pip3 --user, but the script never adds the user bin dir to PATH. If yq is not already on PATH, the validation silently treats all templates as having no upload_extra, defeating the PR’s goal.
Agent Prompt
### Issue description
The new validation invokes `yq` but `yq` is installed via `pip3 --user` and may not be on `PATH` during the same script run. Because stderr is suppressed and a fallback value is used, the validation can silently do nothing.
### Issue Context
`pip --user` installs executables under the Python user base (commonly `~/.local/bin`). The script itself later tells the user to ensure `~/.local/bin` is on their PATH, but validation happens before that guidance.
### Fix Focus Areas
- bin/install.sh[127-138]
- bin/install.sh[212-256]
### Suggested approach
- After the pip installs, add something like:
- `USER_BASE_BIN="$(python3 -m site --user-base)/bin"; export PATH="$USER_BASE_BIN:$PATH"`
- Before running validation, do:
- `command -v yq >/dev/null || { echo "Skipping kit validation: yq not found on PATH"; skip block; }`
- Avoid silently treating missing `yq` as `upload_extra=none`; emit a clear warning when skipping validation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+246
to
+248
| # It's a string, split by spaces | ||
| IFS=' ' read -ra paths <<< "$upload_extra" | ||
| for path in "${paths[@]}"; do |
There was a problem hiding this comment.
3. Comma parsing mismatch 🐞 Bug ✓ Correctness
For string-valued upload_extra, the validation splits only on spaces. This conflicts with existing docs and runtime parsing which treat upload_extra as a comma-separated list; comma-separated strings will be treated as a single path and incorrectly reported missing.
Agent Prompt
### Issue description
The validation’s string parsing only splits on spaces, but `upload_extra` is documented and used elsewhere as comma-separated. This causes false positives (e.g., `/a.tar,/b.tar` checked as a single non-existent path).
### Issue Context
Existing logic in `bin/burden` replaces commas with spaces before iterating. Docs also specify comma-separated.
### Fix Focus Areas
- bin/install.sh[245-255]
- docs/test_config_files.md[124-127]
- bin/burden[823-829]
### Suggested approach
- In the string branch, normalize commas:
- `upload_extra_norm="${upload_extra//,/ }"`
- then split on default IFS whitespace: `read -r -a paths <<< "$upload_extra_norm"`
- Consider applying the same normalization to array elements too (defensive), if an element itself contains commas.
- Add a small comment noting supported delimiters: comma and/or whitespace.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+216
to
+220
| missing_kits=() | ||
| config_dir=$(readlink -f "config") | ||
|
|
||
| for template_file in config/*_template.yml; do | ||
| # Skip if no files match the pattern |
There was a problem hiding this comment.
4. Config path assumption 🐞 Bug ⛯ Reliability
The validation uses readlink -f "config" and globs config/*_template.yml relative to the current working directory. Under set -e, running the installer from a different directory can cause readlink to fail and abort installation after packages were already installed.
Agent Prompt
### Issue description
The validation assumes it is run from repo root (`config/...`). With `set -e`, `readlink -f config` will abort the install if invoked from another directory, potentially leaving a partial install.
### Issue Context
The installer uses relative paths for config templates but does not `cd` to the repo root.
### Fix Focus Areas
- bin/install.sh[20-21]
- bin/install.sh[216-221]
- bin/install.sh[219-256]
### Suggested approach
- Compute repo root from script location:
- `REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd -P)"`
- `CONFIG_DIR="$REPO_ROOT/config"`
- Use `CONFIG_DIR` everywhere:
- `for template_file in "$CONFIG_DIR"/*_template.yml; do ...`
- Avoid a failing `readlink -f` under `set -e`:
- Either drop it and use `$CONFIG_DIR` in messaging,
- or guard: `config_dir=$(readlink -f "$CONFIG_DIR" 2>/dev/null || echo "$CONFIG_DIR")`
- Optionally skip validation with a clear warning if `$CONFIG_DIR` doesn’t exist.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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