refactor(cli): extract upgrade sandbox helpers#2963
Conversation
This reverts commit 4ebeae4.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces three modular helper functions that encapsulate sandbox version classification, upgrade confirmation skipping, and rebuild grouping logic. The upgrade-sandboxes-action is refactored to delegate logic to these helpers. Overall behavior remains unchanged, with code organized for better testability and reusability. ChangesSandbox Upgrade Helpers Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
## Summary Extract pure Docker image parsing and orphan-selection logic from the subprocess-heavy maintenance action module. ## Stack Navigation - Position: 47 of 60 - Previous PR: [#2961 — test(cli): stabilize coverage dist sourcemaps](#2961) - Next PR: [#2963 — refactor(cli): extract upgrade sandbox helpers](#2963) ## Changes - Added `maintenance-image-helpers.ts` for Docker image row parsing and registered image-tag comparison. - Updated `garbageCollectImages` to use the extracted helpers while keeping Docker/prompt orchestration in `maintenance-actions.ts`. - Added direct helper coverage for image parsing, registered tag collection, and orphan detection. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
…/retry-upgrade-sandbox-helpers
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
prekshivyas
left a comment
There was a problem hiding this comment.
LGTM. Same pattern as #2962 — three pure helpers extracted from upgradeSandboxes into upgrade-sandboxes-helpers.ts:
shouldSkipUpgradeConfirmation— wraps the auto-or-yes check.classifyUpgradeableSandboxes— replaces the inline classification for-loop, withcheckVersioninjected as a dep (clean DI for test).splitRebuildableSandboxes— replaces the two.filter()calls.
Inline blocks fully replaced; behavior verbatim. The checkVersion injection point is a strict improvement over the previous direct call — same default, but testable.
+59 lines of direct unit tests covering confirmation bypass (auto/yes/check), classification (stale/running, stale/stopped, unknown, current), and rebuildable/stopped split.
CI: pr.yaml fully green (lint/dco/check-hash/legacy-path-guard/macos-e2e/changes/checks PASS); test-e2e-ollama-proxy + pr-self-hosted builds still in flight at review time. No failures.
## Summary Extract pure sandbox destroy decision helpers from the destructive destroy action module. ## Stack Navigation - Position: 49 of 60 - Previous PR: [#2963 — refactor(cli): extract upgrade sandbox helpers](#2963) - Next PR: [#2965 — refactor(cli): extract sandbox logs helpers](#2965) ## Changes - Added `sandbox-destroy-helpers.ts` for delete outcome parsing and cleanup decision helpers. - Updated destroy and rebuild actions to import delete outcome parsing from the helper module. - Moved/added helper tests for missing-sandbox output, delete outcomes, host-service cleanup decisions, and gateway cleanup decisions. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
Extract pure upgrade sandbox classification helpers from the subprocess-heavy upgrade action module.
Stack Navigation
Changes
upgrade-sandboxes-helpers.tsfor confirmation bypass, stale/unknown classification, and rebuildable/stopped splitting.upgradeSandboxesto call the extracted helpers while keeping OpenShell and rebuild orchestration in the action module.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Refactor
Tests