refactor: simplify validation script - #3310
Conversation
|
ohayo, sensei! WalkthroughThe script validate_version.sh was refactored to iterate registry versions directly, resolve component repos with stricter error handling, check both v{version} and {version} tags per component using cached lookups, print uniform per-version headers and colorized statuses, collect missing tags, and exit on unknown components. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Script as validate_version.sh
participant Registry as registry.json (jq)
participant Repo as git remote(s)
User->>Script: Run validation
Script->>Registry: Enumerate versions (keys_unsorted[])
loop For each version
Script->>Registry: List components for version
loop For each component
Script->>Script: get_repo(component)
alt Unknown component
Script-->>User: Print error to stderr
Script->>Script: exit 1
else Known component
Script->>Repo: check_tag_exists v{comp_version} (cached)
Script->>Repo: check_tag_exists {comp_version} (cached)
Script-->>User: Print status (✓/✗), repo shown inline
Script->>Script: Track missing tags if any
end
end
end
alt Missing tags collected
Script-->>User: Report missing component versions
Script->>Script: exit non-zero
else All exist
Script-->>User: Confirm all component versions exist
Script->>Script: exit 0
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/validate_version.sh (1)
111-116: Fix cache lookup: unescaped regex (|) can corrupt matches; use awk or escape the key. Ohayo, sensei!The grep checks treat “|” in "${repo}|${tag}" as regex alternation, so keys like owner/repo|v1.2.3 can mis-match. This leads to false cache hits/misses.
Apply this diff to make the lookup exact and regex-safe:
- # Check cache to avoid redundant lookups - if grep -q "^${key}=" "$CACHE_FILE" 2>/dev/null; then - local result=$(grep "^${key}=" "$CACHE_FILE" | cut -d= -f2) - return "$result" - fi + # Check cache to avoid redundant lookups (exact match on key before '=') + local result + if result=$(awk -F= -v k="$key" '$1==k{print $2; exit 0} END{exit 1}' "$CACHE_FILE"); then + return "$result" + fi
🧹 Nitpick comments (9)
scripts/validate_version.sh (9)
88-88: Quote cache path in trap to handle spaces.Minor, but robust: quote the path inside trap so it’s safe even if mktemp returns a path with spaces.
-trap "rm -f $CACHE_FILE" EXIT +trap 'rm -f "$CACHE_FILE"' EXIT
149-151: Quote the registry filename in jq invocation.Avoid word-splitting/globbing issues if the filename contains spaces.
-all_versions=$(jq -r 'keys_unsorted[]' $VERSION_REGISTRY_FILE) +all_versions=$(jq -r 'keys_unsorted[]' "$VERSION_REGISTRY_FILE")
146-148: Remove unused variablecurrent_version.It’s defined but never used after the refactor.
-# We need to track the current version being validated -current_version="" -
166-181: Harden jq lookups and quote filename; handle missing arrays gracefully.If a component key exists but is null/empty, jq will error on [] without default. Also quote the filename.
- # iterate over the current component's versions - comp_versions=$(jq -r ".\"${version}\".${comp}[]" $VERSION_REGISTRY_FILE) + # iterate over the current component's versions (default to [] if missing) + comp_versions=$(jq -r ".\"${version}\".${comp} // [] | .[]" "$VERSION_REGISTRY_FILE")Optionally, make interpolation safer and avoid shell quoting pitfalls:
- comp_versions=$(jq -r ".\"${version}\".${comp} // [] | .[]" "$VERSION_REGISTRY_FILE") + comp_versions=$(jq -r --arg v "$version" --arg c "$comp" '.[$v][$c] // [] | .[]' "$VERSION_REGISTRY_FILE")
65-84: Remove legacy ‘pairs’ scan; it’s unused post-refactor and parses JSON twice.This block only serves an emptiness check and hardcodes component names, which is redundant now. Prefer checking all_versions instead.
-# Parse versions.json which has the format: -# { "<version>": { "katana": [...], "torii": [...] }, ... } -# Each key is a version, and the value contains arrays of compatible component versions -pairs=$( - jq -r ' - # Iterate through each version entry - to_entries[] - | .key as $version - | .value - | ["katana","torii"][] as $component - | (.[$component] // empty)[] - | [$component, .] | @tsv - ' "$VERSION_REGISTRY_FILE" -) - -if [[ -z "$pairs" ]]; then - echo "error: no katana/torii versions found in $VERSION_REGISTRY_FILE" >&2 - exit 1 -fi +:Then, right after computing all_versions, add an emptiness check:
all_versions=$(jq -r 'keys_unsorted[]' "$VERSION_REGISTRY_FILE") +# nothing to validate? +if [[ -z "$all_versions" ]]; then + echo "error: no versions found in $VERSION_REGISTRY_FILE" >&2 + exit 1 +fi
40-41: Confirm desired fail-fast on unknown components; consider a non-strict mode.Exiting on unknown keys is a behavior change. If the registry might include metadata keys now or later, this will break validations unexpectedly. Consider honoring an env flag:
- VALIDATE_STRICT=1 (default): exit 1
- otherwise: warn and skip.
Example tweak:
- echo "error: unknown component '$component'" >&2 - exit 1 + if [[ "${VALIDATE_STRICT:-1}" == "1" ]]; then + echo "error: unknown component '$component'" >&2 + exit 1 + else + echo "warn: skipping unknown component '$component'" >&2 + return 1 + fiIf you adopt non-strict mode, update the call site to skip components when get_repo returns non-zero:
repo=$(get_repo "$comp") || continue
132-137: Micro-optimization: ls-remote + grep is redundant.ls-remote with a specific ref returns nothing if missing; piping to grep is unnecessary. Not critical, just tidier.
- if git ls-remote --tags "https://github.com/$repo" "refs/tags/$tag" \ - | grep -qE 'refs/tags/' ; then + if git ls-remote --tags "https://github.com/$repo" "refs/tags/$tag" >/dev/null 2>&1; then echo "${key}=0" >> "$CACHE_FILE" return 0 fi
120-129: Nit: remove unnecessary-q .in gh api when discarding output.You’re already redirecting to /dev/null, so the jq filter is superfluous.
- if gh api -q . "repos/$repo/releases/tags/$tag" >/dev/null 2>&1; then + if gh api "repos/$repo/releases/tags/$tag" >/dev/null 2>&1; then echo "${key}=0" >> "$CACHE_FILE" return 0 fi @@ - if gh api -q . "repos/$repo/git/ref/tags/$tag" >/dev/null 2>&1; then + if gh api "repos/$repo/git/ref/tags/$tag" >/dev/null 2>&1; then echo "${key}=0" >> "$CACHE_FILE" return 0 fi
158-164: ohayo sensei! Validation clean – no unknown keys found inversions.json.The jq scan confirmed that each version only contains “katana” and “torii”, so the current loop won’t encounter extra metadata keys. That said, filtering to the known set remains a useful optional safeguard against future stray entries:
- # get the components for the current version - comps=$(jq -r ".[\"${version}\"] | keys_unsorted[]" "$VERSION_REGISTRY_FILE") + # get only known components for the current version + comps=$(jq -r ".[\"${version}\"] | keys_unsorted[] | select(.==\"katana\" or .==\"torii\")" "$VERSION_REGISTRY_FILE")
📜 Review details
Configuration used: .coderabbit.yaml
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 (1)
scripts/validate_version.sh(3 hunks)
1 participant
related #3308
simplify the bash script for better readability.
Summary by CodeRabbit