chore: validate versions.json entries
#3308
Conversation
|
ohayo, sensei! WalkthroughAdds a GitHub Actions workflow to validate component versions in versions.json and a Bash script that checks tag existence across mapped repositories using GitHub API or git, failing the workflow if any required tags are missing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions (Runner)
participant Script as validate_version.sh
participant API as GitHub API (gh)
participant Git as git ls-remote
Dev->>GH: Push/PR changing versions.json or script
GH->>GH: Setup container, install jq
GH->>Script: Execute scripts/validate_version.sh versions.json
Script->>Script: Parse versions.json, map components->repos
loop For each component/version
alt gh available
Script->>API: Query releases/tags and refs
API-->>Script: Tag exists / not found
else Fallback
Script->>Git: ls-remote repo refs
Git-->>Script: Tag exists / not found
end
Script->>Script: Cache lookup result
end
alt Any missing tags
Script-->>GH: Exit 1 with summary
GH-->>Dev: Job fails
else All tags present
Script-->>GH: Exit 0
GH-->>Dev: Job succeeds
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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: 3
🧹 Nitpick comments (8)
scripts/validate_version.sh (6)
86-88: Quote trap command to avoid premature expansion (SC2064).Use single quotes and quote the variable within to defer expansion until trap execution.
Apply this diff:
-CACHE_FILE=$(mktemp) -trap "rm -f $CACHE_FILE" EXIT +CACHE_FILE=$(mktemp) +trap 'rm -f "$CACHE_FILE"' EXIT
110-114: Avoid masking return values; also use fixed-string grep for cache key.
- Declare
resultseparately (SC2155).- Use
grep -Fto treat the cache key literally (dots and pipes appear in${repo}|${tag}).Apply this diff:
- # 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 + if grep -Fq "^${key}=" "$CACHE_FILE" 2>/dev/null; then + local result + result=$(grep -F "^${key}=" "$CACHE_FILE" | cut -d= -f2) + return "$result" + fi
131-136: Tighten ls-remote check; current grep pattern is redundant.You already filter by ref “refs/tags/$tag”, so matching again for 'refs/tags/' is redundant. Prefer a simple non-empty output check.
Apply this diff:
- 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" | grep -q . ; then echo "${key}=0" >> "$CACHE_FILE" return 0 fi
64-77: Hard-coded components reduce flexibility; derive from COMPONENT_REPOS.The jq query only considers ["katana","torii"]. If COMPONENT_REPOS includes more components (or different names), they’re silently ignored. Consider deriving component names from COMPONENT_REPOS to keep the validator generic.
I can provide a follow-up refactor to pass COMPONENT_REPOS keys into jq (via --argjson) and emit TSV rows per ($version, $component, $ver), avoiding per-iteration jq calls later. Want me to draft it?
148-166: Avoid per-iteration jq to rediscover the version key (performance/readability).You can include
$versionin the initial TSV from jq and read three fields in the loop. This removes repeated file parsing and simplifies the loop logic.I can propose a concrete patch that outputs
$version\t$component\t$verfrom jq and updates the loop accordingly. Shall I draft it?
13-24: Default mapping is sane; document how to extend.Defaults for katana/torii are good. Consider adding a short comment showing how to add more mappings via COMPONENT_REPOS.
.github/workflows/validate-versions-existence.yml (2)
3-8: Consider adding a manual dispatch for ad-hoc runs.Optional: add
workflow_dispatchso maintainers can re-run validation without pushing changes.Example:
on:
push:
paths: [...]
pull_request:
paths: [...]
workflow_dispatch: {}
1-27: Repo-level consistency: file naming uses hyphen in workflow but underscore in script.You fixed this via the diffs above; ensure future scripts follow a consistent naming convention (either hyphen or underscore) across repo to avoid similar slips.
📜 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 (2)
.github/workflows/validate-versions-existence.yml(1 hunks)scripts/validate_version.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/validate_version.sh
[warning] 87-87: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 112-112: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (13)
scripts/validate_version.sh (9)
116-129: Good multi-strategy tag check using gh Releases then git refs.Nice progressive fallback within the GitHub API path. With the cache in place, this should be efficient.
145-166: Nice UX: per-version headers and clear progress.The version header printing makes CI logs readable. Good touch.
175-189: Validation loop looks solid; both 'v' and '' are checked with caching.This should minimize API hits while catching the common tag naming conventions.
45-58: Graceful dependency checks with actionable errors.jq and git checks with explicit errors are helpful. gh optionality is well handled.
143-199: End-to-end flow and failure semantics are correct.Clear summary, non-zero exit on missing versions, and success message are all aligned with CI usage.
1-3: Shell options: great defaults.
set -Eeuo pipefailis appropriate here.
59-63: Input file validation is correct.Early exit with clear error when versions.json is missing is good.
84-90: mktemp + cleanup is good; minor tweak applied above for trap quoting.The cache temp file and cleanup approach is appropriate for portability.
91-141: Cache key contains regex metacharacters; using grep -F (above) will prevent false matches.Once you apply the
-Ftweak, this function becomes robust.Run the script locally against a small synthetic versions.json to confirm both cached-hit and cache-miss paths behave as expected after the change.
.github/workflows/validate-versions-existence.yml (4)
22-25: YAML block indentation is preserved in env; script must trim key whitespace (fixed in get_repo).Given the ENV block’s indentation, COMPONENT_REPOS lines include leading spaces. After applying the
get_repotrim refactor, this will work. Alternatively, left-align values to avoid leading spaces.Please confirm the container prints the parsed mapping correctly by adding a temporary debug step:
- name: Debug COMPONENT_REPOS
run: printf '%s\n' "$COMPONENT_REPOS"
14-17: Container choice is fine; ensure jq and git are present (gh optional).The script falls back to git if gh is absent. If jq or git are missing, the job will fail early. Confirm the image includes them.
I can add a preflight step to print tool versions:
- run: |
jq --version
git --version
command -v gh && gh --version || echo "gh not installed (fallback to git)"
1-11: Workflow scope and permissions are appropriate.Minimal read permissions and targeted triggers keep this safe and fast.
18-25: Nice: explicit mapping via env keeps workflow decoupled.Keeping mappings in env avoids hardcoding inside the script. Now that the parser trims, multiline blocks are safe.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/validate_version.sh (1)
26-41: Support env-based repo mapping (COMPONENT_REPOS) with whitespace tolerance and no subshells.Ohayo, sensei — consider allowing overrides via COMPONENT_REPOS so new components can be mapped without code changes. Trim whitespace and avoid subshell pitfalls during parsing.
Apply this refactor:
get_repo() { - local component="$1" - - case "$component" in - katana) - echo "$KATANA_REPO" - ;; - torii) - echo "$TORII_REPO" - ;; - *) - # Return empty for unknown components - echo "" - ;; - esac + local component="$1" + local line k v + + # Prefer explicit mapping via COMPONENT_REPOS (newline- or comma-separated k=v entries) + if [[ -n "${COMPONENT_REPOS:-}" ]]; then + while IFS= read -r line; do + # Skip empty/comment lines + [[ -z "${line//[[:space:]]/}" ]] && continue + [[ "$line" =~ ^[[:space:]]*# ]] && continue + IFS='=' read -r k v <<< "$line" + # Normalize key and value + k="${k//[[:space:]]/}" + v="${v#"${v%%[![:space:]]*}"}" + v="${v%"${v##*[![:space:]]}"}" + if [[ "$k" == "$component" ]]; then + echo "$v" + return 0 + fi + done <<< "${COMPONENT_REPOS//$','/$'\n'}" + fi + + # Defaults + case "$component" in + katana) echo "$KATANA_REPO" ;; + torii) echo "$TORII_REPO" ;; + *) echo "" ;; # unknown component => no mapping + esac }
🧹 Nitpick comments (5)
scripts/validate_version.sh (5)
66-75: Derive components dynamically and emit version in TSV to remove O(n²) jq inside the loop.Ohayo, sensei — this makes the script future-proof for new components in versions.json and avoids re-parsing JSON for each pair.
Apply this change:
-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" -) +pairs=$( + jq -r ' + # Iterate through each version entry, discover components dynamically + to_entries[] + | .key as $version + | .value as $obj + | $obj | keys[] as $component + | ($obj[$component] // empty)[] + | [$version, $component, .] | @tsv + ' "$VERSION_REGISTRY_FILE" +)And refactor the loop and header logic:
-while IFS=$'\t' read -r comp ver; do - # Parse the version from the component-version pair - # The pairs are generated from jq with the parent version context - # We'll need to extract it from the JSON structure - version_key=$(jq -r --arg comp "$comp" --arg ver "$ver" ' - to_entries[] - | select(.value[$comp] // [] | contains([$ver])) - | .key - | select(. != null) - | . + "" - ' "$VERSION_REGISTRY_FILE" | head -1) - - # Print version header when it changes - if [[ -n "$version_key" ]] && [[ "$version_key" != "$current_version" ]]; then - current_version="$version_key" - echo "" - echo "Validating version: $current_version" - fi +while IFS=$'\t' read -r version_key comp ver; do + # Print version header when it changes + if [[ -n "$version_key" ]] && [[ "$version_key" != "$current_version" ]]; then + current_version="$version_key" + echo "" + echo "Validating version: $current_version" + fiAlso applies to: 146-164
85-85: Fix trap quoting (ShellCheck SC2064).Ohayo, sensei — expand the variable at trap time, not at definition.
-trap "rm -f $CACHE_FILE" EXIT +trap 'rm -f "$CACHE_FILE"' EXIT
108-113: Harden cache lookup: use fixed-string grep, avoid double grep, and address SC2155.Ohayo, sensei — make pattern matching literal (no regex surprises with dots), read only one line, and split declare/assign.
- if grep -q "^${key}=" "$CACHE_FILE" 2>/dev/null; then - local result=$(grep "^${key}=" "$CACHE_FILE" | cut -d= -f2) - return "$result" - fi + if grep -Fq "^${key}=" "$CACHE_FILE" 2>/dev/null; then + local result + result="$(grep -F "^${key}=" "$CACHE_FILE" | head -n1 | cut -d= -f2)" + return $result + fi
167-171: Offer actionable guidance when repo mapping is missing.Ohayo, sensei — consider hinting how to provide a mapping to reduce CI friction (especially if new components are introduced).
- echo "error: no repo mapping for component '$comp'." >&2 + echo "error: no repo mapping for component '$comp'." >&2 + echo " Provide it via COMPONENT_REPOS (e.g., 'katana=dojoengine/katana') or extend defaults in the script." >&2
129-134: Nit: simplify ls-remote fallback check.Ohayo, sensei — not required, but you can rely on non-empty output rather than matching the path again.
- 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" | grep -q . ; then echo "${key}=0" >> "$CACHE_FILE" return 0 fi
📜 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 (2)
.github/workflows/validate-versions-existence.yml(1 hunks)scripts/validate_version.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/validate-versions-existence.yml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/validate_version.sh
[warning] 85-85: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 110-110: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (2)
scripts/validate_version.sh (2)
1-12: Solid scaffolding and guardrails.Ohayo, sensei — shebang, strict mode, and clear requirements are on point. Good baseline for a reliable CI utility.
189-197: Nice summary and failure reporting.Ohayo, sensei — clear output and non-zero exit on missing versions will make CI diagnostics straightforward. ✅
Introduces a new workflow for ensuring that the versions listed in the
versions.jsonfile actually exist.Summary by CodeRabbit