feat: create a new script to mirror dynamic plugin OCI artifacts for airgapped deployments

[Fortune-Ndlovu]

Description

This PR provides a NEW script, the mirror-plugins.sh script, which enables mirroring of dynamic plugin OCI artifacts for airgapped RHDH deployments. This script is decoupled from how we install RHDH. It focuses ONLY on mirroring the dynamic plugin OCI artifacts, not on installing or configuring RHDH itself. After mirroring plugins with this script, we can then configure our RHDH deployment (operator or helm) to pull from our mirrored registry.

Which issue(s) does this PR fix or relate to

• Fixes https://issues.redhat.com/browse/RHIDP-9351

PR acceptance criteria

• Tests
• Documentation

How to test changes / Special notes to the reviewer

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ RHIDP-9351 - PR Code Verified Compliant requirements: Provide a bash script named mirror-plugins.sh placed at .rhdh/scripts/mirror-plugins.sh Support Plugin Catalog Index mode via --plugin-index to pull all plugins from a versioned catalog index Support File List mode via --plugin-list to pull plugins listed in a local text file Support Direct URL mode via --plugins to pull specific plugins by direct OCI reference Support Combined mode mixing --plugin-index with --plugins Support Export to Directory mode via --to-dir for fully disconnected export Support Import from Directory mode via --from-dir combined with --to-registry to push into a registry Provide usage/help and examples Requires further human verification: Validate against real plugin index images that dynamicArtifact parsing yields complete/accurate plugin lists across versions. Execute end-to-end in an actual disconnected environment to ensure archives and push flows work with enterprise registries and TLS. Confirm skopeo authentication behavior and REGISTRY_AUTH_FILE handling across different environments (OpenShift/Kubernetes nodes, CI).

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🔒 Security concerns TLS verification disabled: skopeo operations use --dest-tls-verify=false for pushes to registries, weakening transport security. Make TLS verification configurable and default to secure settings.

⚡ Recommended focus areas for review Possible Issue Catalog index resolution assumes the largest extracted layer contains package YAMLs and greps for 'dynamicArtifact' lines; this may be brittle if structure changes or YAML formatting varies (e.g., multi-line values). Verify correctness across index versions. # Plugin resolution functions function resolve_plugin_index() { local index_url="$1" infof "Resolving plugins from catalog index: $index_url" # Extract registry and version from OCI URL if [[ "$index_url" =~ oci://([^:]+):(.+) ]]; then local registry="${BASH_REMATCH[1]}" local version="${BASH_REMATCH[2]}" debugf "Extracting plugin catalog index image: $registry:$version" # Create temporary directory for extracting the catalog index local temp_dir temp_dir=$(mktemp -d) # Cleanup temp directory when function returns trap "rm -rf '$temp_dir'" RETURN # Extract the catalog index image if ! skopeo copy "docker://$registry:$version" "dir:$temp_dir/catalog-index" 2>/dev/null; then errorf "Failed to extract catalog index image: $registry:$version" return 1 fi # Find the largest layer (likely contains the catalog data) local catalog_layer catalog_layer=$(find "$temp_dir/catalog-index" -type f -not -name "manifest.json" -not -name "version" -exec ls -la {} \; | sort -k5 -nr | head -1 | awk '{print $NF}') if [[ -z "$catalog_layer" ]]; then errorf "No catalog data layer found in index image" return 1 fi debugf "Using catalog layer: $(basename "$catalog_layer")" # Extract the catalog layer local catalog_data_dir="$temp_dir/catalog-data" mkdir -p "$catalog_data_dir" if ! tar -xf "$catalog_layer" -C "$catalog_data_dir" 2>/dev/null; then errorf "Failed to extract catalog data from layer" return 1 fi # Find all package YAML files local package_files package_files=$(find "$catalog_data_dir" -name "*.yaml" -path "*/packages/*" 2>/dev/null) if [[ -z "$package_files" ]]; then warnf "No package files found in catalog index" return 1 fi debugf "Found $(echo "$package_files" | wc -l) package files in catalog index" # Extract OCI plugin URLs from package files local temp_plugin_images=() local filtered_plugins=0 while IFS= read -r package_file; do if [[ -f "$package_file" ]]; then # Extract dynamicArtifact field that contains OCI URLs local oci_urls oci_urls=$(grep -E "^\s*dynamicArtifact:\s*oci://" "$package_file" 2>/dev/null | sed 's/.*dynamicArtifact:\s*//' | tr -d ' ') if [[ -n "$oci_urls" ]]; then # Filter by version if specified (not 'next' or 'latest') if [[ "$version" != "next" && "$version" != "latest" ]]; then # Check if the OCI URL contains the version if echo "$oci_urls" | grep -q ":$version"; then temp_plugin_images+=("$oci_urls") ((filtered_plugins++)) fi else # For 'next' or 'latest', include all OCI URLs temp_plugin_images+=("$oci_urls") ((filtered_plugins++)) fi fi fi done <<< "$package_files" # Remove duplicates if [[ ${#temp_plugin_images[@]} -gt 0 ]]; then local unique_plugins=() for plugin in "${temp_plugin_images[@]}"; do local found=false for existing in "${unique_plugins[@]}"; do if [[ "$existing" == "$plugin" ]]; then found=true break fi done if [[ "$found" == "false" ]]; then unique_plugins+=("$plugin") fi done PLUGIN_IMAGES=("${unique_plugins[@]}") fi infof "Found ${#PLUGIN_IMAGES[@]} unique plugins from catalog index" if [[ "$version" != "next" && "$version" != "latest" ]]; then debugf "Filtered to ${#PLUGIN_IMAGES[@]} plugins matching version $version" fi return 0 else errorf "Invalid OCI URL format: $index_url. Expected format: oci://registry/org/image:tag" return 1 fi } Robustness Deduplication and array handling are manual O(n^2). While fine for small sets, consider using associative arrays to prevent performance issues and simplify logic; also sanitize/normalize plugin refs consistently before dedupe. # Deduplicate plugins (remove duplicates while preserving order) PLUGIN_IMAGES=() local seen_plugins=() for plugin in "${all_plugins[@]}"; do # Check if we've seen this plugin before local is_duplicate=false for seen in "${seen_plugins[@]}"; do if [[ "$plugin" == "$seen" ]]; then is_duplicate=true break fi done if [[ "$is_duplicate" == "false" ]]; then PLUGIN_IMAGES+=("$plugin") seen_plugins+=("$plugin") else debugf "Skipping duplicate plugin: $plugin" fi done TLS/Registry Flags skopeo copy uses '--dest-tls-verify=false' unconditionally for pushes, which disables TLS verification. This may be unacceptable in secure environments; make it configurable (e.g., --tls-verify) and default to true. function mirror_image_to_registry() { local src_image="$1" local dest_image="$2" infof "Mirroring $src_image to $dest_image..." # Handle OCI URLs (remove oci:// prefix and handle subpaths) if [[ "$src_image" == oci://* ]]; then # Extract the Docker image reference (everything before the ! for subpaths) local docker_ref="${src_image%!*}" docker_ref="${docker_ref#oci://}" skopeo copy --preserve-digests --remove-signatures --all --dest-tls-verify=false "docker://$docker_ref" "docker://$dest_image" || return 1 else skopeo copy --preserve-digests --remove-signatures --all --dest-tls-verify=false "docker://$src_image" "docker://$dest_image" || return 1 fi return 0 } function mirror_image_to_archive() { local src_image="$1" local archive_path="$2" debugf "Saving $src_image to $archive_path..." # Handle OCI URLs (remove oci:// prefix and handle subpaths) if [[ "$src_image" == oci://* ]]; then # Extract the Docker image reference (everything before the ! for subpaths) local docker_ref="${src_image%!*}" docker_ref="${docker_ref#oci://}" skopeo copy --preserve-digests --remove-signatures --all "docker://$docker_ref" "dir:$archive_path" || return 1 else skopeo copy --preserve-digests --remove-signatures --all "docker://$src_image" "dir:$archive_path" || return 1 fi return 0 } function push_image_from_archive() { local archive_path="$1" local dest_image="$2" infof "Pushing $archive_path to $dest_image..." skopeo copy --preserve-digests --remove-signatures --all --dest-tls-verify=false "dir:$archive_path" "docker://$dest_image" || return 1 return 0

📄 References No matching references available

[qodo-code-review]

PR Type

(Describe updated until commit 4cc9c87)

Enhancement

---

Description

• Introduces new mirror-plugins.sh script for mirroring RHDH dynamic plugin OCI artifacts

• Supports multiple input sources: catalog index, plugin list file, or direct OCI URLs

• Enables export to directory for fully disconnected environments with import capability

• Provides installation-method agnostic solution compatible with operator and helm deployments

---

File Walkthrough

Relevant files

Enhancement

mirror-plugins.sh New script for mirroring dynamic plugin OCI artifacts        .rhdh/scripts/mirror-plugins.sh New comprehensive bash script for mirroring dynamic plugin OCI artifacts Supports three input modes: plugin catalog index, plugin list file, or direct OCI URLs Implements export-to-directory and import-from-directory workflows for disconnected environments Includes registry authentication handling, image deduplication, and detailed logging Provides extensive command-line options and usage documentation with examples +746/-0

---

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Consider reimplementing in a higher-level language The bash script is large and uses fragile methods like grep and awk to parse structured data. It should be rewritten in a higher-level language like Python for improved robustness and maintainability. Examples: .rhdh/scripts/mirror-plugins.sh [366] catalog_layer=$(find "$temp_dir/catalog-index" -type f -not -name "manifest.json" -not -name "version" -exec ls -la {} \; | sort -k5 -nr | head -1 | awk '{print $NF}') .rhdh/scripts/mirror-plugins.sh [403] oci_urls=$(grep -E "^\s*dynamicArtifact:\s*oci://" "$package_file" 2>/dev/null | sed 's/.*dynamicArtifact:\s*//' | tr -d ' ') Solution Walkthrough: Before: function resolve_plugin_index() { # ... # Heuristic to find the data layer by size using a complex command chain catalog_layer=$(find ... -exec ls -la {} \; | sort ... | head ... | awk ...) # ... tar -xf "$catalog_layer" ... package_files=$(find ... -name "*.yaml" ...) while read -r package_file; do # Parsing YAML using grep and sed is fragile oci_urls=$(grep "dynamicArtifact" "$package_file" | sed 's/...' | tr -d ' ') if [[ -n "$oci_urls" ]]; then temp_plugin_images+=("$oci_urls") fi done <<< "$package_files" # ... } After: import yaml import subprocess import os def resolve_plugin_index(index_url): # ... # Use skopeo to download the OCI artifact subprocess.run(["skopeo", "copy", ...], check=True) # ... extract layer tarball ... plugin_urls = [] for root, _, files in os.walk(catalog_data_dir): for file in files: if file.endswith(".yaml"): with open(os.path.join(root, file)) as f: # Use a proper YAML parser for robustness data = yaml.safe_load(f) if data and "dynamicArtifact" in data: url = data["dynamicArtifact"] if url.startswith("oci://"): plugin_urls.append(url) return plugin_urls Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a major architectural risk: using a large, complex bash script with fragile string/text manipulation to parse structured data like YAML is not robust or maintainable for a critical workflow.	High
General	✅ Improve performance of plugin deduplication Suggestion Impact: The commit changed the deduplication of plugin URLs to use a more efficient unique operation via sorting and uniq (mapfile with sort -u), effectively removing the O(n^2) nested loop and improving performance, aligning with the suggestion’s intent though not exactly via an associative array. code diff: - # Remove duplicates + # Remove duplicates and sort if [[ ${#temp_plugin_images[@]} -gt 0 ]]; then - local unique_plugins=() - for plugin in "${temp_plugin_images[@]}"; do - local found=false - for existing in "${unique_plugins[@]}"; do - if [[ "$existing" == "$plugin" ]]; then - found=true - break - fi - done - if [[ "$found" == "false" ]]; then - unique_plugins+=("$plugin") - fi - done - PLUGIN_IMAGES=("${unique_plugins[@]}") + mapfile -t PLUGIN_IMAGES < <(printf "%s\n" "${temp_plugin_images[@]}" | sort -u) fi infof "Found ${#PLUGIN_IMAGES[@]} unique plugins from catalog index" Improve the performance of plugin deduplication by using an associative array (hash map) instead of a nested loop, reducing the time complexity from O(n^2) to O(n). .rhdh/scripts/mirror-plugins.sh [539-558] # Deduplicate plugins (remove duplicates while preserving order) PLUGIN_IMAGES=() -local seen_plugins=() +declare -A seen_plugins for plugin in "${all_plugins[@]}"; do - # Check if we've seen this plugin before - local is_duplicate=false - for seen in "${seen_plugins[@]}"; do - if [[ "$plugin" == "$seen" ]]; then - is_duplicate=true - break - fi - done - - if [[ "$is_duplicate" == "false" ]]; then + if [[ -z "${seen_plugins[$plugin]}" ]]; then PLUGIN_IMAGES+=("$plugin") - seen_plugins+=("$plugin") + seen_plugins["$plugin"]=1 else debugf "Skipping duplicate plugin: $plugin" fi done [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: The suggestion provides a more performant O(n) algorithm using an associative array to replace the current O(n^2) nested loop for deduplication, which is a significant improvement for large datasets.	Low
	Ensure temporary directory cleanup on exit Use a more robust EXIT trap instead of RETURN for temporary directory cleanup in the resolve_plugin_index function to handle script interruptions. .rhdh/scripts/mirror-plugins.sh [355-356] # Cleanup temp directory when function returns -trap "rm -rf '$temp_dir'" RETURN +trap "rm -rf '$temp_dir'" EXIT Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion correctly points out that trap ... RETURN is not robust against signals, but using trap ... EXIT here would overwrite the script's main EXIT trap, creating another bug.	Low
Possible issue	Fix memory leak from overwritten trap Consolidate EXIT traps to prevent a temporary directory leak. The EXIT trap for TMPDIR is overwritten by another trap in the merge_registry_auth function. .rhdh/scripts/mirror-plugins.sh [243-244] # shellcheck disable=SC2064 -trap "rm -fr $TMPDIR || true" EXIT +trap "rm -fr \"${TMPDIR}\" \"${XDG_RUNTIME_DIR:-}\" || true" EXIT Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a resource leak where an EXIT trap is overwritten, but the provided improved_code is incorrect and would not fix the issue.	Low
Update

[rhdh-qodo-merge]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ RHIDP-9351 - PR Code Verified Compliant requirements: Provide a bash script named mirror-plugins.sh under .rhdh/scripts/ Mirror dynamic plugin OCI artifacts for airgapped RHDH deployments Support Plugin Catalog Index mode Support File List mode Support Direct URL mode Support Combined mode Support Export to Directory mode Support Import from Directory mode Provide usage/help and examples Handle validation/errors appropriately Requires further human verification: Validate the plugin catalog index parsing against real catalog images across versions (e.g., 1.7, 1.8, latest) to ensure dynamicArtifact extraction is reliable. End-to-end test in an actual disconnected environment, including transfer of the export directory and pushing to a private registry with auth/SSL specifics. Confirm integration with downstream RHDH deployment configuration docs and any operator/helm references.

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🔒 Security concerns Insecure transport: The script uses --dest-tls-verify=false in skopeo copy when pushing to registries, which disables TLS verification and could allow MITM attacks. Make this configurable (e.g., --tls-verify true/false) and default to true.

⚡ Recommended focus areas for review Possible Issue Catalog index resolution assumes a specific image layout and picks the largest layer as data, then greps YAML for dynamicArtifact. This may break if the image format changes or if YAML formatting/indentation differs. Consider using skopeo inspect or robust YAML parsing to avoid fragile grep/sed logic. # Plugin resolution functions function resolve_plugin_index() { local index_url="$1" infof "Resolving plugins from catalog index: $index_url" # Extract registry and version from OCI URL if [[ "$index_url" =~ oci://([^:]+):(.+) ]]; then local registry="${BASH_REMATCH[1]}" local version="${BASH_REMATCH[2]}" debugf "Extracting plugin catalog index image: $registry:$version" # Create temporary directory for extracting the catalog index local temp_dir temp_dir=$(mktemp -d) # Cleanup temp directory when function returns trap "rm -rf '$temp_dir'" RETURN # Extract the catalog index image if ! skopeo copy "docker://$registry:$version" "dir:$temp_dir/catalog-index" 2>/dev/null; then errorf "Failed to extract catalog index image: $registry:$version" return 1 fi # Find the largest layer (likely contains the catalog data) local catalog_layer catalog_layer=$(find "$temp_dir/catalog-index" -type f -not -name "manifest.json" -not -name "version" -exec ls -la {} \; | sort -k5 -nr | head -1 | awk '{print $NF}') if [[ -z "$catalog_layer" ]]; then errorf "No catalog data layer found in index image" return 1 fi debugf "Using catalog layer: $(basename "$catalog_layer")" # Extract the catalog layer local catalog_data_dir="$temp_dir/catalog-data" mkdir -p "$catalog_data_dir" if ! tar -xf "$catalog_layer" -C "$catalog_data_dir" 2>/dev/null; then errorf "Failed to extract catalog data from layer" return 1 fi # Find all package YAML files local package_files package_files=$(find "$catalog_data_dir" -name "*.yaml" -path "*/packages/*" 2>/dev/null) if [[ -z "$package_files" ]]; then warnf "No package files found in catalog index" return 1 fi debugf "Found $(echo "$package_files" | wc -l) package files in catalog index" # Extract OCI plugin URLs from package files local temp_plugin_images=() local filtered_plugins=0 while IFS= read -r package_file; do if [[ -f "$package_file" ]]; then # Extract dynamicArtifact field that contains OCI URLs local oci_urls oci_urls=$(grep -E "^\s*dynamicArtifact:\s*oci://" "$package_file" 2>/dev/null | sed 's/.*dynamicArtifact:\s*//' | tr -d ' ') if [[ -n "$oci_urls" ]]; then # Filter by version if specified (not 'next' or 'latest') if [[ "$version" != "next" && "$version" != "latest" ]]; then # Check if the OCI URL contains the version if echo "$oci_urls" | grep -q ":$version"; then temp_plugin_images+=("$oci_urls") ((filtered_plugins++)) fi else # For 'next' or 'latest', include all OCI URLs temp_plugin_images+=("$oci_urls") ((filtered_plugins++)) fi fi fi done <<< "$package_files" # Remove duplicates if [[ ${#temp_plugin_images[@]} -gt 0 ]]; then local unique_plugins=() for plugin in "${temp_plugin_images[@]}"; do local found=false for existing in "${unique_plugins[@]}"; do if [[ "$existing" == "$plugin" ]]; then found=true break fi done if [[ "$found" == "false" ]]; then unique_plugins+=("$plugin") fi done PLUGIN_IMAGES=("${unique_plugins[@]}") fi infof "Found ${#PLUGIN_IMAGES[@]} unique plugins from catalog index" if [[ "$version" != "next" && "$version" != "latest" ]]; then debugf "Filtered to ${#PLUGIN_IMAGES[@]} plugins matching version $version" fi return 0 else errorf "Invalid OCI URL format: $index_url. Expected format: oci://registry/org/image:tag" return 1 fi TLS/Registry Flags skopeo copy uses --dest-tls-verify=false by default when pushing to registries. This disables TLS verification and may mask cert issues. Consider making TLS verification configurable via a flag and default to secure behavior. function mirror_image_to_registry() { local src_image="$1" local dest_image="$2" infof "Mirroring $src_image to $dest_image..." # Handle OCI URLs (remove oci:// prefix and handle subpaths) if [[ "$src_image" == oci://* ]]; then # Extract the Docker image reference (everything before the ! for subpaths) local docker_ref="${src_image%!*}" docker_ref="${docker_ref#oci://}" skopeo copy --preserve-digests --remove-signatures --all --dest-tls-verify=false "docker://$docker_ref" "docker://$dest_image" || return 1 else skopeo copy --preserve-digests --remove-signatures --all --dest-tls-verify=false "docker://$src_image" "docker://$dest_image" || return 1 fi return 0 } function mirror_image_to_archive() { local src_image="$1" local archive_path="$2" debugf "Saving $src_image to $archive_path..." # Handle OCI URLs (remove oci:// prefix and handle subpaths) if [[ "$src_image" == oci://* ]]; then # Extract the Docker image reference (everything before the ! for subpaths) local docker_ref="${src_image%!*}" docker_ref="${docker_ref#oci://}" skopeo copy --preserve-digests --remove-signatures --all "docker://$docker_ref" "dir:$archive_path" || return 1 else skopeo copy --preserve-digests --remove-signatures --all "docker://$src_image" "dir:$archive_path" || return 1 fi return 0 } function push_image_from_archive() { local archive_path="$1" local dest_image="$2" infof "Pushing $archive_path to $dest_image..." skopeo copy --preserve-digests --remove-signatures --all --dest-tls-verify=false "dir:$archive_path" "docker://$dest_image" || return 1 return 0 }

📚 Focus areas based on broader codebase context Auth File Handling The script overrides XDG_RUNTIME_DIR and REGISTRY_AUTH_FILE and sets a trap to remove the new XDG dir, potentially clobbering or conflicting with existing auth contexts. Consider explicit existence checks, avoiding unconditional cleanup of possibly shared dirs, and honoring a user-provided REGISTRY_AUTH_FILE without copying or altering it. (Ref 3) # Handle OCI URLs (remove oci:// prefix and handle subpaths) if [[ "$src_image" == oci://* ]]; then # Extract the Docker image reference (everything before the ! for subpaths) local docker_ref="${src_image%!*}" docker_ref="${docker_ref#oci://}" skopeo copy --preserve-digests --remove-signatures --all --dest-tls-verify=false "docker://$docker_ref" "docker://$dest_image" || return 1 else skopeo copy --preserve-digests --remove-signatures --all --dest-tls-verify=false "docker://$src_image" "docker://$dest_image" || return 1 fi return 0 } function mirror_image_to_archive() { local src_image="$1" local archive_path="$2" debugf "Saving $src_image to $archive_path..." # Handle OCI URLs (remove oci:// prefix and handle subpaths) if [[ "$src_image" == oci://* ]]; then # Extract the Docker image reference (everything before the ! for subpaths) local docker_ref="${src_image%!*}" docker_ref="${docker_ref#oci://}" skopeo copy --preserve-digests --remove-signatures --all "docker://$docker_ref" "dir:$archive_path" || return 1 else skopeo copy --preserve-digests --remove-signatures --all "docker://$src_image" "dir:$archive_path" || return 1 fi return 0 } function push_image_from_archive() { local archive_path="$1" local dest_image="$2" infof "Pushing $archive_path to $dest_image..." skopeo copy --preserve-digests --remove-signatures --all --dest-tls-verify=false "dir:$archive_path" "docker://$dest_image" || return 1 return 0 Reference reasoning: Other bash utilities in this repo use strict bash settings and explicit env checks (e.g., requiring TO_PSW via parameter expansion) rather than mutating global auth contexts. Following that pattern, prefer validating required auth inputs and minimizing side effects on environment files.

📄 References redhat-developer/rhdh-operator/config/profile/rhdh/plugin-infra/plugin-infra.sh [1-49] redhat-developer/rhdh-operator/config/profile/rhdh/plugin-infra/plugin-infra.sh [50-75] redhat-developer/rhdh-operator/hack/db_copy.sh [1-23] redhat-developer/rhdh-operator/config/profile/rhdh/plugin-infra/gitops-secret-setup.sh [1-35] redhat-developer/rhdh-operator/config/profile/rhdh/plugin-infra/gitops-secret-setup.sh [133-182] redhat-developer/rhdh-operator/config/profile/rhdh/plugin-infra/gitops-secret-setup.sh [87-130] redhat-developer/rhdh-operator/config/profile/rhdh/plugin-infra/gitops-secret-setup.sh [199-222] redhat-developer/rhdh-operator/config/profile/rhdh/plugin-infra/gitops-secret-setup.sh [64-87]

[rhdh-qodo-merge]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix bug in plugin list aggregation Fix a bug where combining --plugin-index and --plugin-list causes plugin lists to be overwritten by refactoring the functions to return plugins via stdout instead of modifying a global variable. .rhdh/scripts/mirror-plugins.sh [499-525] +# In resolve_plugin_index and load_plugin_list_from_file, replace `PLUGIN_IMAGES=...` with: +# ... +# printf "%s\n" "${unique_plugins[@]}" +# return 0 + +# In mirror_plugins function: # Add plugins from catalog index if [[ -n "$PLUGIN_INDEX" ]]; then debugf "Resolving plugins from index: $PLUGIN_INDEX" - local temp_plugins=() - if resolve_plugin_index "$PLUGIN_INDEX"; then - temp_plugins=("${PLUGIN_IMAGES[@]}") - all_plugins+=("${temp_plugins[@]}") - debugf "Added ${#temp_plugins[@]} plugins from catalog index" - else + local index_plugins + if ! mapfile -t index_plugins < <(resolve_plugin_index "$PLUGIN_INDEX"); then errorf "Failed to resolve plugin index: $PLUGIN_INDEX" return 1 + fi + if [ ${#index_plugins[@]} -gt 0 ]; then + all_plugins+=("${index_plugins[@]}") + debugf "Added ${#index_plugins[@]} plugins from catalog index" fi fi # Add plugins from file if [[ -n "$PLUGIN_LIST_FILE" ]]; then debugf "Loading plugins from file: $PLUGIN_LIST_FILE" - local temp_plugins=() - if load_plugin_list_from_file "$PLUGIN_LIST_FILE"; then - temp_plugins=("${PLUGIN_IMAGES[@]}") - all_plugins+=("${temp_plugins[@]}") - debugf "Added ${#temp_plugins[@]} plugins from file" - else + local file_plugins + if ! mapfile -t file_plugins < <(load_plugin_list_from_file "$PLUGIN_LIST_FILE"); then errorf "Failed to load plugin list from file: $PLUGIN_LIST_FILE" return 1 fi + if [ ${#file_plugins[@]} -gt 0 ]; then + all_plugins+=("${file_plugins[@]}") + debugf "Added ${#file_plugins[@]} plugins from file" + fi fi [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: This suggestion identifies a critical bug where using multiple plugin sources (--plugin-index and --plugin-list) results in data loss, and proposes a robust refactoring to fix it by avoiding global state modification.	High
	✅ Robustly find largest file layer Suggestion Impact: The commit removed the 'ls | awk' usage to pick the largest layer and instead iterates through layers, extracting each until index.json is found. This eliminates the fragile parsing and addresses the robustness concern, albeit via a different implementation than the suggested 'find -printf'. code diff: + local found_index=false + for layer in "$temp_dir/catalog-index"/*; do + if [[ -f "$layer" ]] && [[ ! "$layer" =~ (manifest\.json|version)$ ]]; then + debugf "Extracting layer: $(basename "$layer")" + if tar -xf "$layer" -C "$catalog_data_dir" 2>/dev/null; then + # Check if index.json exists after extracting this layer + if [[ -f "$catalog_data_dir/index.json" ]]; then + found_index=true + debugf "Found index.json in layer: $(basename "$layer")" + break + fi + fi + fi + done + + if [[ "$found_index" != "true" ]]; then + errorf "No index.json found in catalog index image" return 1 fi Replace the fragile ls | awk command for finding the largest file with a more robust find -printf command to prevent issues with special characters in filenames. .rhdh/scripts/mirror-plugins.sh [364-366] # Find the largest layer (likely contains the catalog data) local catalog_layer -catalog_layer=$(find "$temp_dir/catalog-index" -type f -not -name "manifest.json" -not -name "version" -exec ls -la {} \; | sort -k5 -nr | head -1 | awk '{print $NF}') +catalog_layer=$(find "$temp_dir/catalog-index" -type f -not -name "manifest.json" -not -name "version" -printf '%s %p\n' | sort -rn | head -n 1 | cut -d' ' -f2-) [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that parsing ls output is fragile and replaces it with a robust find -printf command, which is a best practice for handling file paths safely in shell scripts.	Medium
High-level	Rethink fragile plugin index parsing The current plugin index parsing relies on fragile heuristics like finding the largest image layer and hardcoded file paths. This should be replaced with a more robust method, such as parsing a manifest file from a well-known location within the catalog image. Examples: .rhdh/scripts/mirror-plugins.sh [366] catalog_layer=$(find "$temp_dir/catalog-index" -type f -not -name "manifest.json" -not -name "version" -exec ls -la {} \; | sort -k5 -nr | head -1 | awk '{print $NF}') .rhdh/scripts/mirror-plugins.sh [386] package_files=$(find "$catalog_data_dir" -name "*.yaml" -path "*/packages/*" 2>/dev/null) Solution Walkthrough: Before: function resolve_plugin_index(index_url) { // ... skopeo copy "docker://..." "dir:..." // Find the largest layer, assuming it contains the data catalog_layer = find ... | sort by size | head -1 // Extract the layer tar -xf catalog_layer // Find YAML files in a hardcoded 'packages' path package_files = find . -name "*.yaml" -path "*/packages/*" // Parse YAML with grep/sed to find plugin URLs for file in package_files: oci_urls = grep "dynamicArtifact" file | sed ... // ... // ... } After: function resolve_plugin_index(index_url) { // ... skopeo copy "docker://..." "dir:..." // Check for a well-known manifest file if file_exists("catalog-index/plugins.json"): // Parse the manifest directly plugin_urls = jq '.plugins[].url' "catalog-index/plugins.json" else: // Fallback to old method or error out error "Manifest file not found in catalog index." // ... } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant fragility in the resolve_plugin_index function, which relies on brittle heuristics that could easily break, making the --plugin-index feature unreliable.	Medium
General	✅ Improve performance of plugin deduplication Suggestion Impact: The commit changed plugin deduplication from a nested loop to a more efficient single-pass approach by piping through sort -u, achieving the intended performance improvement (though not via an associative array). code diff: - # Remove duplicates + # Remove duplicates and sort if [[ ${#temp_plugin_images[@]} -gt 0 ]]; then - local unique_plugins=() - for plugin in "${temp_plugin_images[@]}"; do - local found=false - for existing in "${unique_plugins[@]}"; do - if [[ "$existing" == "$plugin" ]]; then - found=true - break - fi - done - if [[ "$found" == "false" ]]; then - unique_plugins+=("$plugin") - fi - done - PLUGIN_IMAGES=("${unique_plugins[@]}") + mapfile -t PLUGIN_IMAGES < <(printf "%s\n" "${temp_plugin_images[@]}" | sort -u) fi Replace the nested loop for plugin deduplication with a more efficient method using an associative array to improve performance. .rhdh/scripts/mirror-plugins.sh [539-558] # Deduplicate plugins (remove duplicates while preserving order) PLUGIN_IMAGES=() -local seen_plugins=() +declare -A seen_plugins for plugin in "${all_plugins[@]}"; do - # Check if we've seen this plugin before - local is_duplicate=false - for seen in "${seen_plugins[@]}"; do - if [[ "$plugin" == "$seen" ]]; then - is_duplicate=true - break - fi - done - - if [[ "$is_duplicate" == "false" ]]; then + if [[ -z "${seen_plugins[$plugin]}" ]]; then PLUGIN_IMAGES+=("$plugin") - seen_plugins+=("$plugin") + seen_plugins["$plugin"]=1 else debugf "Skipping duplicate plugin: $plugin" fi done [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies an inefficient O(n^2) deduplication loop and provides a more performant O(n) solution using an associative array, improving script execution time for large plugin lists.	Low
More

[rm3l]

/review

[rhdh-qodo-merge]

You are above your monthly Qodo Merge usage quota. For more information, please visit here.

[rm3l]

It seems to be working well, handling the different disconnected scenarios. A few comments below.

[Fortune-Ndlovu]

cc/ @rm3l @nickboldt look good to me!

[rm3l]

LGTM - thanks!

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rm3l

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [rm3l]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment