feat(backend): allow version_expr to post-process version_regex results#10302
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds post-processing support for version lists via a ChangesVersion Expression Post-Processing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Greptile SummaryThis PR fixes incorrect
Confidence Score: 5/5Safe to merge. The change is well-scoped, backward-compatible, and all existing tests pass alongside a new pipeline test. The core logic change — moving version_expr to a post-processing step and always injecting versions (including as an empty array when regex finds nothing) — is clean and handles edge cases correctly. No existing registry entries combine version_regex and version_expr, making the functional delta a no-op for all current tools outside oc and openshift-install. The sortVersions function is well-guarded, and the new test directly exercises the regex-then-sort pipeline. Concerns raised in prior review threads are addressed in the current code. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "chore(registry): fix taplo formatting fo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/version_list.rs`:
- Around line 135-140: The code currently only inserts "versions" into the
template context when the local `versions` Vec is non-empty, causing expression
errors like `sortVersions(versions)` when no matches occur; change the logic in
the block around `versions` / `ctx.insert` so that the "versions" key is always
bound in `ctx` (insert a Value::Array of the mapped strings even if `versions`
is empty, or insert an explicit empty Value::Array in the else branch) so
downstream expressions receive an empty array instead of being missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 127447e3-0ef6-44c3-b4dc-cad6fa40165f
📒 Files selected for processing (3)
registry/oc.tomlregistry/openshift-install.tomlsrc/backend/version_list.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/version_list.rs (1)
144-164:⚠️ Potential issue | 🟠 MajorFix guideline violation in
sortVersions: removeversions::Versioning::new(...)-based sorting
src/backend/version_list.rsimplementssortVersionsby callingversions.sort_by_cached_key(|v| Versioning::new(v)), which reorders a version list using a semver comparator and conflicts with the guideline to avoidVersioning::new/semver comparators at new call sites for version-list sorting.- Delegate ordering back to the backend/tool resolution flow (e.g., via
Backend::latest_version,Backend::latest_installed_version,Backend::list_versions_matching, orToolRequest::resolve) instead of sorting here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/version_list.rs` around lines 144 - 164, The sortVersions host function must not perform semver-based sorting using Versioning::new; remove the versions.sort_by_cached_key(|v| Versioning::new(v)) call and any direct uses of Versioning::new inside the sortVersions closure. Instead, return the input array unchanged (preserving caller order) or delegate sorting/resolution to the backend/tool resolution APIs (e.g., Backend::latest_version, Backend::latest_installed_version, Backend::list_versions_matching, or ToolRequest::resolve) so that the backend controls ordering; ensure the closure no longer references Versioning::new and add a short comment stating that ordering is delegated to the backend.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/backend/version_list.rs`:
- Around line 144-164: The sortVersions host function must not perform
semver-based sorting using Versioning::new; remove the
versions.sort_by_cached_key(|v| Versioning::new(v)) call and any direct uses of
Versioning::new inside the sortVersions closure. Instead, return the input array
unchanged (preserving caller order) or delegate sorting/resolution to the
backend/tool resolution APIs (e.g., Backend::latest_version,
Backend::latest_installed_version, Backend::list_versions_matching, or
ToolRequest::resolve) so that the backend controls ordering; ensure the closure
no longer references Versioning::new and add a short comment stating that
ordering is delegated to the backend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 838b8998-0f1a-4213-855a-8dd564439df1
📒 Files selected for processing (1)
src/backend/version_list.rs
|
nice solution! |
2 participants
Summary
version_exprandversion_regexto work together as a pipeline: regex extracts versions, then expr post-processes them (e.g. sorting)if/else ifchain (feat(http): add version_expr support and Tera templating #7723). This PR allows combining them, but if there was a reason for keeping them separate, happy to take a different approach.ocandopenshift-installversion sorting by addingsortVersions(versions)to their registry entriesMotivation
The upstream HTML directory at
mirror.openshift.comlists versions lexicographically.find_match_in_listreturns the last element assuming ascending order, solatestresolves to4.9.9instead of4.22.0.A registry-side fix was the only option because:
mirror.openshift.com, not GitHub Releases, so switching toaqua:orgithub:is not possibleversion_expralone can't extract from HTML — expr-lang has no regex capture function, andversion_regex/version_exprwere mutually exclusiveChanges
src/backend/version_list.rs: Moveversion_exprevaluation to run after extraction. When bothversion_regexandversion_exprare specified, the regex results are passed as aversionsvariable into the expr context. Whenversion_expris used alone, it operates onbodyas before. No existing registry entries combine the two, so this is a no-op for all current tools.registry/oc.toml,registry/openshift-install.toml: Addversion_expr = 'sortVersions(versions)'Test plan
cargo test backend::version_list— 24 tests pass (including new pipeline test)mise ls-remote ocreturns versions in ascending semantic ordermise latest ocresolves to4.22.0(not4.9.9)Before (current release)
After (this PR)
Summary by CodeRabbit