Skip to content

fix: correct LXC node assignment in getVMs#4

Open
gilby125 wants to merge 1 commit intomainfrom
fix/lxc-node-assignment-minimal
Open

fix: correct LXC node assignment in getVMs#4
gilby125 wants to merge 1 commit intomainfrom
fix/lxc-node-assignment-minimal

Conversation

@gilby125
Copy link
Owner

@gilby125 gilby125 commented Feb 8, 2026

Fixes LXC node attribution when aggregating containers across nodes by always using the iterated node name (not a potentially stale/incorrect VM payload field).

Also:

  • Makes "index.js" import-safe by only starting the server when executed directly.
  • Adds a lightweight unit test using Node's built-in test runner.

This is intended as a minimal, reviewable alternative to PR #2 (which mixes the bugfix with a TS migration and other unrelated changes).

Summary by CodeRabbit

  • New Features

    • ProxmoxServer class is now exported as the default module export for external use.
  • Bug Fixes

    • Fixed VM node assignment to accurately identify which cluster node each virtual machine belongs to when aggregating data.
  • Tests

    • Added test case to validate VM node assignment accuracy.
  • Chores

    • Added automated test script to package configuration.

@coderabbitai
Copy link

coderabbitai bot commented Feb 8, 2026

📝 Walkthrough

Walkthrough

The ProxmoxServer class is now exported from index.js, making it available for external imports. An LXC node assignment bug is fixed to use the iterated node consistently rather than falling back to VM payload data. A test script is added to package.json, and a corresponding unit test validates the corrected node assignment behavior.

Changes

Cohort / File(s) Summary
Core Module Export
index.js
ProxmoxServer class is now exported. LXC node mapping adjusted to consistently use node.node from iteration, removing fallback to vm.node from payload.
Test Configuration
package.json
Added npm test script entry to run unit tests via node --test test/*.test.js.
Unit Tests
test/getVMs.lxc-node.test.js
New test file validating that ProxmoxServer.getVMs assigns the node from iterated node object (pve1) rather than from VM payload data, ensuring correct node metadata for LXC containers.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A server exported, now freely shared,
Nodes aligned with careful care,
Tests ensure the path rings true,
No more wrong-nodes in the crew! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: correct LXC node assignment in getVMs' clearly and accurately summarizes the main change: correcting how the node is assigned for LXC containers in the getVMs method.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/lxc-node-assignment-minimal

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
index.js (1)

3147-3150: Main guard is correct for ESM.

Note that the module-level .env loading (lines 18–35) still runs on import even when used as a library. The catch block handles missing files gracefully, so tests won't break, but you may see a warning on stderr during test runs. Consider gating the .env loading inside the constructor or behind the same main guard if this becomes noisy.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant