NC | Remove error on No upgrade required #9189
Conversation
Signed-off-by: jackyalbo <jacky.albo@gmail.com>
WalkthroughA new response code Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant UpgradeManager
participant ManageCLIResponse
CLI->>UpgradeManager: start_config_dir_upgrade()
UpgradeManager->>UpgradeManager: Check config dir versions
alt Versions match (no upgrade needed)
UpgradeManager->>ManageCLIResponse: Get NoUpgradeRequired
UpgradeManager-->>CLI: { code: NoUpgradeRequired, upgrade_info: { detail: ... } }
else Versions differ (upgrade needed)
UpgradeManager->>UpgradeManager: Perform upgrade
UpgradeManager-->>CLI: { code: UpgradeSuccessful, upgrade_info: ... }
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 comments)
Other keywords and placeholders
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: 4
🧹 Nitpick comments (2)
src/test/integration_tests/nc/cli/test_cli_upgrade.test.js (1)
369-372: Assert NoUpgradeRequired (LGTM) — consider normalizing exec_manage_cli parsingThe new assertions match the updated behavior. Consider unifying result parsing across tests (res vs res.stdout) to reduce confusion, unless exec_manage_cli intentionally returns different shapes for success vs error.
src/test/unit_tests/nc/configuration/test_nc_upgrade_manager.test.js (1)
366-368: Clarify invalid host shape in testUsing an empty array for the host object works to produce undefined current_version, but it’s atypical and can hide structure issues. Prefer an empty object to keep schema clear.
- const system_data = { - [hostname]: [] - }; + const system_data = { + [hostname]: {} + };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/manage_nsfs/manage_nsfs_cli_responses.js(1 hunks)src/manage_nsfs/upgrade.js(1 hunks)src/test/integration_tests/nc/cli/test_cli_upgrade.test.js(5 hunks)src/test/unit_tests/nc/configuration/test_nc_upgrade_manager.test.js(12 hunks)src/upgrade/nc_upgrade_manager.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit Configuration File
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/unit_tests/nc/configuration/test_nc_upgrade_manager.test.jssrc/test/integration_tests/nc/cli/test_cli_upgrade.test.js
🧬 Code Graph Analysis (3)
src/manage_nsfs/manage_nsfs_cli_responses.js (3)
src/cmd/manage_nsfs.js (1)
ManageCLIResponse(28-28)src/manage_nsfs/diagnose.js (1)
ManageCLIResponse(12-12)src/manage_nsfs/manage_nsfs_logging.js (1)
ManageCLIResponse(10-10)
src/manage_nsfs/upgrade.js (1)
src/cmd/manage_nsfs.js (1)
ManageCLIResponse(28-28)
src/test/integration_tests/nc/cli/test_cli_upgrade.test.js (2)
src/upgrade/nc_upgrade_manager.js (1)
hostname(14-14)src/test/unit_tests/nc/configuration/test_nc_upgrade_manager.test.js (1)
hostname(34-34)
🔇 Additional comments (20)
src/manage_nsfs/manage_nsfs_cli_responses.js (1)
157-161: Add NoUpgradeRequired response (LGTM) — confirm event mapping intentConstant is well-formed and consistent with existing responses. Please confirm that emitting no “success” event for this scenario is intentional (it’s not mapped in NSFS_CLI_SUCCESS_EVENT_MAP). If you do want an event, consider adding a distinct mapping and event type.
src/upgrade/nc_upgrade_manager.js (2)
12-12: Import ManageCLIResponse (LGTM)Needed for the new response code.
241-241: JSDoc formatting update (LGTM)No functional impact.
src/test/integration_tests/nc/cli/test_cli_upgrade.test.js (3)
79-86: Formatting-only change (LGTM)No behavior change.
107-114: Formatting-only change (LGTM)No behavior change.
134-149: Test data formatting adjustments (LGTM)Structure and values are preserved; just condensed formatting.
src/test/unit_tests/nc/configuration/test_nc_upgrade_manager.test.js (14)
16-21: Import formatting (LGTM)Readability improved, no functional change.
24-30: Import formatting (LGTM)Consistent style, no behavior change.
38-38: Template string formatting (LGTM)No behavior change.
49-49: Template string formatting (LGTM)No behavior change.
60-60: Template string formatting (LGTM)No behavior change.
377-379: Hosts obj shape (LGTM)Top-level host entries are correct.
387-390: Hosts obj shape (LGTM)Top-level host entries are correct.
436-438: Test data formatting (LGTM)No behavior change.
571-571: Spread options (LGTM)Clearer and equivalent.
797-797: Adapt to new return shape (LGTM)Using res.upgrade_info matches the manager changes.
809-812: Assert “no upgrade required” message (LGTM)Matches manager behavior.
849-849: Adapt to new return shape (LGTM)Prepending res.upgrade_info to successful_upgrades matches new contract.
869-870: Destructuring formatting (LGTM)Readability improvement.
895-902: Destructuring formatting (LGTM)Readability improvement.
| write_stdout_response( | ||
| upgrade_res.code || ManageCLIResponse.UpgradeSuccessful, | ||
| upgrade_res.upgrade_info, { expected_version, expected_hosts } | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Dynamic CLI response (LGTM) — consider not emitting “started” event when no upgrade is required
The stdout handling looks correct. However, we currently emit CONFIG_DIR_UPGRADE_STARTED before learning that no upgrade is needed. To avoid misleading events, gate (or defer) the “started” event to cases where an upgrade actually runs.
If you adopt a consistent return shape with a code on success, this becomes straightforward:
- new NoobaaEvent(NoobaaEvent.CONFIG_DIR_UPGRADE_STARTED).create_event(undefined, { expected_version, expected_hosts }, undefined);
-
validate_expected_version(expected_version, skip_verification);
if (!expected_hosts) dbg.warn('expected_hosts flag is empty, config dir upgrade will be performed without hosts version verification');
const nc_upgrade_manager = new NCUpgradeManager(config_fs, { custom_upgrade_scripts_dir });
const upgrade_res = await nc_upgrade_manager.upgrade_config_dir(expected_version, expected_hosts, { skip_verification });
if (!upgrade_res) throw new Error('Upgrade config directory failed', { cause: upgrade_res });
+ // Emit "started" only when an upgrade actually starts (i.e., not NoUpgradeRequired)
+ if ((upgrade_res.code?.code || ManageCLIResponse.UpgradeSuccessful.code) === ManageCLIResponse.UpgradeSuccessful.code) {
+ new NoobaaEvent(NoobaaEvent.CONFIG_DIR_UPGRADE_STARTED).create_event(undefined, { expected_version, expected_hosts }, undefined);
+ }
write_stdout_response(
upgrade_res.code || ManageCLIResponse.UpgradeSuccessful,
upgrade_res.upgrade_info, { expected_version, expected_hosts }
);🏁 Script executed:
#!/bin/bash
# Search for CONFIG_DIR_UPGRADE_STARTED and show context in src/manage_nsfs/upgrade.js
rg -n "CONFIG_DIR_UPGRADE_STARTED" -A3 -B3 src/manage_nsfs/upgrade.js || true
# Also display lines 1-100 of the file for broader context
sed -n '1,100p' src/manage_nsfs/upgrade.jsLength of output: 5408
🏁 Script executed:
#!/bin/bash
# Locate manage_nsfs_cli_responses.js
rg -l "UpgradeSuccessful" -g "src/**/manage_nsfs_cli_responses.js"
# Show the definition of ManageCLIResponse.UpgradeSuccessful
rg -n "UpgradeSuccessful" -A3 -B3 src/manage_nsfs/manage_nsfs_cli_responses.jsLength of output: 780
🏁 Script executed:
#!/bin/bash
# Show the implementation of upgrade_config_dir in NCUpgradeManager
rg -n "upgrade_config_dir" -A30 -B5 src/upgrade/nc_upgrade_manager.jsLength of output: 3530
Defer CONFIG_DIR_UPGRADE_STARTED event until an upgrade actually runs
Remove the unconditional “started” event at the top of start_config_dir_upgrade and emit it only when an upgrade is kicked off (i.e. not when there’s nothing to do).
• File: src/manage_nsfs/upgrade.js
• Remove the initial
js new NoobaaEvent(NoobaaEvent.CONFIG_DIR_UPGRADE_STARTED) .create_event(undefined, { expected_version, expected_hosts }, undefined);
• After obtaining upgrade_res, but before write_stdout_response, add a guard that skips the event when upgrade_res.code === ManageCLIResponse.NoUpgradeRequired. For example:
--- a/src/manage_nsfs/upgrade.js
+++ b/src/manage_nsfs/upgrade.js
@@ async function start_config_dir_upgrade(user_input, config_fs) {
- // early “started” event here removed
validate_expected_version(expected_version, skip_verification);
if (!expected_hosts) dbg.warn('expected_hosts flag is empty, config dir upgrade will be performed without hosts version verification');
const nc_upgrade_manager = new NCUpgradeManager(config_fs, { custom_upgrade_scripts_dir });
const upgrade_res = await nc_upgrade_manager.upgrade_config_dir(expected_version, expected_hosts, { skip_verification });
if (!upgrade_res) throw new Error('Upgrade config directory failed', { cause: upgrade_res });
+ // Emit “started” only when an actual upgrade runs (not when no upgrade is required)
+ if ((upgrade_res.code?.code || ManageCLIResponse.UpgradeSuccessful.code)
+ === ManageCLIResponse.UpgradeSuccessful.code
+ ) {
+ new NoobaaEvent(NoobaaEvent.CONFIG_DIR_UPGRADE_STARTED)
+ .create_event(undefined, { expected_version, expected_hosts }, undefined);
+ }
write_stdout_response(
upgrade_res.code || ManageCLIResponse.UpgradeSuccessful,
upgrade_res.upgrade_info, { expected_version, expected_hosts }
);🤖 Prompt for AI Agents
In src/manage_nsfs/upgrade.js around lines 58 to 61, remove the unconditional
emission of the CONFIG_DIR_UPGRADE_STARTED event at the start of
start_config_dir_upgrade. Instead, after obtaining upgrade_res but before
calling write_stdout_response, add a conditional check to emit the
CONFIG_DIR_UPGRADE_STARTED event only if upgrade_res.code is not equal to
ManageCLIResponse.NoUpgradeRequired, ensuring the event is only emitted when an
upgrade actually runs.
| const system_data = { | ||
| [hostname]: { current_version: pkg.version, other_hostname: { current_version: pkg.version } } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix nested host in system_data
In this test, other_hostname is nested under the primary host, which is likely accidental. Make both hosts top-level for consistency (similar to the other tests).
- const system_data = {
- [hostname]: { current_version: pkg.version, other_hostname: { current_version: pkg.version } }
- };
+ const system_data = {
+ [hostname]: { current_version: pkg.version },
+ other_hostname: { current_version: pkg.version }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const system_data = { | |
| [hostname]: { current_version: pkg.version, other_hostname: { current_version: pkg.version } } | |
| }; | |
| const system_data = { | |
| [hostname]: { current_version: pkg.version }, | |
| other_hostname: { current_version: pkg.version } | |
| }; |
🤖 Prompt for AI Agents
In src/test/unit_tests/nc/configuration/test_nc_upgrade_manager.test.js around
lines 406 to 408, the system_data object incorrectly nests other_hostname inside
the primary hostname object. Refactor system_data so that both hostname and
other_hostname are top-level keys within the object, each with their own
current_version property, to maintain consistency with other tests.
| const message = 'config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade'; | ||
| dbg.warn(`upgrade_config_dir: ${message}`); | ||
| return { | ||
| code: ManageCLIResponse.NoUpgradeRequired, | ||
| upgrade_info: { | ||
| detail: message | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return NoUpgradeRequired instead of throwing (LGTM) — suggest standardizing return shape
Good change aligned with PR goal. To simplify callers (and avoid fallbacks), consider always including a code on success as well, so the return shape is consistent in all cases.
Example follow-up in the success path (see Lines 137-139 comment).
🤖 Prompt for AI Agents
In src/upgrade/nc_upgrade_manager.js around lines 114 to 121, the return object
on the no-upgrade path should always include a code property to standardize the
return shape. Modify the return statement to include a code field (e.g.,
ManageCLIResponse.NoUpgradeRequired) alongside upgrade_info, ensuring all
success returns have a consistent structure with a code and upgrade_info object.
| return { | ||
| upgrade_info: this_upgrade | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unify return shape: include success code in result
Return a code in the success path as well to make the contract consistent and remove the need for special-casing in callers.
- return {
- upgrade_info: this_upgrade
- };
+ return {
+ code: ManageCLIResponse.UpgradeSuccessful,
+ upgrade_info: this_upgrade
+ };To ensure no other call sites assume the old return type, run:
🏁 Script executed:
#!/bin/bash
# Find all upgrade_config_dir call sites and show surrounding context
rg -n -A3 -B3 $'upgrade_config_dir\\('Length of output: 7044
🏁 Script executed:
#!/bin/bash
sed -n '760,830p' src/test/unit_tests/nc/configuration/test_nc_upgrade_manager.test.jsLength of output: 4109
🏁 Script executed:
#!/bin/bash
rg -n "ManageCLIResponse" src/upgrade/nc_upgrade_manager.jsLength of output: 208
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/manage_nsfs/upgrade.jsLength of output: 5069
Unify return shape in upgrade_config_dir
Include a code field on the successful return value so callers can rely on a consistent response shape:
--- a/src/upgrade/nc_upgrade_manager.js
+++ b/src/upgrade/nc_upgrade_manager.js
@@ 134,7 +134,9 @@ class NCUpgradeManager {
- return {
- upgrade_info: this_upgrade
- };
+ return {
+ code: ManageCLIResponse.UpgradeSuccessful,
+ upgrade_info: this_upgrade
+ };• File: src/upgrade/nc_upgrade_manager.js (around line 137)
• Callers in manage_nsfs/upgrade.js already handle upgrade_res.code || ManageCLIResponse.UpgradeSuccessful.
• Unit tests only inspect res.upgrade_info and will continue to pass unchanged.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| upgrade_info: this_upgrade | |
| }; | |
| return { | |
| code: ManageCLIResponse.UpgradeSuccessful, | |
| upgrade_info: this_upgrade | |
| }; |
🤖 Prompt for AI Agents
In src/upgrade/nc_upgrade_manager.js around lines 137 to 139, the return object
from the function lacks a 'code' field, causing inconsistency with other return
shapes. Modify the return statement to include a 'code' property with a value
indicating success, such as ManageCLIResponse.UpgradeSuccessful, alongside the
existing 'upgrade_info' field. This ensures callers can rely on a consistent
response structure without affecting existing unit tests.
jackyalbo
changed the title
2 participants
Describe the Problem
We used to throw an error when trying to upgrade to the same version - Now we will just a regular response with "NoUpgradeRequired"
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
Bug Fixes
Tests