feat!/major api improvements #37
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces extensive modifications across continuous integration, Docker configurations, domain management APIs, and testing suites. The workflow now caches additional packages and runs Docker integration tests, while the Dockerfile adopts a new base image and enhanced libvirt settings with a non-root user. Domain description interfaces and XML serialization have been expanded, and legacy native bindings have been removed and replaced with streamlined exports. New asynchronous operations for domain destruction and undefining have been implemented in C++ and exposed via updated Hypervisor and Domain classes. Several new tests and configuration improvements further validate the changes. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant H as Hypervisor
participant W as Worker
participant D as Domain
%% Domain Destruction Flow
C->>H: DomainDestroy(request)
H->>W: Create DomainDestroyWorker(callback, promise, domain)
W->>D: virDomainDestroy()
D-->>W: Return status/error
W->>H: Resolve promise with result
H-->>C: DomainDestroy result
%% Domain Undefine Flow
C->>H: DomainUndefine(request)
H->>W: Create DomainUndefineWorker(callback, promise, domain)
W->>D: virDomainUndefine()
D-->>W: Return status/error
W->>H: Resolve promise with result
H-->>C: DomainUndefine result
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
lib/domain-xml.ts (2)
89-125:⚠️ Potential issueDeserialization only handles the first loader object, creating mismatch with serialization arrays.
While the serialization can produce an array of
loaderelements, the deserialization only extracts the first element. This inconsistency can lead to data loss if multiple loaders are serialized. Consider supporting an array in deserialization, or make it explicit that only one loader is supported.Possible fix:
- if (os.loader?.[0]) { - osDesc.loader = { }; - ... - } + if (os.loader && os.loader.length > 0) { + if (os.loader.length === 1) { + // Single loader object + osDesc.loader = { ... }; + } else { + // Multiple loader objects + osDesc.loader = os.loader.map((loaderObj: any) => { + const loaderRef: any = {}; + // Populate loaderRef similarly + return loaderRef; + }); + } + }
369-404: 🛠️ Refactor suggestionWrap switch-case declarations in blocks.
Local declarations like
const emulatorDesccan leak into subsequent cases, violating lint rules. Wrapping each case body in braces resolves thenoSwitchDeclarationslint error and clarifies scope.Proposed change:
- case 'emulator': - const emulatorDesc = deviceDesc.emulator; - if (emulatorDesc.value) { - domain.devices.emulator.push({ - _: emulatorDesc.value - }); - } - break; + case 'emulator': { + const emulatorDesc = deviceDesc.emulator; + if (emulatorDesc.value) { + domain.devices.emulator.push({ + _: emulatorDesc.value + }); + } + break; + } ... - case 'console': - const consoleDesc = deviceDesc.console; - domain.devices.console.push({ - $: { - type: consoleDesc.type - } - }); - break; + case 'console': { + const consoleDesc = deviceDesc.console; + domain.devices.console.push({ + $: { + type: consoleDesc.type + } + }); + break; + } ... - case 'acpi': - domain.devices.acpi.push({}); - break; + case 'acpi': { + domain.devices.acpi.push({}); + break; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 369-369: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 388-388: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🧹 Nitpick comments (15)
.github/workflows/main.yml (1)
45-49: Proper service initialization for libvirt.The addition of these steps ensures that the libvirt service is running and properly configured for the tests to execute correctly. This is essential for the hypervisor management functionality.
However, consider quoting the
$USERvariable on line 48 to prevent potential issues with shell expansion.- sudo usermod -a -G libvirt $USER + sudo usermod -a -G libvirt "$USER"🧰 Tools
🪛 actionlint (1.7.4)
46-46: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
scripts/test-docker.sh (2)
49-53: Verify Docker buildx installation success.The script attempts to install Docker buildx if not found, but doesn't verify if the installation was successful before proceeding with the tests.
if ! docker buildx version >/dev/null 2>&1; then echo -e "${YELLOW}Docker buildx not found. Installing...${NC}" docker buildx install + if ! docker buildx version >/dev/null 2>&1; then + echo -e "${RED}Failed to install Docker buildx. Exiting.${NC}" + exit 1 + fi fi
56-56: Unused test-artifacts directory.The script creates a directory for test artifacts but doesn't appear to use it. Consider either removing it or documenting its purpose.
-# Create a temporary directory for test artifacts -mkdir -p test-artifacts +# If you need to store test artifacts, uncomment the line below +# mkdir -p test-artifacts.github/workflows/docker-integration.yml (1)
74-74: Add a newline at the end of the file and remove trailing spaces.Lint errors indicate missing end-of-file newline and trailing spaces.
Apply this diff:
- retention-days: 7 + retention-days: 7 +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 74-74: no new line character at the end of file
(new-line-at-end-of-file)
[error] 74-74: trailing spaces
(trailing-spaces)
lib/domain-xml.ts (1)
55-82: Consider unifying loader serialization logic.Presently, the code handles
loaderarrays and single objects separately, resulting in duplicated logic. Refactoring to a helper function or single code path could reduce complexity and improve maintainability.__tests__/integration.test.ts (1)
190-309: Evaluate parallel test runs for domain naming conflicts.Integration tests with a fixed domain name (
TEST_VM_NAME) and a shared image path can fail if multiple workflows run concurrently. Consider using unique domain names or ephemeral resources.lib/domain-desc.ts (3)
29-30: Ensure alignment with potential additional firmware types.While
'efi'and'bios'are common, there may be alternate firmware/boot loader specifications. If upstream changes or more variants (e.g., custom EFIs) are needed in the future, consider making this property more extensible.
31-39: Validate consistency of union structure forloader.The union of a single object vs. an array of objects is flexible but can complicate usage. If the architecture allows multiple loaders consistently, consider always using an array—possibly of length one or more—to reduce branching logic for consumers.
147-149: Clarify intended usage of theacpidevice.
acpi: Record<string, never>indicates an empty config object. If additional ACPI-related properties might be required in the future, consider defining placeholders or making the type more extensible now.Dockerfile (4)
7-19: Consider a more minimalist or pinned approach for packages.You’re installing many QEMU and libvirt components without version pinning. While this helps keep the environment fresh, it might lead to nondeterministic builds if package versions shift.
32-33: Consider caching pnpm installation.Installing
pnpmglobally each time can slow builds. Caching or leveraging an official Node image withpnpmbuilt-in could speed up CI/CD.
35-36: Evaluate multi-stage builds for faster, smaller images.By building in one stage and copying only the final artifacts, you can reduce the final Docker image size and isolate dev dependencies from runtime.
52-52: Reassess running multiple processes in a single container.
libvirtd --daemonplusnpm testin one container can complicate logs and persistent processes. Consider using a separate service container or a single entrypoint with supervisor if you need both processes concurrently.lib/types.ts (2)
8-37: Ensure promise rejections are handled.All methods return Promises. Confirm that callers handle failures (e.g., connection errors), and consider adding doc comments specifying possible error states.
39-46: Document the lifecycle ofHypervisorClass.Mention how
HypervisorClassis typically used (e.g., via a native binding). This helps maintainers and users understand the injection/creation pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/docker-integration.yml(1 hunks).github/workflows/main.yml(1 hunks)Dockerfile(1 hunks)__tests__/integration.test.ts(1 hunks)lib/domain-desc.ts(3 hunks)lib/domain-xml.ts(4 hunks)lib/get-bindings.ts(1 hunks)lib/index.ts(1 hunks)lib/types.ts(2 hunks)package.json(1 hunks)scripts/test-docker.sh(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
lib/get-bindings.ts (1)
lib/types.ts (1)
HypervisorClass(43-45)
lib/types.ts (2)
lib/index.ts (3)
ConnectListAllDomainsFlags(13-13)Domain(12-12)DomainGetXMLDescFlags(14-14)lib/get-bindings.ts (2)
ConnectListAllDomainsFlags(30-47)DomainGetXMLDescFlags(49-54)
🪛 YAMLlint (1.35.1)
.github/workflows/docker-integration.yml
[error] 74-74: no new line character at the end of file
(new-line-at-end-of-file)
[error] 74-74: trailing spaces
(trailing-spaces)
🪛 Biome (1.9.4)
lib/domain-xml.ts
[error] 369-369: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
lib/types.ts
[error] 135-135: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🪛 actionlint (1.7.4)
.github/workflows/main.yml
46-46: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (12)
.github/workflows/main.yml (1)
42-42: Good addition of QEMU package to cache.Adding
qemu-systemto the cached packages will improve workflow execution times by ensuring this dependency is available for subsequent runs.lib/get-bindings.ts (1)
11-11: Improved type naming from HypervisorConstructor to HypervisorClass.This change improves code clarity by using a more descriptive and intuitive type name that better reflects the purpose of the interface - a class for creating hypervisor instances.
Also applies to: 19-19
scripts/test-docker.sh (2)
1-14: Good initialization of Docker test script with clear architecture targets.The script is well-structured with clear documentation of supported architectures and color codes for improved output readability.
15-44: Well-designed test execution function with appropriate error handling.The
run_testsfunction properly builds and executes Docker containers for testing with the required privileges and volume mounts for libvirt. The error handling is appropriate with a non-zero exit code on failure.package.json (1)
29-31: Well-structured test script separation.Excellent reorganization of the test scripts into unit and integration tests. This separation provides better flexibility by allowing developers to run specific test suites as needed while maintaining the ability to run all tests together.
.github/workflows/docker-integration.yml (2)
13-33: Matrix enumeration looks good!The matrix strategy effectively tests various OS and architecture combinations. This approach ensures broad coverage for Docker builds and offers confidence in multi-platform support.
68-74:Details
❓ Verification inconclusive
Verify artifact directory before upload.
It's not immediately clear whether
test-artifacts/is guaranteed to exist across all runs. If no artifacts are generated, the upload step might fail silently. Consider creating or checking the directory conditionally.
🏁 Script executed:
#!/bin/bash # Verify that 'test-artifacts/' is created or referenced in the repository fd test-artifactsLength of output: 19
I'm not seeing any concrete evidence that the
test-artifacts/directory is created before its use in the upload step. Thefd test-artifactscommand produced no output, which is inconclusive. To get further clarity, please run a script that searches for references to the creation or use of thetest-artifactsdirectory (for example, looking for commands likemkdir test-artifactsor other mentions of it) across the repository.Below is a shell script that might help verify this more thoroughly:
#!/bin/bash echo "Searching for commands that create the test-artifacts directory (e.g., mkdir):" rg -n "mkdir.*test-artifacts" . echo "Searching for any reference to 'test-artifacts' in the repository:" rg -n "test-artifacts" .Once you verify whether the directory is created in any workflow or script, please update accordingly:
- If no creation is found: Consider adding a conditional step to create (or check) the directory before running the upload action to prevent silent failures.
- If the directory is created elsewhere: Please confirm that its creation is reliably executed on all runs.
Next Action:
Please run the script above to manually verify how thetest-artifacts/directory is handled across the codebase and confirm the expected behavior.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 74-74: no new line character at the end of file
(new-line-at-end-of-file)
[error] 74-74: trailing spaces
(trailing-spaces)
__tests__/integration.test.ts (1)
16-47:Details
❓ Verification inconclusive
Confirm that the
firmwarefield is set for relevant architectures.The code references
archConfig.firmware?.codeandarchConfig.firmware?.vars, butgetArchConfig()never sets these properties. This leads to unimplemented firmware copying for non-Apple architectures and potential dead code paths. Verify if you intend to add or remove firmware logic.
🏁 Script executed:
#!/bin/bash # Check for any references to 'firmware' in the codebase outside of 'getArchConfig' rg "firmware"Length of output: 1153
Firmware configuration is incomplete: Please clarify intended behavior
- The integration test (in
__tests__/integration.test.ts) accessesarchConfig.firmware?.codeandarchConfig.firmware?.vars, but thegetArchConfig()function never provides afirmwareproperty.- Additionally, the type definition in
lib/domain-desc.tsdeclaresfirmwaredifferently (firmware?: 'efi' | 'bios'), which is inconsistent with the object structure expected by the integration test.- Please confirm whether firmware support should be implemented in
getArchConfig()for relevant architectures, or if the firmware-related checks and references in the tests need removal/adjustment.lib/domain-desc.ts (2)
41-45: Check the interplay ofnvramandfirmware.It’s common for
nvramto be relevant primarily under certain firmware conditions (e.g., EFI). Ensure that the rest of the codebase properly accounts for scenarios wherenvramis present butfirmwareisbios, and vice versa.
105-105: Confirm usage and testing for the new'qemu'option.Expanding
typeto'qemu'is a sensible extension. Verify all domain creation, management, and validation logic to ensure it supports'qemu'consistently.Dockerfile (2)
2-2: Confirm Node.js 20 compatibility with QEMU and libvirt.Switching from
node:22tonode:20may affect library compatibility, especially with system libraries. Verify no unexpected issues arise from the older Node version.
21-26: Validate group existence before adding the new user.Some slim images might not have the
libvirtorkvmgroups pre-defined. Double-check that group creation or existence checks are properly handled for a robust build on all systems.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
__tests__/integration.test.ts (3)
55-57: Consider conditionally skipping tests if QEMU is missing.
Currently, the test immediately throws an error when QEMU is not installed. For CI pipelines or local dev environments lacking QEMU, you may prefer skipping or gracefully handling these tests rather than failing outright.
70-92: Revisit broad “ignore errors” approach in disk cleanup.
Multiple try/catch blocks absorb any potential errors during disk/NVRAM cleanup. While ignoring specific known errors may be intentional (e.g., the disk does not exist), masking other exceptions can hinder identifying real issues. Consider logging errors at a more visible level (e.g., warnings) and only ignoring truly benign conditions.
107-119: Improve shutdown error handling for better diagnostic clarity.
Ignoring errors during shutdown might mask genuine failures or concurrency issues. Even if it is expected to fail without an OS, capturing and logging the error details more explicitly could assist in troubleshooting and provide clearer insights into abnormal shutdown sequences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
__tests__/integration.test.ts(1 hunks)lib/types.ts(2 hunks)src/domain.h(1 hunks)src/hypervisor-domain.cc(1 hunks)src/hypervisor.cc(1 hunks)src/hypervisor.h(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/hypervisor.cc (1)
src/hypervisor-domain.cc (2)
DomainDestroy(723-743)DomainDestroy(723-723)
src/hypervisor-domain.cc (2)
src/worker.cc (2)
SetVirError(12-17)SetVirError(12-12)src/hypervisor-connect.cc (2)
dummyCallback(15-19)dummyCallback(15-15)
🪛 Biome (1.9.4)
lib/types.ts
[error] 136-136: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (6)
src/domain.h (1)
44-44: Friend class addition correctly follows established pattern.The addition of
DomainDestroyWorkeras a friend class allows it to access the privatedomainPtrmember, which is necessary for the domain destruction operation. This follows the same pattern used for other worker classes in this file.src/hypervisor.h (1)
69-69: Method declaration appropriately extends domain management capabilities.The
DomainDestroymethod declaration is correctly placed among other domain management methods and follows the same signature pattern as similar methods likeDomainShutdown.src/hypervisor.cc (1)
38-38: Instance method registration follows established naming conventions.The
domainDestroyinstance method is properly registered and follows the camelCase naming convention used for JavaScript-exposed methods. The registration is correctly placed among other domain management methods.src/hypervisor-domain.cc (2)
701-721: DomainDestroyWorker implementation follows established worker pattern.The
DomainDestroyWorkerclass correctly implements the worker pattern used throughout the codebase:
- Inherits from
Workerbase class- Takes appropriate parameters in constructor
- Implements Execute() to interact with libvirt API
- Properly handles error cases
This maintains consistency with other similar worker implementations in the file.
723-743: DomainDestroy method implementation is consistent with other domain operations.The implementation follows the established pattern for domain operations:
- Sets up environment and promise handling
- Validates input parameters
- Unwraps the Domain object
- Creates and queues the appropriate worker
- Returns the promise for asynchronous resolution
This ensures that the new functionality integrates well with the existing architecture.
lib/types.ts (1)
136-136: Avoid using empty interfaces.
This comment mirrors a past review indicating that an empty interface is effectively{}. Switching to a type alias (e.g.,type Domain = Record<string, never>;) or adding domain-specific details helps maintain clarity and consistency.-export interface Domain {} +export type Domain = Record<string, never>;🧰 Tools
🪛 Biome (1.9.4)
[error] 136-136: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (14)
tsconfig.json (1)
4-4: Clarify the Incremental Compilation Setting CommentThe configuration sets
"incremental": false, which disables incremental compilation, but the inline comment still reads "Enable incremental compilation." This creates a contradiction. Please update the comment to accurately reflect that incremental compilation is disabled (or update the value if the intent was different).src/domain.h (1)
44-47: Ensure new friend classes are tested.Adding
DomainDestroyWorkerandDomainUndefineWorkeras friend classes is fine if they require direct access to internal members. Verify that these new classes are covered by tests to ensure reliable domain destroy/undefine functionalities.lib/domain.ts (1)
3-58: Consider removing theanytype forhypervisor.The domain wrapper’s asynchronous methods delegate logic to the
hypervisor. Explicitly typing this property, for example by using a forward type reference (import type { Hypervisor } from './hypervisor.js';), could improve type safety.- private hypervisor: any; + private hypervisor: Hypervisor; // or a suitable interface/typelib/hypervisor.ts (1)
15-36: Robust error handling.Converting all errors into
LibvirtErrorfor uniform handling is a solid approach. As a minor enhancement, consider checking if the error is already aLibvirtErrorto avoid re-wrapping:if (error instanceof LibvirtError) { throw error; } else if (error instanceof Error) { ... }__tests__/integration.test.ts (5)
16-47: Refine architecture detection logic.
IngetArchConfig, you are branching twice onprocess.platformandprocess.arch, but the code paths for Darwin vs. other OS appear nearly duplicated aside from the emulator paths. Consider extracting shared logic (e.g., architecture strings and machine types) into a separate function or map to reduce duplication.function getArchConfig(): { arch: string; machine: string; firmware?: { code: string; vars: string }; emulator: string } { if (process.platform === 'darwin') { if (process.arch === 'arm64') { return { arch: 'aarch64', machine: 'virt', - emulator: '/opt/homebrew/bin/qemu-system-aarch64' + emulator: findQemuEmulator('darwin', 'arm64') // Example helper }; } else { return { arch: 'x86_64', machine: 'q35', - emulator: '/opt/homebrew/bin/qemu-system-x86_64' + emulator: findQemuEmulator('darwin', 'x64') // Example helper }; } } else { // ... } }
50-68: Confirm synchronous file checks won’t block the test flow.
verifyQemuInstallationrelies onexistsSynccalls that block the event loop. Although acceptable for a small integration test, large-scale tests might see performance issues. Consider an asynchronous approach if the test suite grows.
70-93: Ensure safe handling for leftover lock files.
cleanupDiskImagedoes a best-effort lock removal usingexecSync(qemu-img info ...). Although ignoring errors is fine in tests, it might be useful to log them with a distinct message if ephemeral lock cleanup fails repeatedly, to aid debugging on CI environments.
161-184: Avoid large logic in lifecycle hooks.
Thebeforehook is doing significant domain management and QEMU verifications. While it’s valid, consider splitting these steps or re-checking them in each test for clarity and fewer cross-test dependencies.
431-448: Verify multiple non-existent domain variations.
The test'should handle errors when trying to undefine a non-existent domain'checks a single non-existent name. Consider adding a test scenario for an invalid domain object or empty string to ensure libvirt errors are handled consistently.src/hypervisor-domain.cc (2)
701-743: Verify domain pointer validity before destruction.
InDomainDestroy, there is no explicit check for a NULLdomainPtrbefore callingvirDomainDestroy. While the call itself may fail gracefully, consider logging or throwing a friendlier error ifdomainPtris unexpectedly null.int destroy = 0; if (domain->domainPtr == nullptr) { - destroy = virDomainDestroy(domain->domainPtr); + SetError("Domain pointer is null, cannot destroy"); } else { destroy = virDomainDestroy(domain->domainPtr); }
749-787: Add warnings for persistent domains.
DomainUndefinecan remove persistent domain definitions. If the domain is still active or partially configured, it may cause confusion during subsequent commands. Consider logging a warning that the domain is being removed while still active.Would you like help implementing a quick check for an active domain before calling
virDomainUndefine?lib/types.ts (3)
8-18: Validate memory values inDomainInfo.
Consider clarifying thatmaxMemandmemoryare measured in KiB (the typical libvirt behavior). If you store them in bytes or MB in the rest of the codebase, the mismatch could cause confusion.
100-123: Avoid duplication of connect list flags.
ConnectListAllDomainsFlagsis comprehensive. If new flags are introduced in libvirt, you’ll need to update manually. Alternatively, these flags could be loaded from the native library if exposed.Would you like me to generate a snippet that attempts to read flags from libvirt’s headers or docs at build time?
136-139: ReplaceDomain = anywith a more specific type if possible.
Usinganyforfeits type-checking benefits. If the domain shape will remain dynamic, consider using a more descriptive union or partial interface for future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (12)
__tests__/integration.test.ts(1 hunks)lib/domain-desc.ts(6 hunks)lib/domain.ts(1 hunks)lib/get-bindings.ts(0 hunks)lib/hypervisor.ts(1 hunks)lib/index.ts(1 hunks)lib/types.ts(3 hunks)src/domain.h(1 hunks)src/hypervisor-domain.cc(1 hunks)src/hypervisor.cc(1 hunks)src/hypervisor.h(3 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- lib/get-bindings.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/hypervisor.cc
- src/hypervisor.h
- lib/domain-desc.ts
🧰 Additional context used
🧬 Code Definitions (3)
lib/hypervisor.ts (1)
lib/types.ts (8)
LibvirtError(141-159)ConnectListAllDomainsFlags(103-120)ConnectListAllDomainsFlags(122-122)Domain(139-139)DomainInfo(11-17)DomainGetXMLDescFlags(127-132)DomainGetXMLDescFlags(134-134)NodeInfo(22-31)
src/hypervisor-domain.cc (3)
lib/domain.ts (1)
destroy(25-27)src/worker.cc (2)
SetVirError(12-17)SetVirError(12-12)src/hypervisor-connect.cc (2)
dummyCallback(15-19)dummyCallback(15-15)
__tests__/integration.test.ts (5)
lib/hypervisor.ts (1)
Hypervisor(7-233)lib/types.ts (5)
Domain(139-139)DomainState(63-72)DomainState(74-74)HypervisorOptions(4-6)LibvirtError(141-159)lib/domain.ts (1)
Domain(3-58)lib/domain-desc.ts (1)
DomainDesc(103-150)lib/domain-xml.ts (1)
domainDescToXml(300-412)
🔇 Additional comments (13)
lib/index.ts (3)
1-2: Exports look well-structured.These lines ensure that all core types and constants are exposed for external use. There appears to be no overlap or duplication with other exports.
4-6: Single entry point for key classes is helpful.Re-exporting
HypervisorandDomainat the index level centralizes imports and enhances discoverability. This fosters a better developer experience.
8-8: Domain utilities export is consistent.Exporting
domain-desc.js,domain-xml.js, anddomain-builder.jshere is consistent with your approach of providing a one-stop index for domain-related functionality. No conflicts detected.lib/domain.ts (1)
1-2: Imports are appropriate.Importing the domain-related types helps maintain strong typing for domain operations.
lib/hypervisor.ts (6)
7-13: Excellent constructor approach for native binding.Using
requirefor the native module within an ES module context is a practical solution. Ensure that the path'../build/Release/libvirt'is well-documented or configurable to support different build configurations.
38-79: Connection methods and domain listing appear correct.The
connectOpen/CloseandconnectListAllDomainsmethods properly handle promises and rethrow errors viahandleError, promoting clarity in error reporting.
81-125: Domain creation and lookup methods are consistent.Each method returns a
Domainobject, providing uniform access to domain functionality. This design choice simplifies domain management at the higher layers.
127-166: Asynchronous domain operations are well-structured.Methods like
domainSave,domainCreate,domainShutdown,domainDestroy, anddomainUndefineconsistently forward the native domain pointers. This pattern centralizes error handling and ensures a clean interface from theDomainclass.
168-207: Domain information methods are straightforward.Using consistent naming (
domainGetInfo,domainGetID, etc.) is readable and intuitive, minimizing confusion.
209-232: Additional connect/node methods further enhance utility.Providing methods for retrieving maximum VCPUs, hostname, and node info completes the hypervisor management toolset. Consider verifying test coverage for these methods to ensure they handle edge cases gracefully.
__tests__/integration.test.ts (1)
95-159: Guard against concurrency or multiple cleanup calls.
IncleanupDomain, you are performing multiple asynchronous steps (graceful shutdown, forced shutdown, undefine). If two cleanup calls ran concurrently, domain lookups or forcible shutdown attempts might race. Since integration tests typically run sequentially, this might be acceptable, but it’s worth noting if you plan parallel test execution in the future.Could you confirm that these integration tests are always run serially on your CI so concurrency issues won’t arise?
lib/types.ts (2)
141-159: Ensure alignment of error details inLibvirtError.
You’re capturingcode,domain,level, and up to three optional strings. Make sure these properties precisely mirror the fields fromvirErrorso usage in your code remains consistent.Could you confirm that these are indeed all the fields you need from
virError?
60-75:Details
✅ Verification successful
Align domain states with official libvirt docs.
DomainStateconstants appear correct, but completeness and future changes may occur. If you prefer to track new states automatically, you might dynamically map them from libvirt constants at runtime. Otherwise, keep them updated whenever libvirt adds new domain states.
🌐 Web query:
Could you check the official libvirt documentation to confirm there are no newly added domain states beyond PMSUSPENDED?💡 Result:
The official libvirt documentation, as reflected in the provided sources, confirms six primary domain states: running, idle, paused, in shutdown, shut off, crashed, and pmsuspended[1][2][14]. These states are consistently documented across various versions of the libvirt tools, including
virsh, and are supported in recent resources like the Red Hat Enterprise Linux documentation and libvirt's API references[5][12][15].Key Domain States:
- Running: Actively executing on the hypervisor.
- Idle: Not actively using CPU resources but still allocated resources.
- Paused: Execution suspended, resources retained[1][2].
- In shutdown: Gracefully stopping operations.
- Shut off: Not running, either after shutdown or not started.
- Crashed: Terminated unexpectedly without restart.
- Pmsuspended: Suspended via guest power management (e.g., S3 sleep)[1][5][8].
Validation Against Sources:
- The
virshmanpage explicitly lists these states, with pmsuspended included[1].- Red Hat’s documentation for RHEL 7 and 6 reiterates these states, including pmsuspended[5][14].
- GitHub discussions (e.g., issues in
vagrant-libvirtandvirt-manager) address handling pmsuspended but do not mention newer states[8][9][11].- The libvirt domain XML schema and Java API also align with these definitions[10][15].
No sources indicate newly added states beyond pmsuspended in the latest documentation as of March 2025. For robustness, always refer to the official libvirt domain states documentation for updates.
Citations:
- 1: https://www.libvirt.org/manpages/virsh.html
- 2: https://wiki.libvirt.org/VM_lifecycle.html
- 3: https://wiki.libvirt.org/FAQ.html
- 4: https://www.pulumi.com/registry/packages/libvirt/api-docs/domain/
- 5: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/6/html/virtualization_administration_guide/sub-sect-starting_suspending_resuming_saving_and_restoring_a_guest_virtual_machine-waking_up_a_domain_from_pmsuspend_state
- 6: https://discussion.fedoraproject.org/t/virsh-wont-return-domains/62061
- 7: https://vagrant-libvirt.github.io/vagrant-libvirt/configuration.html
- 8: vagrant-libvirt/vagrant-libvirt#1200
- 9: virt-manager/virt-manager#258
- 10: https://download.libvirt.org/java/javadoc/org/libvirt/Domain.html
- 11: ansible-collections/community.libvirt#167
- 12: https://bugs.launchpad.net/bugs/1174672
- 13: https://libvirt.org/html/libvirt-libvirt-domain.html
- 14: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/7/html/virtualization_deployment_and_administration_guide/sect-managing_guest_virtual_machines_with_virsh-displaying_per_guest_virtual_machine_information
- 15: https://libvirt.org/formatdomain.html
- 16: https://wiki.gentoo.org/wiki/Virsh
- 17: https://libvirt.org/docs.html
- 18: https://libvir-list.redhat.narkive.com/0AjTpnkc/libvirt-patch-qemu-forbid-suspend-if-already-pmsuspended
- 19: https://gitlab.com/libvirt/libvirt-java/blob/master/src/main/java/org/libvirt/Domain.java
- 20: https://libvirt.gitlab.io/libvirt-appdev-guide-python/libvirt_application_development_guide_using_python-Guest_Domains-Listing_Domains.html
Libvirt domain states verified – code aligns with the official documentation.
The current
DomainStateinlib/types.ts(lines 60–75) correctly reflects the states defined by libvirt up toPMSUSPENDED. Verification against the official documentation confirms that no new domain states have been added beyond those currently listed. For additional future-proofing, you might consider a dynamic mapping approach, but for now the explicit constants are correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
lib/domain.ts (2)
28-30: Consider adding filename validation to prevent path traversal.The
savemethod accepts a filename parameter without validation. Consider adding validation to ensure the path is safe and doesn't allow for path traversal attacks.async save(filename: string): Promise<void> { + // Validate filename to prevent path traversal + if (!filename || filename.includes('..')) { + throw new Error('Invalid filename'); + } return this.hypervisor.domainSave(this, filename); }
119-121: Consider using a getter instead of a method for internal access.Since
getNativeDomainis a simple accessor that returns an internal property, consider using a TypeScript getter property instead of a method. This would make the code more idiomatic.-/** - * Gets the native libvirt domain object. - * This method is for internal use only. - * @returns The native domain object - */ -getNativeDomain(): NativeDomain { - return this.nativeDomain; -} +/** + * Gets the native libvirt domain object. + * This property is for internal use only. + * @returns The native domain object + */ +get nativeDomain(): NativeDomain { + return this.nativeDomain; +}Then update all references to
domain.getNativeDomain()todomain.nativeDomain.lib/hypervisor.ts (3)
12-23: Consider adding TypeScript type for the native hypervisor.The
nativeHypervisorproperty is typed asany, which reduces type safety. Consider creating a specific interface for the native hypervisor to improve type checking.+/** + * Interface for the native libvirt hypervisor. + */ +interface NativeHypervisor { + connectOpen(): Promise<void>; + connectClose(): Promise<void>; + connectListAllDomains(flags?: number): Promise<any[]>; + // Add other methods as needed +} export class Hypervisor { - private nativeHypervisor: any; + private nativeHypervisor: NativeHypervisor; /** * Creates a new Hypervisor instance. * @param options - Connection options * @param options.uri - The URI to connect to the hypervisor (e.g., 'qemu:///system') */ constructor(options: { uri: string }) { // Use require for native module since it's not an ES module - this.nativeHypervisor = new (require('../build/Release/libvirt').Hypervisor)(options); + this.nativeHypervisor = new (require('../build/Release/libvirt').Hypervisor)(options) as NativeHypervisor; }
31-52: Enhance error handling for edge cases.The
handleErrormethod doesn't handle the case whereerrormight beundefinedornull. Also, the second branch assumeserroris an object with atoStringmethod.private handleError(error: any): never { + if (error === undefined || error === null) { + throw new LibvirtError( + 'Unknown error occurred', + -1, + -1, + -1 + ); + } if (error instanceof Error) { // If it's already an Error instance, convert it to LibvirtError const libvirtError = new LibvirtError( error.message, (error as any).code || -1, (error as any).domain || -1, (error as any).level || -1, (error as any).str1, (error as any).str2, (error as any).str3 ); throw libvirtError; } // If it's a string or something else, create a generic LibvirtError throw new LibvirtError( - error.toString(), + typeof error === 'object' && error !== null ? error.toString() : String(error), -1, -1, -1 ); }
200-206: Add resource cleanup method.The class doesn't provide a way to automatically clean up resources. Consider adding a method to explicitly close connections and release resources.
/** * Cleans up resources used by this hypervisor instance. * This should be called when the hypervisor is no longer needed. * @throws {LibvirtError} If cleanup fails */ async cleanup(): Promise<void> { try { // Close the connection if it's open return await this.connectClose(); } catch (error) { this.handleError(error); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
lib/domain.ts(1 hunks)lib/hypervisor.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
lib/domain.ts (3)
lib/index.ts (2)
Domain(6-6)Hypervisor(5-5)lib/types.ts (4)
Domain(139-139)DomainGetXMLDescFlags(127-132)DomainGetXMLDescFlags(134-134)DomainInfo(11-17)lib/hypervisor.ts (1)
Hypervisor(12-370)
lib/hypervisor.ts (2)
lib/types.ts (7)
LibvirtError(141-159)ConnectListAllDomainsFlags(103-120)ConnectListAllDomainsFlags(122-122)Domain(139-139)DomainInfo(11-17)DomainGetXMLDescFlags(127-132)DomainGetXMLDescFlags(134-134)lib/domain.ts (1)
Domain(8-122)
🔇 Additional comments (5)
lib/domain.ts (3)
1-3: LGTM! Proper imports with extension for ES modules.The imports are correctly specified with the
.jsextension, which is necessary for ES modules to work properly in Node.js. Additionally, the import from./types.jsproperly aliasesDomaintoNativeDomainto avoid naming conflicts with the class being defined.
8-20: Well-structured domain class with proper encapsulation.The
Domainclass follows good object-oriented design by encapsulating both the native domain object and the hypervisor instance. This design allows for a clean separation of concerns, where theDomainclass provides a high-level API while delegating the actual work to the hypervisor.
37-39: LGTM! Method implementations follow a consistent pattern.All methods consistently delegate to the hypervisor instance, which handles the actual operations and error management. This approach ensures a clean separation of concerns and makes the codebase easier to maintain.
Also applies to: 46-48, 55-57, 64-66, 74-76, 83-85, 92-94, 101-103, 110-112
lib/hypervisor.ts (2)
1-6: LGTM! Proper imports and module creation for mixed module systems.The imports are correctly specified with the
.jsextension for ES modules. The use ofcreateRequireis appropriate for importing CommonJS modules in an ES module context, ensuring compatibility with the native libvirt module.
323-329:Details
✅ Verification successful
Verify flags handling in domainGetXMLDesc.
Ensure that the flags parameter is correctly passed to the native hypervisor. Some C bindings might have issues with optional parameters.
Run the following script to check how the native hypervisor handles flags:
🏁 Script executed:
#!/bin/bash # Find all places where domainGetXMLDesc is called rg "domainGetXMLDesc" -A 2 -B 2Length of output: 1094
Flags parameter forwarding verified
After reviewing the usage in both the TypeScript (in
lib/hypervisor.tsandlib/domain.ts) and native binding registrations (insrc/hypervisor.cc), the investigation shows that the optionalflagsparameter is correctly passed down to the native hypervisor call as implemented. No discrepancies were found in its handling.
- The code consistently forwards
flagsas-is tothis.nativeHypervisor.domainGetXMLDesc.- All usages in the codebase (including the wrapper in
lib/domain.ts) mirror this design.If the native C binding cannot handle an undefined value (if that’s a potential concern), consider defaulting
flagsto a concrete value (e.g.,0). Otherwise, the current implementation is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
lib/hypervisor.ts (3)
12-13: Consider using a more explicit type fornativeHypervisor.
Currently,nativeHypervisoris declared asany. Defining a dedicated interface or type can increase type safety and readability.- private nativeHypervisor: any; + private nativeHypervisor: NativeHypervisor; // For example, define an interface "NativeHypervisor"
31-52: Log the original error as a cause for better debugging.
While wrapping errors is good practice, including the original error in the newLibvirtError(e.g., as a.causeproperty) can help preserve the stack trace.
62-68: Potential concurrency considerations forwrapMethod.
If multiple async calls occur in parallel, ensuring thread-safety or clarifying concurrency constraints in the documentation might be beneficial, particularly for stateful native operations.__tests__/integration.test.ts (3)
71-94: Consider file operation resilience.
UsingexecSyncto remove or inspect disk images is risky if run on locked or in-use files. Adding more robust error handling or fallback logic could prevent unexpected test failures.
96-160: Potential improvement: modularize domain cleanup.
Many steps are chained together, which is fine for manual debugging but can complicate maintenance. Extracting repeated cleanup steps into utility functions or a shared library might keep this file more concise.
212-333: Consider splitting this test into smaller scenarios.
The test covers multiple actions (create, define, start, shut down) in a single flow. Smaller, more focused tests can yield clearer failures and faster results.Do you want help extracting each step (create, run, stop) into individual tests that share a common setup?
lib/types.ts (1)
131-149: EnhanceLibvirtErrorwith optional error cause chaining.
Modern JavaScript/TypeScript supports preserving the original cause in an error. Attaching the original cause can make debugging more effective.constructor( message: string, code: number, domain: number, level: number, str1?: string, str2?: string, str3?: string, + originalError?: any ) { super(message); + if (originalError) { + (this as any).cause = originalError; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
__tests__/integration.test.ts(1 hunks)lib/hypervisor.ts(1 hunks)lib/types.ts(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
__tests__/integration.test.ts (5)
lib/hypervisor.ts (1)
Hypervisor(12-349)lib/types.ts (3)
Domain(129-129)HypervisorOptions(4-6)LibvirtError(131-149)lib/domain.ts (1)
Domain(8-122)lib/domain-desc.ts (1)
DomainDesc(103-150)lib/domain-xml.ts (1)
domainDescToXml(300-412)
🔇 Additional comments (2)
lib/hypervisor.ts (1)
70-77: Implementation is clear and consistent.
The logic inconnectOpenis straightforward and uses the standardizedwrapMethodapproach effectively.__tests__/integration.test.ts (1)
11-48:Details
❓ Verification inconclusive
Verify cross-platform compatibility.
The current architecture detection might fail on less common setups or unsupported environments. Consider skipping these tests or providing instructions ifqemu-system-*is unavailable.
🏁 Script executed:
#!/bin/bash # Attempt to locate qemu binaries for non-standard platforms fd qemu-system- -x echo Found {}Length of output: 36
Action Required: Manual Verification of QEMU Binaries on Non‑Standard Setups
The current architecture detection ingetArchConfig()only covers typical macOS (darwin) and Linux environments, with hardcoded paths for qemu binaries. However, the supplied shell script (usingfd) returned no output, indicating that on non‑standard platforms these binaries might not be detectable. Please manually verify the availability of these binaries on less common setups. Consider either:
- Adding clearer fallback behavior (e.g., skipping integration tests if
qemu-system-*is unavailable), or- Documenting pre‑test requirements/instructions for environments where the qemu binaries might not be installed.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (28)
lib/types.ts (2)
120-120: Add description for the NONE flag.The other enum values have descriptive comments in the class-level JSDoc, but the NONE flag doesn't have an explanation of its purpose.
export enum DomainGetXMLDescFlags { - NONE = 0, + /** No special flags */ + NONE = 0, SECURE = 1, INACTIVE = 2, UPDATE_CPU = 4, MIGRATABLE = 8 }
151-153: Enhance error message context for more error types.You've improved the error message for 'Expected a number' errors, but similar improvements could be made for other common error types to help with debugging.
// Add more context to the error message if (message === 'Expected a number.') { this.message = `Type error: Expected a number but received ${typeof str1 || 'undefined'}. This error typically occurs when calling libvirt methods that require numeric parameters.`; +} else if (message === 'Expected a string.') { + this.message = `Type error: Expected a string but received ${typeof str1 || 'undefined'}. This error typically occurs when calling libvirt methods that require string parameters.`; +} else if (message.includes('not found')) { + this.message = `Resource not found: ${message}. Please verify that the requested resource exists and you have proper permissions.`; }vitest.config.ts (1)
20-25: Consider more realistic coverage thresholds.Setting all coverage thresholds to 100% is ambitious and may be difficult to maintain long-term, especially for edge cases and error handling branches. Consider slightly more forgiving thresholds (e.g., 90-95%) to balance thoroughness with practicality.
thresholds: { - branches: 100, - functions: 100, - lines: 100, - statements: 100 + branches: 95, + functions: 100, + lines: 98, + statements: 98 }__tests__/error.test.ts (1)
26-26: Replacedeleteoperator withundefinedassignment for better performance.The static analysis tool correctly flags the use of the
deleteoperator, which can impact performance. Assignundefinedinstead.- delete errorWithoutStack.stack; + errorWithoutStack.stack = undefined; - delete libvirtErrorNoStack.stack; + libvirtErrorNoStack.stack = undefined; - delete errorWithAllPropsNoStack.stack; + errorWithAllPropsNoStack.stack = undefined; - delete errorWithSomePropsNoStack.stack; + errorWithSomePropsNoStack.stack = undefined; - delete errorWithNothingNoStack.stack; + errorWithNothingNoStack.stack = undefined;Also applies to: 40-40, 112-112, 124-124, 131-131
🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
lib/error.ts (1)
55-61: Missing explicit return statement.While the function will never reach line 60 because
handleErroralways throws, TypeScript expects a return value of typeTfor this function.export async function wrapMethod<T>(method: (...args: any[]) => Promise<T>, ...args: any[]): Promise<T> { try { return await method(...args); } catch (error) { handleError(error); - } + // This line is never reached as handleError always throws + return undefined as never; + } }lib/domain.spec.ts (1)
1-86: Consider adding more test cases for remaining Domain methods.The current tests cover only a subset of the Domain class methods. For complete test coverage, consider adding tests for other methods like create, shutdown, destroy, and undefine.
lib/domain-xml.ts (2)
175-176: Use optional chaining for error prevention as suggested by static analysis.Several places in the code are flagged by static analysis for missing optional chaining. Follow the pattern you've already applied in other sections.
Apply this diff to address static analysis warnings:
-if (disk.driver && disk.driver[0]) { +if (disk.driver?.[0]) { -if (disk.source && disk.source[0]) { +if (disk.source?.[0]) { -if (disk.target && disk.target[0]) { +if (disk.target?.[0]) { -if (iface.source && iface.source[0]) { +if (iface.source?.[0]) { -if (iface.mac && iface.mac[0]) { +if (iface.mac?.[0]) { -if (iface.model && iface.model[0]) { +if (iface.model?.[0]) {Also applies to: 186-186, 193-193, 246-246, 253-253, 260-260
🧰 Tools
🪛 Biome (1.9.4)
[error] 175-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
369-369: Fix switch clause variable declaration issue.Static analysis detected an issue with the declaration of
emulatorDescin a switch clause, which can potentially be accessed by other clauses.case 'emulator': - const emulatorDesc = deviceDesc.emulator; - if (emulatorDesc.value) { - domain.devices.emulator.push({ - _: emulatorDesc.value - }); - } + { + const emulatorDesc = deviceDesc.emulator; + if (emulatorDesc.value) { + domain.devices.emulator.push({ + _: emulatorDesc.value + }); + } + } break;🧰 Tools
🪛 Biome (1.9.4)
[error] 369-369: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
__tests__/domain-listing.test.ts (1)
9-11: Improve type safety by using specific typesThe
connectionandarchConfigvariables are typed asany, which reduces type safety. Consider using more specific types for better IDE support and compile-time checks.- let connection: any; + let connection: import('../lib').Connect; - let archConfig: any; + let archConfig: { arch: string; machine: string; emulator: string; firmware?: { vars: string } };__tests__/domain-cleanup.test.ts (5)
9-11: Improve type safety by using specific typesThe
connectionandarchConfigvariables are typed asany, which reduces type safety. Consider using more specific types for better IDE support and compile-time checks.- let connection: any; + let connection: import('../lib').Connect; - let archConfig: any; + let archConfig: { arch: string; machine: string; emulator: string; firmware?: { vars: string } };
39-60: Reduce code duplication by extracting common domain configurationThe same domain configuration is repeated in both test cases. Consider extracting it to a helper function or a constant to reduce duplication.
+ const createMinimalDomainDesc = (): DomainDesc => ({ + type: 'qemu', + name: TEST_VM_NAME, + memory: { value: 512 * 1024 }, + vcpu: { value: 1 }, + os: { + type: { + arch: archConfig.arch, + machine: archConfig.machine, + value: 'hvm' + }, + boot: { dev: 'hd' } + }, + devices: [ + { + type: 'emulator', + emulator: { + value: archConfig.emulator + } + } + ] + }); it('should allow undefining a domain through the domain object', async () => { // Create a minimal VM configuration - const domainDesc: DomainDesc = { - type: 'qemu', - name: TEST_VM_NAME, - memory: { value: 512 * 1024 }, - vcpu: { value: 1 }, - os: { - type: { - arch: archConfig.arch, - machine: archConfig.machine, - value: 'hvm' - }, - boot: { dev: 'hd' } - }, - devices: [ - { - type: 'emulator', - emulator: { - value: archConfig.emulator - } - } - ] - }; + const domainDesc = createMinimalDomainDesc();
72-74: Remove unnecessary null checkThe code has already confirmed that
domainis not null on line 65 with the expectation check. The null check here is redundant.- if (domain) { - await domain.undefine(); - } + await domain.undefine();
83-104: Reuse domain configuration to reduce duplicationUse the extracted helper function for creating the domain description.
- const domainDesc: DomainDesc = { - type: 'qemu', - name: TEST_VM_NAME, - memory: { value: 512 * 1024 }, - vcpu: { value: 1 }, - os: { - type: { - arch: archConfig.arch, - machine: archConfig.machine, - value: 'hvm' - }, - boot: { dev: 'hd' } - }, - devices: [ - { - type: 'emulator', - emulator: { - value: archConfig.emulator - } - } - ] - }; + const domainDesc = createMinimalDomainDesc();
116-118: Remove unnecessary null checkThe code has already confirmed that
domainis not null on line 109 with the expectation check. The null check here is redundant.- if (domain) { - await connection.domainUndefine(domain); - } + await connection.domainUndefine(domain);__tests__/domain-lifecycle.test.ts (4)
9-11: Improve type safety by using specific typesThe
connectionandarchConfigvariables are typed asany, which reduces type safety. Consider using more specific types for better IDE support and compile-time checks.- let connection: any; + let connection: import('../lib').Connect; - let archConfig: any; + let archConfig: { arch: string; machine: string; emulator: string; firmware?: { vars: string } };
93-93: Remove console.log from production codeConsole logging statements should be removed before submitting the code. Consider using a test logger if debugging information is needed.
- console.log('Generated XML:', xml);This applies to all console.log statements in this file.
40-41: Use asynchronous file operationsUsing
execSynccan block the event loop. Consider using the asynchronous version of file operations.- execSync(`cp "${archConfig.firmware.vars}" "/tmp/test-vm.nvram"`); + await fs.promises.copyFile(archConfig.firmware.vars, "/tmp/test-vm.nvram");You'll need to import the fs module:
import * as fs from 'fs';
96-96: Use asynchronous file operationsUsing
execSynccan block the event loop. Consider using the asynchronous version of file operations.- execSync(`qemu-img create -f qcow2 ${DISK_IMAGE} 1G`); + await util.promisify(exec)(`qemu-img create -f qcow2 ${DISK_IMAGE} 1G`);You'll need to import the util module and change the import for child_process:
-import { execSync } from 'child_process'; +import { exec } from 'child_process'; +import * as util from 'util';.github/workflows/docker-integration.yml (3)
54-54: Use more specific Docker image tagThe current tag
libvirt-test-${{ matrix.arch }}might conflict with other images. Consider adding the commit SHA or another unique identifier to the tag.- -t libvirt-test-${{ matrix.arch }} \ + -t libvirt-test-${{ matrix.arch }}-${{ github.sha }} \Also update the reference to this tag on line 65:
- libvirt-test-${{ matrix.arch }} + libvirt-test-${{ matrix.arch }}-${{ github.sha }}
49-49: Fix linting issues: Remove trailing whitespaceThere are trailing spaces on this line that should be removed to fix linting issues.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 49-49: trailing spaces
(trailing-spaces)
73-73: Fix linting issues: Add newline at end of file and remove trailing whitespaceAdd a newline at the end of the file and remove trailing spaces to fix linting issues.
- retention-days: 7 + retention-days: 7 +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 73-73: no new line character at the end of file
(new-line-at-end-of-file)
[error] 73-73: trailing spaces
(trailing-spaces)
Dockerfile (1)
53-54: Use a more robust setup for the daemon startThe current command uses a fixed sleep duration (2 seconds) which might not be sufficient for the daemon to fully initialize on all systems, potentially causing race conditions in tests.
Consider using a healthcheck approach instead:
-CMD ["/bin/sh", "-c", "/usr/sbin/libvirtd --daemon && sleep 2 && npm test"] +CMD ["/bin/sh", "-c", "/usr/sbin/libvirtd --daemon && while ! virsh -c qemu:///session list > /dev/null 2>&1; do sleep 0.5; done && npm test"]lib/domain-builder.spec.ts (1)
95-107: Consider testing additional template propertiesWhile you test the basic template functionality, consider adding assertions for more complex nested properties to ensure they are properly preserved.
You could enhance the template test with more complex structures:
const template: DomainDesc = { name: 'template-vm', memory: { unit: 'KiB', value: 1024 * 1024 }, vcpu: { placement: 'static', value: 1 }, + devices: [ + { + type: 'disk', + disk: { + type: 'file', + device: 'disk', + source: { file: '/path/to/disk.img' }, + target: { dev: 'vda', bus: 'virtio' } + } + } + ] };__tests__/hypervisor.test.ts (2)
150-151: Use unique temporary paths for test filesThe hardcoded path for the save file (
/tmp/domain-save-test) could cause issues if tests run in parallel or if previous test runs failed to clean up.Consider generating a unique filename:
-const saveFile = '/tmp/domain-save-test'; +const saveFile = `/tmp/domain-save-test-${Date.now()}-${Math.floor(Math.random() * 10000)}`;
47-53: Improve error handling for KVM checkThe current approach just logs a message if KVM is not available, but doesn't indicate this in the test output.
Consider this approach that makes the skip more explicit:
try { const maxVcpusKvm = await connection.connectGetMaxVcpus('kvm'); expect(maxVcpusKvm).toBeGreaterThan(0); } catch (error) { - // Some systems might not support KVM, so we'll just log this - console.log('KVM not available on this system'); + // Some systems might not support KVM + console.log('Skipping KVM test: not available on this system'); + expect((error as Error).message).toContain('Error'); // Verify it's an expected error }__tests__/helpers.ts (3)
9-40: Architecture detection can be simplified.The architecture detection logic has duplicated code paths for Darwin and other platforms that could be consolidated.
export function getArchConfig(): { arch: string; machine: string; firmware?: { code: string; vars: string }; emulator: string } { - if (process.platform === 'darwin') { - if (process.arch === 'arm64') { - return { - arch: 'aarch64', - machine: 'virt', - emulator: '/usr/bin/qemu-system-aarch64' - }; - } else { - return { - arch: 'x86_64', - machine: 'q35', - emulator: '/usr/bin/qemu-system-x86_64' - }; - } - } else { - if (process.arch === 'arm64') { - return { - arch: 'aarch64', - machine: 'virt', - emulator: '/usr/bin/qemu-system-aarch64' - }; - } else { - return { - arch: 'x86_64', - machine: 'q35', - emulator: '/usr/bin/qemu-system-x86_64' - }; - } - } + if (process.arch === 'arm64') { + return { + arch: 'aarch64', + machine: 'virt', + emulator: '/usr/bin/qemu-system-aarch64' + }; + } else { + return { + arch: 'x86_64', + machine: 'q35', + emulator: '/usr/bin/qemu-system-x86_64' + }; + } }
63-86: Consider parameterizing hardcoded paths in disk cleanup.The current implementation uses hardcoded paths which may limit flexibility and portability.
-export const cleanupDiskImage = () => { +export const cleanupDiskImage = (diskImagePath = DISK_IMAGE, nvramPattern = '/tmp/test-vm.nvram*') => { try { // Clean up disk image - if (existsSync(DISK_IMAGE)) { + if (existsSync(diskImagePath)) { // Try to remove any locks first try { - execSync(`qemu-img info ${DISK_IMAGE} --force-share`); + execSync(`qemu-img info ${diskImagePath} --force-share`); } catch (error) { // Ignore errors from force-share } // Remove the disk image - execSync(`rm -f ${DISK_IMAGE}`); + execSync(`rm -f ${diskImagePath}`); } // Clean up NVRAM files try { - execSync('rm -f /tmp/test-vm.nvram*'); + execSync(`rm -f ${nvramPattern}`); } catch (error) { // Ignore errors from NVRAM cleanup } } catch (error) { console.error('Error cleaning up files:', error); } };
88-152: Extract wait logic and use consistent error logging.The domain cleanup function includes repeated timeout logic that could be extracted, and uses inconsistent error logging approaches.
Create a
sleephelper function and use consistent error handling:+// Helper function for waiting +const sleep = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); export const cleanupDomain = async (connection: Hypervisor) => { try { console.log('Starting domain cleanup...'); // Check both defined and active domains const definedDomains = await connection.connectListDefinedDomains(); const activeDomainIds = await connection.connectListDomains(); console.log('Defined domains:', definedDomains); console.log('Active domain IDs:', activeDomainIds); // Check if domain exists in either list if (definedDomains.includes(TEST_VM_NAME) || activeDomainIds.length > 0) { console.log(`Found existing domain ${TEST_VM_NAME}, attempting cleanup...`); try { const existingDomain = await connection.domainLookupByName(TEST_VM_NAME); if (existingDomain) { try { const info = await existingDomain.getInfo(); console.log('Domain state:', info.state); if (info.state === DomainState.RUNNING) { // Try graceful shutdown first try { console.log('Attempting graceful shutdown...'); await existingDomain.shutdown(); - await new Promise(resolve => setTimeout(resolve, 2000)); + await sleep(2000); } catch (error) { - console.log('Graceful shutdown failed:', error instanceof Error ? error.message : error); + console.error('Graceful shutdown failed:', error instanceof Error ? error.message : error); } // Force shutdown if still running try { console.log('Attempting force shutdown...'); await existingDomain.destroy(); - await new Promise(resolve => setTimeout(resolve, 2000)); + await sleep(2000); } catch (error) { - console.log('Force shutdown failed:', error instanceof Error ? error.message : error); + console.error('Force shutdown failed:', error instanceof Error ? error.message : error); } } } catch (error) { - console.log('Error getting domain info:', error instanceof Error ? error.message : error); + console.error('Error getting domain info:', error instanceof Error ? error.message : error); } } } catch (error) { - console.log('Error looking up domain:', error instanceof Error ? error.message : error); + console.error('Error looking up domain:', error instanceof Error ? error.message : error); } // Now try to undefine the domain try { console.log('Attempting to undefine domain...'); const domain = await connection.domainLookupByName(TEST_VM_NAME); if (domain) { await connection.domainUndefine(domain); - await new Promise(resolve => setTimeout(resolve, 2000)); + await sleep(2000); console.log('Domain undefined successfully'); } } catch (error) { - console.log('Error during undefine:', error instanceof Error ? error.message : error); + console.error('Error during undefine:', error instanceof Error ? error.message : error); } } else { console.log('No existing domain found to clean up'); } } catch (error) { console.error('Error during domain cleanup:', error instanceof Error ? error.message : error); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.github/workflows/docker-integration.yml(1 hunks).gitignore(1 hunks)Dockerfile(1 hunks)__tests__/domain-cleanup.test.ts(1 hunks)__tests__/domain-lifecycle.test.ts(1 hunks)__tests__/domain-listing.test.ts(1 hunks)__tests__/error.test.ts(1 hunks)__tests__/helpers.ts(1 hunks)__tests__/hypervisor.test.ts(1 hunks)lib/domain-builder.spec.ts(1 hunks)lib/domain-xml.spec.ts(9 hunks)lib/domain-xml.ts(7 hunks)lib/domain.spec.ts(1 hunks)lib/error.ts(1 hunks)lib/hypervisor.ts(1 hunks)lib/types.spec.ts(1 hunks)lib/types.ts(6 hunks)package.json(4 hunks)vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🧬 Code Definitions (5)
lib/types.spec.ts (1)
lib/types.ts (1)
LibvirtError(132-159)
lib/domain.spec.ts (2)
lib/types.ts (1)
Domain(130-130)lib/hypervisor.ts (6)
Hypervisor(13-316)domainSave(151-157)domainGetXMLDesc(266-272)domainGetID(226-231)domainGetName(239-244)domainGetUUIDString(252-257)
lib/hypervisor.ts (8)
lib/index.ts (2)
Hypervisor(5-5)Domain(6-6)src/hypervisor.cc (1)
Hypervisor(57-78)src/hypervisor.h (1)
Hypervisor(48-48)lib/error.ts (1)
wrapMethod(55-61)lib/domain.ts (1)
Domain(8-122)lib/types.ts (1)
Domain(130-130)src/domain.h (2)
Domain(22-22)Domain(23-23)src/hypervisor-domain.cc (1)
id(199-211)
lib/error.ts (1)
lib/types.ts (1)
LibvirtError(132-159)
lib/types.ts (3)
src/domain.h (2)
Domain(22-22)Domain(23-23)lib/index.ts (1)
Domain(6-6)lib/domain.ts (1)
Domain(8-122)
🪛 Biome (1.9.4)
lib/domain-xml.ts
[error] 175-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 186-186: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 193-193: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 253-253: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 260-260: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 369-369: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
__tests__/error.test.ts
[error] 26-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 112-112: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 124-124: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 131-131: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 YAMLlint (1.35.1)
.github/workflows/docker-integration.yml
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 73-73: no new line character at the end of file
(new-line-at-end-of-file)
[error] 73-73: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Integration Tests (arm64)
- GitHub Check: Docker Integration Tests (amd64)
🔇 Additional comments (57)
lib/types.ts (2)
130-130: Reconsider usinganyfor the Domain type.Using
anybypasses TypeScript's type safety, which can lead to runtime errors. This was previously flagged in an earlier review. Consider creating a proper interface that defines the expected shape of Domain objects.-export type Domain = any; +export interface Domain { + // Define known properties here + // This could be populated based on the actual domain structure +}
132-159: LGTM! Well-designed error class.The
LibvirtErrorclass is well-structured with appropriate properties and methods. The additional context for error messages and the formattedtoString()method will be helpful for debugging.lib/types.spec.ts (1)
1-20: LGTM! Comprehensive test coverage for LibvirtError.The tests thoroughly validate the key functionality of the LibvirtError class:
- Error message enhancement for numeric type errors
- Correct formatting of error information in the toString method
- Proper handling of undefined values
vitest.config.ts (1)
1-36: LGTM! Well-configured Vitest setup.The configuration includes appropriate settings for the test environment, file patterns, coverage reporting, and timeouts. Using the 'v8' coverage provider is a good choice for accurate coverage reporting.
__tests__/error.test.ts (1)
5-161: LGTM! Comprehensive error handling test cases.The tests cover an impressive range of error scenarios, which will help ensure robust error handling throughout the application. Testing various error types, object structures, and edge cases provides excellent coverage of the error handling logic.
🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 112-112: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 124-124: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 131-131: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
lib/error.ts (2)
1-7: Good documentation for error handling function.The JSDoc comment clearly explains the purpose and behavior of the
handleErrorfunction. It's valuable to explicitly document the throw behavior as this helps users understand the control flow.
8-46: Comprehensive error handling with well-structured conversion.The
handleErrorfunction effectively handles different error types (Error instances, strings, and other objects) and consistently converts them toLibvirtErrorinstances with appropriate fallbacks. The preservation of stack traces is particularly valuable for debugging.lib/domain.spec.ts (5)
1-5: Appropriate test framework setup.Good choice of testing framework (Vitest) and proper imports of the components to be tested.
6-46: Well-structured test setup with comprehensive mocking.The test setup properly mocks all necessary Hypervisor methods and initializes the Domain instance with mock dependencies. This approach isolates the Domain class for focused testing.
48-54: Good test case for save method.The test properly verifies that the Domain.save method correctly delegates to the hypervisor's domainSave method with the right parameters.
56-69: Comprehensive testing of getXMLDesc with and without flags.Good approach testing both the default behavior (no flags) and with specific flags. This ensures the method works correctly in different scenarios.
71-85: Complete test coverage for ID and UUID retrieval.Tests for getID and getUUIDString are well-structured, verifying both the return values and correct delegation to the hypervisor methods.
lib/domain-xml.ts (4)
55-82: Support for loader property with array handling.Good implementation of the loader property serialization with support for both array and single-object inputs. The code correctly handles all optional properties.
89-124: Improved safety with optional chaining for property access.Using optional chaining (
?.) for accessing properties prevents potential null/undefined errors. The loader property deserialization is properly implemented.
362-363: Added support for ACPI devices.The addition of the 'acpi' array to the devices object appropriately extends the serialization capabilities.
401-403: Implemented ACPI device type handling.The new case to handle 'acpi' device type follows the established pattern for device handling.
lib/hypervisor.ts (7)
1-24: Well-structured class with appropriate imports and initialization.The Hypervisor class is well-designed with clear imports and a constructor that properly initializes the native hypervisor connection. Good use of JSDoc comments to document the class and constructor.
30-40: Consistent error handling with wrapMethod.The implementation properly uses the wrapMethod utility for standardized error handling, which is an improvement over duplicating try/catch blocks in each method.
49-55: Comprehensive domain listing with proper type handling.The connectListAllDomains method correctly maps native domain objects to Domain instances and provides sensible default flags.
81-101: Good domain creation methods with proper domain wrapping.Both domainCreateXML and domainDefineXML are well-implemented, properly wrapping the native domains in Domain instances for easier use.
151-205: Comprehensive domain management methods with consistent patterns.The domain management methods (save, create, shutdown, destroy, undefine) all follow a consistent pattern and are well-documented with JSDoc comments.
213-272: Thorough domain information retrieval methods.The methods for retrieving domain information (getInfo, getID, getName, getUUIDString, getXMLDesc) are well-implemented and follow a consistent pattern.
280-315: Good hypervisor and node information methods.The methods for retrieving hypervisor and node information, along with the domain restore method, complete the class with a comprehensive API.
Dockerfile (3)
21-26: LGTM: Good security practice using non-root userCreating a non-root user for running libvirt is a good security practice. The setup correctly adds the user to the necessary groups and sets appropriate permissions.
39-42: Recheck security implications ofauth_unix_rw = "none".This setting removes authentication for the read-write socket, which might be acceptable in a restricted container environment but could be a security concern if containers share the same host environment.
2-2:Details
✅ Verification successful
Downgraded Node.js version from 22 to 20
The base image has been downgraded from Node.js 22 to 20. While this might be intentional for compatibility reasons, ensure this aligns with your application requirements.
🏁 Script executed:
#!/bin/bash # Check if there are any dependencies or code that requires Node.js 20 specifically grep -r "engines" --include="package.json" .Length of output: 13571
Node.js Version Downgrade Verified
The Dockerfile now uses
FROM node:20-slim, and a scan of theenginesfields acrosspackage.jsonfiles (including those in dependencies) shows that none explicitly require Node.js 22. Most dependencies indicate compatibility with older versions (e.g.,>=10), which suggests that the downgrade is safe.Please ensure that the application's primary
package.json(if anenginesfield is defined) also aligns with targeting Node.js 20. Otherwise, no further changes are required.lib/domain-builder.spec.ts (1)
1-126: Excellent test coverage for the DomainBuilder classThe test suite is comprehensive and well-structured, covering all the key functionality of the DomainBuilder class including creating domains, setting properties, and managing devices. The tests properly validate both the positive cases and removal operations.
package.json (3)
29-31: LGTM: Successfully migrated from Mocha to VitestThe test framework has been updated from Mocha to Vitest with appropriate scripts for running tests, generating coverage reports, and accessing the UI. This is a good modernization of the testing infrastructure.
102-103: LGTM: Clear system requirements specificationThe system requirements have been updated from text descriptions to specific version numbers, which makes it much clearer what versions of libvirt and QEMU are required. This is more precise than the previous descriptions.
75-77:Details
✅ Verification successful
Verify Vitest compatibility with project requirements
Ensure that the newly added Vitest dependencies and their versions are compatible with your other tools and CI pipeline.
Also applies to: 87-88
🏁 Script executed:
#!/bin/bash # Check if Vitest is compatible with the current Node.js version and TypeScript version echo "Node.js version:" node -v echo "TypeScript version:" grep "typescript" package.json echo "Vitest version:" grep "vitest" package.jsonLength of output: 566
Vitest Dependency Versions are Consistent and Compatible
The Node.js (v22.9.0), TypeScript (^5.7.3), and Vitest (^1.3.1) versions in the package.json have been verified and are aligned. There are no version conflicts, and the test scripts reflect the intended usage of Vitest. Please ensure that the CI pipeline is updated to run the Vitest commands as configured.
- Verify package.json lines 75–77 (Vitest and its plugins) and lines 87–88 (test scripts).
- Confirm that your CI environment supports Node.js v22.9.0 and has the necessary settings for Vitest execution.
__tests__/hypervisor.test.ts (2)
13-35: LGTM: Proper test setup and teardownThe test suite includes well-structured setup and teardown procedures to ensure resources are properly initialized and cleaned up, even if tests fail. The timeout values are appropriate for these operations.
250-260: LGTM: Thorough error handling testsThe error handling tests are well-designed to verify that the system properly rejects invalid inputs and throws the expected error types. This is essential for robust API design.
lib/domain-xml.spec.ts (21)
13-13: Good addition of Vitest for test framework.The import of
describeanditfrom Vitest demonstrates a migration to or addition of Vitest alongside Chai, which is a positive modernization of the testing infrastructure.
51-68: Good handling of missing type value edge case.This test properly verifies that the XML serialization gracefully handles the case when a domain OS type has attributes but no value.
70-84: Proper validation of edge case with missing boot dev attribute.This test ensures the serialization correctly handles empty boot elements, which is essential for robust XML generation.
86-100: Good coverage for empty loader fields serialization.This test verifies that empty loader configurations are serialized correctly, improving the robustness of the serialization process.
102-125: Well-implemented test for loader array serialization.This test thoroughly validates that arrays of loaders with various missing fields are serialized correctly, ensuring compatibility with different configurations.
146-185: Comprehensive testing of OS XML serialization and deserialization.These tests thoroughly validate both full and minimal OS configurations in both serialization and deserialization paths, ensuring robustness and compatibility across various use cases.
Also applies to: 187-201, 203-241, 243-256
258-272: Excellent coverage of OS XML edge cases.These tests thoroughly verify handling of missing optional fields and empty configurations during serialization and deserialization, which is crucial for handling real-world XML variations.
Also applies to: 274-287, 289-305, 307-321, 323-343
369-395: Good test for disk serialization with missing fields.This test ensures that disk serialization works correctly even when type and device fields are missing, supporting backward compatibility and resilience.
419-598: Comprehensive validation of disk XML handling.These tests thoroughly cover optional field handling, serialization and deserialization of full and partial disk configurations, ensuring robust XML handling for all disk scenarios.
601-647: Thorough testing of empty disk object cases.These tests verify that disk configurations with empty object properties are handled gracefully during both serialization and deserialization.
673-697: Good validation of interfaces without type field.This test ensures that network interfaces without a specified type can still be properly serialized, which is important for backward compatibility and handling incomplete configurations.
722-866: Comprehensive testing of interface XML edge cases.These tests thoroughly verify the handling of optional fields, full configurations, partial configurations, and empty objects in interface serialization and deserialization.
906-982: Complete coverage for graphics XML variations.These tests validate the serialization and deserialization of graphics configurations with various combinations of fields and empty properties, ensuring robust handling of VNC and other display options.
1182-1243: Good tests for memory and CPU value handling.These tests ensure proper parsing of string values and attributes for memory and vcpu elements, which is essential for correctly interpreting VM configurations.
1245-1254: Important validation of error handling for invalid XML.This test ensures that the parsing function properly rejects invalid XML inputs with appropriate error messages, improving the robustness of the API.
1256-1321: Good tests for loader configurations in domain XML.These tests verify that both single and multiple loader configurations are correctly serialized and deserialized, which is important for UEFI boot support.
1323-1393: Excellent round-trip conversion test.This test validates that a domain description can be converted to XML and back to a domain description without losing information, which is crucial for the overall reliability of the XML handling.
1395-1449: Comprehensive tests for attribute handling.These tests verify the correct handling of different attribute formats and optional attributes for memory and CPU configurations.
1451-1476: Good error handling and edge case validation.These tests ensure that invalid XML is rejected and minimal domain configurations are handled correctly.
1478-1503: Good handling of edge cases for device types.These tests verify that unknown device types are handled gracefully and known types like ACPI are serialized correctly.
1505-1519: Proper domain ID handling.These tests ensure that domain IDs are correctly serialized and deserialized, which is important for domain identification.
__tests__/helpers.ts (4)
1-8: Well-structured test helper imports and constants.Good organization with clear import statements and constants for test VM name and disk image path.
42-61: Good QEMU verification function.This function properly verifies the existence of required QEMU components before tests run, which prevents confusing test failures due to missing prerequisites.
154-164: Good disk image creation function.This function creates a test disk image with proper error handling and logging.
166-187: Well-structured test environment setup.This function provides a clean, sequential setup process for test environments, ensuring all prerequisites are in place before tests run.
| throw error; | ||
| } finally { | ||
| // Clear the domain reference before cleanup | ||
| domain = null; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider maintaining domain reference for cleanup
Setting domain = null in the finally block might interfere with the cleanup in the afterAll hook. Consider removing this line or setting a flag to indicate that cleanup has already been performed.
🏁 Script executed:
#!/bin/bash
# Check if the domain reference is needed for cleanup in afterAll
grep -B 5 -A 15 "afterAll" __tests__/domain-lifecycle.test.tsLength of output: 1255
Action Required: Preserve Domain Reference Until Global Cleanup
Based on the inspection of the tests/domain-lifecycle.test.ts file, the afterAll hook checks for the existence of the domain reference (via an if (domain) condition) to perform its cleanup routine before resetting it to null. Setting domain = null in an earlier finally block (as shown on line 154) risks clearing this reference too soon, which would prevent the cleanup code in afterAll from executing its intended actions.
Please Address the Following:
- Either remove the premature
domain = null;assignment from the finally block so thatafterAllcan correctly detect and clean up the domain. - Or replace it with logic (such as a flag) that indicates cleanup has already been performed, ensuring that
afterAllbehaves accordingly.
| sudo chmod -R 777 /var/run/libvirt | ||
| sudo chmod -R 777 /var/lib/libvirt |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use more restrictive permissions for libvirt directories
Setting directories to 777 permissions is overly permissive and could be a security concern. Consider using more restrictive permissions that still allow the necessary access.
- sudo chmod -R 777 /var/run/libvirt
- sudo chmod -R 777 /var/lib/libvirt
+ sudo chmod -R 755 /var/run/libvirt
+ sudo chmod -R 755 /var/lib/libvirt
+ sudo chown -R $(id -u):$(id -g) /var/run/libvirt
+ sudo chown -R $(id -u):$(id -g) /var/lib/libvirt📝 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.
| sudo chmod -R 777 /var/run/libvirt | |
| sudo chmod -R 777 /var/lib/libvirt | |
| sudo chmod -R 755 /var/run/libvirt | |
| sudo chmod -R 755 /var/lib/libvirt | |
| sudo chown -R $(id -u):$(id -g) /var/run/libvirt | |
| sudo chown -R $(id -u):$(id -g) /var/lib/libvirt |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/main.yml (3)
42-44: Updated Cache APT Packages:
The caching step now includesqemu-systemandqemu-user-staticalong withlibvirt-dev. Please verify that these additional packages are indeed required for your workflow and are available on the runner environment.
85-91: Creating libvirt Directories with Broad Permissions:
The workflow creates required libvirt directories and applies777permissions to ensure unrestricted access during CI runs. While this is acceptable for a controlled testing environment, consider tightening the permissions if this configuration is ever applied outside of CI to minimize potential security risks.
96-96: Remove Trailing Whitespace:
Static analysis indicates trailing spaces on this line. Removing them will help maintain clean YAML formatting.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 96-96: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
.github/workflows/main.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/main.yml
[error] 96-96: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
.github/workflows/main.yml (5)
75-79: Added Docker Buildx Setup:
The new step for setting up Docker Buildx is correctly implemented. This configuration is essential for building multi-architecture images, and theinstall: trueflag ensures the Buildx installation is enforced.
80-84: Configured QEMU Setup:
The integration of QEMU to support thelinux/amd64andlinux/arm64platforms is a solid addition. Ensure these are the intended target architectures and that any dependency on QEMU in your integration tests is well-documented.
92-117: Implementation of Docker Integration Tests:
The integration tests are comprehensive—they build Docker images for bothamd64andarm64platforms and run container tests with necessary privileges. Please re-confirm that using--privilegedmode and the specified volume bindings is strictly needed, as they grant elevated access. Additionally, these tests assume that all necessary libvirt services will be correctly mounted inside the container.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 96-96: trailing spaces
(trailing-spaces)
118-124: Upload Test Artifacts Step:
The step to upload test artifacts is clearly configured with a 7-day retention policy. This will ensure that any logs or output from the integration tests are preserved for debugging purposes if needed.
126-132: Upload Coverage Reports Step:
This step uploads the coverage reports as artifacts and uses a consistent retention period of 7 days. Verify that the coverage directory is correctly generated by your tests so that the expected files are available for upload.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
__tests__/helpers.ts (4)
9-56: Architecture detection implementation can be simplified.The function correctly determines architecture-specific configurations for QEMU, but the conditional branches for macOS and other platforms contain identical code paths for each architecture.
export function getArchConfig(): { arch: string; machine: string; firmware?: { code: string; vars: string }; emulator: string } { // Check for QEMU in Homebrew path on macOS const homebrewQemuPath = process.platform === 'darwin' ? '/opt/homebrew/bin' : null; const systemQemuPath = '/usr/bin'; // Function to find QEMU binary const findQemuBinary = (arch: string): string => { const binaryName = `qemu-system-${arch}`; if (homebrewQemuPath && existsSync(`${homebrewQemuPath}/${binaryName}`)) { return `${homebrewQemuPath}/${binaryName}`; } if (existsSync(`${systemQemuPath}/${binaryName}`)) { return `${systemQemuPath}/${binaryName}`; } throw new Error(`QEMU binary ${binaryName} not found in ${homebrewQemuPath} or ${systemQemuPath}`); }; - if (process.platform === 'darwin') { - if (process.arch === 'arm64') { - return { - arch: 'aarch64', - machine: 'virt', - emulator: findQemuBinary('aarch64') - }; - } else { - return { - arch: 'x86_64', - machine: 'q35', - emulator: findQemuBinary('x86_64') - }; - } - } else { - if (process.arch === 'arm64') { - return { - arch: 'aarch64', - machine: 'virt', - emulator: findQemuBinary('aarch64') - }; - } else { - return { - arch: 'x86_64', - machine: 'q35', - emulator: findQemuBinary('x86_64') - }; - } - } + // The configuration is the same regardless of platform, only depends on architecture + if (process.arch === 'arm64') { + return { + arch: 'aarch64', + machine: 'virt', + emulator: findQemuBinary('aarch64') + }; + } else { + return { + arch: 'x86_64', + machine: 'q35', + emulator: findQemuBinary('x86_64') + }; + } }
89-111: Disk cleanup could use safer string interpolation.The function effectively cleans up disk images and NVRAM files, but uses direct string interpolation in shell commands which could be a security risk if the constants were user-provided.
Consider using path sanitization or escaping for the shell commands, even though in this case the paths are constants defined in the file.
113-177: Domain cleanup logic is thorough but could be simplified.The domain cleanup function has comprehensive error handling but contains some redundant checks and could be streamlined.
- On line 125, the condition
activeDomainIds.length > 0doesn't guarantee that one of those domains is the TEST_VM_NAME.- Lines 129-130 check if
existingDomainexists, but it will always exist if the previous line didn't throw.- On line 162-163, you repeat the domain lookup that was already done earlier.
Consider refactoring the flow to reduce redundant checks and domain lookups.
1-212: Consider adding a cleanup function for the entire test environment.The file provides functions for setting up the test environment, but is missing a comprehensive cleanup function that could be used in test teardown.
Consider adding a
teardownTestEnvfunction that:
- Cleans up the domain
- Closes the hypervisor connection
- Cleans up the disk image
This would complement the
setupTestEnvfunction and ensure proper resource cleanup after tests..github/workflows/main.yml (4)
85-91: Libvirt Directory Permissions
The workflow creates and adjusts permissions for libvirt directories usingchmod -R 777. While acceptable for a controlled CI environment, please verify that such permissive settings are not propagated to production. Consider whether more restrictive permissions might be feasible.
120-127: Quoting Shell Substitution in Docker Run
The docker run command maps the local test artifacts directory using a command substitution ($(pwd)). To prevent potential word-splitting issues (especially if the path contains spaces), it’s recommended to quote the substitution. For example:- -v $(pwd)/test-artifacts:/app/test-artifacts \ + -v "$(pwd)/test-artifacts":/app/test-artifacts \
96-96: Remove Trailing Spaces
A trailing space was detected on this line. Please remove any extraneous white space to keep the YAML clean.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 96-96: trailing spaces
(trailing-spaces)
108-108: Remove Trailing Spaces
Trailing spaces were detected on this line. Removing them will help maintain compliance with YAML lint rules.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 108-108: trailing spaces
(trailing-spaces)
Dockerfile (2)
2-2: Base Image Downgrade Verification
The base image has changed fromnode:22-bookworm-slimtonode:20-slim. Please verify that this downgrade does not adversely affect the application’s runtime requirements or dependency compatibility.
81-85: Directory Creation and Permissions for Libvirt User
Creating the configuration directory in/home/libvirt/.config/libvirtand setting ownership withchown -R libvirt:libvirtis appropriate. Just ensure that these permissions meet the minimum requirements for required functionality without overexposing the system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
.github/workflows/main.yml(2 hunks)Dockerfile(1 hunks)__tests__/helpers.ts(1 hunks)scripts/test-docker.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/test-docker.sh
🧰 Additional context used
🧬 Code Definitions (1)
__tests__/helpers.ts (2)
lib/hypervisor.ts (1)
Hypervisor(13-316)lib/types.ts (1)
HypervisorOptions(4-6)
🪛 actionlint (1.7.4)
.github/workflows/main.yml
93-93: shellcheck reported issue in this script: SC2046:warning:32:8: Quote this to prevent word splitting
(shellcheck)
🪛 YAMLlint (1.35.1)
.github/workflows/main.yml
[error] 96-96: trailing spaces
(trailing-spaces)
[error] 108-108: trailing spaces
(trailing-spaces)
🔇 Additional comments (11)
__tests__/helpers.ts (4)
1-7: Import statements and constants look good.The import statements are clean and focused, bringing in necessary dependencies for the test helpers. The constants for VM name and disk image path are well-defined.
58-86: Thorough QEMU installation verification.The function effectively validates the QEMU installation, providing clear error messages when components are missing. The platform-specific error guidance for macOS is particularly helpful.
One improvement would be to return the full
archConfigas you're already computing it, instead of returning it only after verification.
179-189: Disk image creation looks good.The function properly creates a disk image for testing and has appropriate error handling.
191-212: Test environment setup function is well-structured.The function orchestrates the setup process in a logical sequence:
- Clean up existing resources
- Create new resources
- Connect to the hypervisor
- Verify the environment
Returning both the connection and architecture configuration is helpful for test cases.
.github/workflows/main.yml (2)
42-43: Expanded APT Package Caching
The caching step now includes additional packages (qemu-systemandqemu-user-static) alongsidelibvirt-dev, which is a beneficial enhancement for integration tests involving virtualization components.
75-84: Addition of Docker Buildx and QEMU Setup
The new steps to set up Docker Buildx and QEMU ensure support for multi-architecture builds. This setup appears correct and will help in building and testing images for bothamd64andarm64.Dockerfile (5)
8-12: Build Stage Dependency Installation
The commands installing build dependencies (e.g.,build-essential,python3,libvirt-dev) and subsequent cleanup withrm -rf /var/lib/apt/lists/*are well structured, helping to keep the image lean.
38-49: Enhanced Test Stage Dependencies
In the test stage, additional packages related to QEMU, OVMF, and libvirt (e.g.,qemu-system-x86,qemu-system-arm,libvirt-daemon-system) have been added. This comprehensive dependency set should provide robust support for multi-architecture and virtualization tests.
51-57: Non-Root User and Permission Setup for Libvirt
The Dockerfile creates a non-root userlibvirt, adds it to thekvmgroup, and sets up the required directories. This is a good practice for minimizing potential security risks.
75-78: Libvirt Configuration & Security Considerations
Libvirt’s configuration is now set up by appending values to/etc/libvirt/libvirtd.conf. Note thatauth_unix_rw = "none"disables authentication for read/write access. This setting carries inherent security risks if the container were ever to run in an uncontrolled environment.
90-90: Container Startup Command Robustness
The CMD launches the libvirt daemon and then runs tests after a fixed delay (sleep 2). Verify that the sleep interval reliably allows enough time for libvirtd to initialize under various conditions. You might consider implementing a health check loop rather than a fixed sleep.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
__tests__/domain-listing.test.ts (2)
9-11: Avoid usinganytype for better type safety.The
connectionandarchConfigvariables are typed asany, which reduces TypeScript's ability to provide type checking and autocompletion. Consider using more specific types that match the expected structure of these objects.- let connection: any; - let domain: Domain | null = null; - let archConfig: any; + let connection: import('../lib').Hypervisor; + let domain: Domain | null = null; + let archConfig: { + arch: string; + machine: string; + emulator: string; + };
25-25: Consider using a more robust approach for waiting on domain state changes.Fixed timeouts (1000ms) might be brittle if the test environment is slower than expected. Consider polling the domain state instead.
- await new Promise(resolve => setTimeout(resolve, 1000)); + // Wait for domain to complete shutdown + let retries = 0; + while (retries < 10) { + try { + const info = await connection.domainGetInfo(domain); + if (info.state !== DomainState.RUNNING) { + break; + } + } catch (error) { + // Domain might no longer exist + break; + } + await new Promise(resolve => setTimeout(resolve, 500)); + retries++; + }Also applies to: 136-136
.github/workflows/main.yml (3)
95-101: Review Libvirt Directory Permission Settings
The step that creates libvirt directories with sudo privileges and applieschmod -R 777is functionally correct in a CI context. However, since 777 permissions are generally considered insecure, it would be beneficial to add an inline comment clarifying that these permissions are acceptable only in the ephemeral CI environment and not recommended for production use.
102-146: Multi-Architecture Docker Integration Testing Workflow
The multi-arch Docker integration test block is comprehensive. It builds both "builder" and "test" stages, manages cache directories, and then runs container tests with the necessary privileges. For enhanced robustness, consider addingset -e(or a stricter shell configuration likeset -euxo pipefail) at the beginning of the script block. This change would ensure that the step exits immediately if any command fails, improving error handling and debuggability.Proposed diff snippet for the shell script block (for example, add on the first line of the script):
-run: | +run: | + set -e🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 106-106: trailing spaces
(trailing-spaces)
[error] 118-118: trailing spaces
(trailing-spaces)
[error] 122-122: trailing spaces
(trailing-spaces)
106-106: Remove Trailing Whitespace
Static analysis has flagged the presence of trailing spaces on lines 106, 118, and 122. Please remove these extra spaces to adhere to YAML formatting standards.Example diff for one of the lines:
- mkdir -p test-artifacts + mkdir -p test-artifactsAlso applies to: 118-118, 122-122
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 106-106: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
.github/workflows/main.yml(2 hunks)__tests__/domain-listing.test.ts(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/main.yml
[error] 106-106: trailing spaces
(trailing-spaces)
[error] 118-118: trailing spaces
(trailing-spaces)
[error] 122-122: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (12)
__tests__/domain-listing.test.ts (5)
19-35: Clean domain state management in afterAll looks good.The cleanup logic properly checks the domain state before attempting to shut it down and handles potential errors gracefully. This ensures that resources are properly released even if tests fail.
39-88: Well-structured domain description with comprehensive configuration.The domain description provides a good example of API usage, covering essential VM configuration including memory, CPU, OS, and devices. This is valuable both as a test case and as documentation of the expected format.
94-97: Comprehensive testing of domain listing flags.The test effectively validates the behavior of all important domain listing flags (ACTIVE, INACTIVE, RUNNING, PERSISTENT, TRANSIENT) and their combinations. Good use of array assertions to verify expected behavior.
Also applies to: 99-104, 109-114, 116-126, 128-133
127-133: Good use of bitwise operations for flag combination.The test demonstrates how to correctly combine multiple flags using the bitwise OR operator, which is an important pattern when working with this API.
135-139: Proper test cleanup ensures test isolation.The explicit cleanup at the end of the test ensures that resources are released even if the afterAll hook doesn't run, which helps maintain test isolation and prevents interference between test runs.
.github/workflows/main.yml (7)
42-44: Extended APT Cache Packages
The "Cache APT Packages" step now includes additional packages (qemu-systemandqemu-user-static) along withlibvirt-dev, which ensures that all dependencies required for the forthcoming libvirt and QEMU configurations are cached.
75-79: Set up Docker Buildx for Multi-Arch Builds
The integration of the Docker Buildx setup usingdocker/setup-buildx-action@v3is correctly implemented. This is essential for handling multi-architecture image builds and aligns well with the new Docker integration testing strategy.
80-84: Set up QEMU for Architecture Emulation
The configuration for QEMU viadocker/setup-qemu-action@v3with the platformslinux/amd64,linux/arm64is properly set up. This addition effectively supports the multi-arch builds in the later stages.
85-94: Caching Docker Build Cache for Improved Build Speeds
The Docker cache step is well-structured. The use of clearly defined keys and restore keys for both amd64 and arm64 caches should help in reducing rebuild times during image creation.
148-155: Save Docker Build Cache Step
The step dedicated to saving the updated Docker build caches for both architectures is correctly implemented. This will help subsequent builds leverage cached data and improve overall build efficiency.
156-163: Upload Test Artifacts
The step to upload test artifacts (with a retention period of 7 days) is clearly and correctly defined. It looks good as part of preserving build outputs for further analysis.
164-171: Upload Coverage Reports
The configuration for uploading coverage reports is properly set up to ensure that coverage data is preserved for review. This will help in later analysis and debugging if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/main.yml (3)
85-91: Libvirt Directories Creation with Open Permissions.
The workflow creates the required libvirt directories and sets their permissions to777so that they are fully accessible. While this is acceptable in a CI environment, consider double-checking that such open permissions are appropriate and do not inadvertently introduce security issues if reused in a different context.
96-96: Trailing Whitespace Detected.
There are extraneous trailing spaces on this line. Removing these will ensure adherence to YAML style guidelines.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 96-96: trailing spaces
(trailing-spaces)
108-108: Trailing Whitespace Detected.
Please remove the trailing spaces on this line to maintain clean YAML formatting.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 108-108: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
.github/workflows/main.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/main.yml
[error] 96-96: trailing spaces
(trailing-spaces)
[error] 108-108: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (8)
.github/workflows/main.yml (8)
42-44: APT Packages Update: Inclusion of QEMU Packages.
The caching step now includesqemu-systemandqemu-user-staticalong withlibvirt-dev. This change ensures that the necessary packages for multi-architecture Docker tests are pre-cached, which can help speed up the workflow execution.
75-79: Docker Buildx Setup Step.
This step sets up Docker Buildx (usingdocker/setup-buildx-action@v3) to facilitate multi-architecture builds. The configuration withinstall: trueis appropriate and aligns with the new testing strategy.
80-84: QEMU Setup for Multi-Architecture Testing.
The new QEMU setup action enables support for bothlinux/amd64andlinux/arm64platforms. This will help ensure that your Docker images build correctly across multiple architectures.
92-128: Comprehensive Docker Integration Tests Step.
This block implements integration tests that build and run Docker images for bothamd64andarm64architectures. The loop structure, caching strategy (--cache-fromand--cache-to), and the subsequentdocker runcommand with the required volume mounts are well-constructed.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 96-96: trailing spaces
(trailing-spaces)
[error] 108-108: trailing spaces
(trailing-spaces)
130-137: Upload Test Artifacts Step.
The configuration for uploading test artifacts is clear, with a defined artifact name and a 7-day retention period. This helps in retaining crucial test output for troubleshooting and review.
138-144: Upload Coverage Reports Step.
This step correctly uploads coverage reports with the specified retention and artifact naming. The configuration is consistent with the overall artifact management strategy in the workflow.
146-150: NPM Authentication Setup.
The step that sets up NPM authentication for publishing is implemented in a secure and standard manner by redirecting the auth token to~/.npmrc.
151-156: Publish to NPM Step.
The publishing step is conditionally executed based on the output of the release-please action, and it leverages the stored NPM authentication token. This configuration appears correct and follows best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
__tests__/domain-lifecycle.test.ts (1)
171-173:⚠️ Potential issueDomain reference is prematurely cleared.
Setting
domain = nullin the finally block could interfere with the cleanup in theafterAllhook, which uses this reference to determine if cleanup is needed.Remove this line to ensure proper cleanup in the
afterAllhook:} finally { // Clear the domain reference before cleanup - domain = null; }
🧹 Nitpick comments (2)
__tests__/domain-cleanup.test.ts (2)
9-11: Consider adding type definitions for variables.While using
anytype in test code is acceptable, consider adding more specific types for better code clarity and type safety.- let connection: any; + let connection: Hypervisor; - let archConfig: any; + let archConfig: ArchConfig;
99-141: Comprehensive test for domain undefining via hypervisor.This test correctly verifies the hypervisor-based approach to undefining domains. However, there's significant code duplication with the previous test.
Consider refactoring the common domain creation and verification logic into a helper function to reduce duplication between these two tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
__tests__/domain-cleanup.test.ts(1 hunks)__tests__/domain-lifecycle.test.ts(1 hunks)__tests__/domain-listing.test.ts(1 hunks)__tests__/hypervisor.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
__tests__/domain-cleanup.test.ts (3)
lib/types.ts (2)
Domain(130-130)LibvirtError(132-159)__tests__/helpers.ts (2)
setupTestEnv(192-212)TEST_VM_NAME(6-6)lib/domain-xml.ts (1)
domainDescToXml(300-412)
__tests__/domain-lifecycle.test.ts (3)
lib/types.ts (1)
Domain(130-130)__tests__/helpers.ts (3)
setupTestEnv(192-212)TEST_VM_NAME(6-6)DISK_IMAGE(7-7)lib/domain-xml.ts (1)
domainDescToXml(300-412)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (24)
__tests__/domain-cleanup.test.ts (6)
1-7: Appropriate imports for the test suite.The imports correctly include all necessary modules for the domain cleanup tests - Vitest test framework, domain-related types and functions, and helper utilities.
13-17: Good test environment setup.The
beforeAllhook correctly sets up the test environment using the shared helper function. The timeout of 10 seconds provides sufficient time for the setup operations to complete.
19-35: Thorough domain cleanup before each test.The
beforeEachhook properly handles existing domain cleanup, including state checking, graceful shutdown, and undefining. The error handling is appropriate for a cleanup routine.
37-53: Proper resource cleanup after all tests.The
afterAllhook correctly manages resource cleanup, handling potential errors during the process. Good practice to set references to null after cleanup.
55-97: Well-structured test for domain undefining via domain object.The test appropriately creates a domain configuration, defines it, verifies its existence, undefines it through the domain object's method, and then verifies its removal. The 60-second timeout provides adequate time for VM operations.
143-155: Good error handling test for non-existent domains.The test appropriately validates the error handling behavior when attempting to work with non-existent domains, including checking the error type and properties.
__tests__/domain-lifecycle.test.ts (3)
1-7: Appropriate imports for lifecycle testing.The imports correctly include all necessary modules for domain lifecycle tests, including the child_process module for executing system commands.
8-53: Well-structured test setup and teardown.The setup and teardown hooks provide proper isolation for each test and handle resource cleanup appropriately.
55-170: Comprehensive VM lifecycle test.The test thoroughly covers the domain lifecycle including creation, starting, and stopping. It properly handles firmware setup, XML generation, disk image creation, and VM state verification.
__tests__/domain-listing.test.ts (7)
1-7: Appropriate imports for domain listing tests.The imports correctly include required modules for testing domain listing functionality, including the essential ConnectListAllDomainsFlags enum.
8-53: Well-structured test setup and teardown.The lifecycle hooks appropriately manage the test environment setup and cleanup, following the same pattern as other test files for consistency.
55-106: Detailed domain configuration.The test creates a comprehensive domain configuration with appropriate memory, CPU, OS, and device settings. The memory unit is explicitly set to KiB, which is a good practice for clarity.
108-122: Good verification of inactive domains.The test correctly verifies domain creation and its presence in the inactive domains list before starting it.
123-132: Proper testing of running domains.The test appropriately starts the domain and verifies its presence in the running domains list.
133-144: Comprehensive domain flags testing.The test checks both persistent and transient domain flags, verifying expected behavior for the defined domain.
145-156: Proper cleanup after testing.The test correctly cleans up resources after testing by shutting down and undefining the domain.
__tests__/hypervisor.test.ts (8)
1-7: Appropriate imports for hypervisor testing.The imports include all necessary components for testing hypervisor functionality, including the DomainBuilder for creating domain configurations.
8-53: Well-structured test setup and teardown.The test suite follows the established pattern for setup and teardown, maintaining consistency with other test files.
55-77: Good coverage of connection operations.The test verifies essential hypervisor connection functionality like node information, maximum VCPUs, and hostname retrieval, with appropriate error handling for unsupported features.
79-129: Effective use of DomainBuilder.The test utilizes the DomainBuilder pattern effectively to create a domain configuration, which is more maintainable than direct object creation.
130-182: Comprehensive domain management testing.The test thoroughly covers domain lifecycle operations including creation, information retrieval, state management, and cleanup, with appropriate error handling validation.
184-196: Concise domain listing tests.The test efficiently verifies all domain listing methods with appropriate type checking.
198-266: Thorough domain creation and lookup testing.The test comprehensively verifies domain creation and lookup operations, including the use of the UUID for domain identification and proper error handling.
268-278: Good error case coverage.The test thoroughly validates error handling for common failure scenarios like invalid XML, domain names, and UUIDs, ensuring proper error types are thrown.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/integration-test.yml (1)
33-39: Libvirt Directory Creation and Permissions
The script creates the required libvirt directories and sets permissions usingchmod -R 777. While this approach is acceptable in a CI testing environment, it may be worth revisiting the permissiveness for improved security if this logic is ever used in a production-like context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
.github/workflows/integration-test.yml(1 hunks).github/workflows/main.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/main.yml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Integration Tests (arm64)
- GitHub Check: Docker Integration Tests (amd64)
🔇 Additional comments (8)
.github/workflows/integration-test.yml (8)
1-9: Workflow Trigger Configuration Looks Good
The workflow is triggered on pushes, pull requests, and manual dispatch (workflow_dispatch) for themainbranch. This comprehensive trigger setup ensures that integration tests run in all the required scenarios.
10-18: Job Definition and Matrix Strategy are Configured Properly
Thedocker-integrationjob definition leverages a matrix strategy to test on bothamd64andarm64architectures. The use offail-fast: falseguarantees that tests for all defined platforms are executed even if one fails.
19-27: Checkout and Docker Buildx Setup are Implemented Correctly
Usingactions/checkout@v4ensures the repository is checked out cleanly, and the Docker Buildx setup withdocker/setup-buildx-action@v3(withinstall: true) is correctly configured to enable multi-platform builds.
28-32: QEMU Setup is Appropriately Configured
The step that sets up QEMU withdocker/setup-qemu-action@v3and explicitly specifies platforms (linux/amd64,linux/arm64) adequately prepares the environment for executing binaries across these architectures.
40-44: Dependencies Installation Step is Adequate
Updating the package list and installingqemu-user-staticensures that the runner is equipped to handle multi-architecture builds. This step is straightforward and correctly implemented.
45-59: Docker Build Command is Well Structured
The build step makes good use of Docker Buildx with multi-platform support. The use of--build-arg BASE_IMAGE=node:20-slim, caching parameters (--cache-fromand--cache-to), and the--loadflag helps in creating reproducible builds efficiently.
60-68: Container Run Step is Configured Correctly
The docker run command includes crucial flags such as--rm,--privileged, and correctly mounts volumes for the libvirt socket, libvirt directory, and test artifacts. This configuration ensures the container receives the necessary privileges and access to host directories during the test run.
69-75: Artifact Upload Step is Correctly Configured
Using theactions/upload-artifact@v4step with anif: always()condition ensures that test artifacts are uploaded regardless of test results. Specifying a retention period of 7 days is appropriate for integration testing purposes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
127-143: Updated Examples Section with pnpm Commands
The "Examples" section now clearly demonstrates how to usepnpmto run various domain operations (listing, starting, shutting down, and creating domains using the builder pattern).Consider adding brief inline comments for each command to further clarify their purpose for new users.
147-158: Contribution Guidelines and Tone
The "Contribute" section has been updated with clear steps to install dependencies, run linting, and execute tests.
- While the enthusiastic tone ("Any contribution is welcome!") is inviting, consider moderating the use of exclamation marks to maintain a professional tone (as suggested by LanguageTool).
- Also, verify that legacy references (for example, "vmngr/libvirt contributers" in the copyright notice) are updated if the repository ownership has changed.
🧰 Tools
🪛 LanguageTool
[style] ~147-~147: Using many exclamation marks might seem excessive (in this case: 9 exclamation marks for a text that’s 2038 characters long)
Context: ... Contribute Any contribution is welcome! To check whether your contribution conf...(EN_EXCESSIVE_EXCLAMATION)
186-194: Updated Badge Definitions
The badge URL definitions at the bottom of the file have been updated to reflect the new repository location (unraid/libvirt). This ensures consistency throughout the README.It may be worth reviewing other sections (such as contributor notices) for similar legacy references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
cover.pngis excluded by!**/*.png
📒 Files selected for processing (1)
README.md(4 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~147-~147: Using many exclamation marks might seem excessive (in this case: 9 exclamation marks for a text that’s 2038 characters long)
Context: ... Contribute Any contribution is welcome! To check whether your contribution conf...
(EN_EXCESSIVE_EXCLAMATION)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Integration Tests (arm64)
- GitHub Check: Docker Integration Tests (amd64)
🔇 Additional comments (7)
README.md (7)
7-8: Badge Additions for Tests and Coverage
The new "Tests" and "Coverage" badges have been added at these lines. They follow the established format and help improve the visibility of test status and code coverage.
13-14: Badge Additions for NPM Version and Last Update
The newly introduced badges for "NPM Version" and "Last Update" provide additional valuable information about the package state. Their inclusion is consistent with the other visual indicators.
84-85: Updated Debian/Ubuntu Installation Instructions
The installation command now includesqemu-systemalong with other essential packages. This update ensures that users on Debian/Ubuntu systems install all required dependencies.
93-94: Updated MacOS Installation Instructions
The MacOS installation command has been updated to install bothlibvirtandqemuusing Homebrew, aligning with the new dependency requirements.
97-103: New Development Section Addition
A "Development" section has been added to guide contributors on usingpnpmas the package manager. The instructions—including the global installation command (npm install -g pnpm)—are clear and useful.
105-110: Building Instructions Updated for pnpm
The "Building" section now instructs users to runpnpm installfollowed bypnpm build. This update is consistent with the project’s switch topnpmand provides clear guidance.
112-122: Testing Instructions Updated for Vitest
The updated testing instructions now leverage Vitest commands (pnpm test,pnpm test:coverage, andpnpm test:ui). This not only clarifies the testing strategy but also aligns with the corresponding changes in the test framework.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Dockerfile (2)
7-12: Installation of Build Dependencies
The build stage properly installs essential packages (build-essential,python3, andlibvirt-dev). For enhanced reproducibility, consider pinning critical package versions if you encounter any build inconsistencies over time.
89-90: libvirtd Startup and Test Execution
Usingsleep 2after startinglibvirtdis a quick fix to allow the daemon to initialize. However, this static delay may be unreliable under varying system loads. Consider implementing a dynamic wait (e.g., checking for an open socket or a readiness flag) to ensurelibvirtdhas started before runningpnpm test:coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
.github/workflows/integration-test.yml(1 hunks)Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/integration-test.yml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Integration Tests (arm64)
- GitHub Check: Docker Integration Tests (amd64)
🔇 Additional comments (13)
Dockerfile (13)
1-2: Base Image and Stage Naming Update
The builder stage now usesnode:20-slim(with stage aliasbuilder) instead of the previousnode:22-bookworm-slim. Please verify that this downgrade is intentional and that all dependencies and build scripts remain compatible with Node.js 20.
4-6: Set Non-Interactive Environment Variable
DefiningDEBIAN_FRONTEND=noninteractiveis an excellent choice to suppress interactive prompts during package installation.
15-20: Caching Package Files for Efficient Builds
Copyingpackage*.jsonandpnpm-lock.yamlprior to the source files optimizes the build cache. This is a standard best practice that helps speed up subsequent builds.
21-24: Installing pnpm and Project Dependencies
Installingpnpmglobally and then invokingpnpm install --frozen-lockfileis clear and effective for managing dependencies.
25-29: Copying Source Files and Building the Project
Copying the full source and executing the build command (pnpm run build) adheres to a robust multi-stage build strategy.
32-33: Test Stage Base Image Consistency
The test stage also usesnode:20-slim, ensuring consistency between build and test environments.
34-50: Installing Test Dependencies
The test stage installs a comprehensive set of dependencies including various QEMU packages and libvirt tools, which are crucial for your integration tests. Please verify that these packages are fully supported on the chosen base image and meet your test requirements.
51-57: Non-root User Setup for libvirt
Creating a non-root user (libvirt) and adding it to thekvmgroup, with appropriate directory permissions for/var/run/libvirt, enhances the container’s security posture.
59-66: Test Environment Dependency Installation
Repeating the dependency installation steps in the test stage (copying package files, installingpnpm, and running dependency installs) ensures that the test environment is self-contained. This mirrors the build stage, which is typical in multi-stage Docker builds.
67-73: Transferring Build Artifacts and Test Files
Copying built files and configuration (from/app/dist,/app/lib, and test folders) from the builder stage into the test stage is correctly implemented to reuse artifacts.
74-78: Setting Up libvirt Configuration
The Dockerfile configures libvirt by updating/etc/libvirt/libvirtd.confwith settings likeunix_sock_groupandunix_sock_rw_perms, and notably setsauth_unix_rwto"none". A previous review raised concerns regarding the security implications of disabling authentication for the read-write socket. Please revalidate that this configuration is safe given your container environment.
80-85: User Directories and Permissions Setup
Creating the directory/home/libvirt/.config/libvirtwith a default configuration file and setting the correct ownership for both this directory and/appis implemented well.
86-88: Switching to a Non-root User
Switching to the non-rootlibvirtuser before executing the final command reinforces container security practices.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
147-158: Contribution Guidelines Enhancement
The instructions for contributing now cover installing dependencies, linting, and testing. This sets clear expectations for contributors.
Consider toning down the exclamation mark in the opening sentence for a more formal, professional tone. For example:-Any contribution is welcome! To check whether your contribution conforms to our style guide, run the following tasks: +Any contribution is welcome. To check whether your contribution conforms to our style guide, run the following tasks:🧰 Tools
🪛 LanguageTool
[style] ~147-~147: Using many exclamation marks might seem excessive (in this case: 9 exclamation marks for a text that’s 2036 characters long)
Context: ... Contribute Any contribution is welcome! To check whether your contribution conf...(EN_EXCESSIVE_EXCLAMATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
.github/workflows/integration-test.yml(1 hunks)README.md(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/integration-test.yml
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~147-~147: Using many exclamation marks might seem excessive (in this case: 9 exclamation marks for a text that’s 2036 characters long)
Context: ... Contribute Any contribution is welcome! To check whether your contribution conf...
(EN_EXCESSIVE_EXCLAMATION)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Integration Tests (arm64)
- GitHub Check: Docker Integration Tests (amd64)
🔇 Additional comments (9)
README.md (9)
7-8: New Badge Additions: Tests and Codecov
The addition of the "Tests badge" and "Codecov badge" enhances visibility into your project's testing status and code coverage. The URLs are correctly updated to point to the new repository location underunraid.
13-14: New Badge Additions: NPM Version and Last Update
These badges provide clear insights into the current published package version and the timestamp of the last commit. Their inclusion aids users in quickly assessing the project's state.
83-86: Updated Installation Instructions for Debian/Ubuntu
The installation command now includesqemu-system, which aligns with the updated Docker integration and testing requirements. This change improves clarity and ensures that users install all necessary dependencies for the library to work correctly.
92-95: Updated Installation Instructions for MacOS
The revised command now installs bothlibvirtandqemuvia Homebrew. This update matches the new dependency requirements and ensures consistency with the Debian/Ubuntu instructions.
97-103: Addition of the Development Section
Introducing a dedicated "Development" section with instructions for usingpnpmhelps clearly differentiate between using the package as a consumer (vianpm) and contributing to its development (viapnpm). The guidance is concise and appropriately placed.
107-110: Building Commands Usingpnpm
The build instructions are straightforward, instructing users to install dependencies and build the project usingpnpm. This aligns well with the newly introduced development practices.
116-125: Enhanced Testing Instructions
The testing section now provides commands for running tests, generating coverage reports, and launching a UI for tests. This comprehensive set of instructions will help contributors verify changes and ensure quality.
131-143: Updated Examples Section
The examples section now details various commands (listing domains, starting, shutting down, and using the builder pattern) that demonstrate the library's functionality. These clear examples will be very helpful for new users.
186-194: Updated Badge Definitions in the Footer
The badge URLs have been updated to reflect the move fromvmngrtounraid, ensuring consistency throughout the documentation. These changes improve the accuracy of repo references for License, CI, Tests, Codecov, and more.
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Introduced asynchronous operations for virtual machine domain management, enabling smoother domain destruction and removal. - Added a new shell script for Docker-based integration testing across multiple architectures. - Added new methods for domain management in the hypervisor context, including domain destruction and undefining. - Introduced a `Domain` class for comprehensive management of virtual machine instances. - Added a new GitHub Actions workflow for automated Docker integration tests across multiple architectures. - **Enhancements** - Improved Docker container configuration with a secure non-root user setup for safer deployments. - Expanded integration tests across multiple architectures and enhanced error handling to provide clearer feedback and improved reliability. - Enhanced XML serialization and deserialization processes for domain configurations, improving robustness against edge cases. - Updated testing framework to utilize Vitest, replacing previous testing tools and enhancing coverage reporting. - Added detailed documentation for several interfaces and enums to improve clarity. - Updated README with new badges and installation instructions, along with a new "Development" section. - **Chores** - Updated CI and dependency management to boost build stability and overall system consistency. - Introduced a new testing configuration for Vitest to streamline test execution and coverage reporting. - Updated system requirements in package management for better compatibility. - Modified `.gitignore` to include new entries for coverage and test artifacts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [2.0.0](v1.2.2...v2.0.0) (2025-03-30) ### ⚠ BREAKING CHANGES * major api improvements ([#37](#37)) ### Features * major api improvements ([#37](#37)) ([5b7fa01](5b7fa01)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Domainclass for comprehensive management of virtual machine instances.Enhancements
Chores
.gitignoreto include new entries for coverage and test artifacts.