feat: basic docker controls #1292
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis update resets configuration parameters in the Unraid server config and introduces several Docker-related enhancements. The GraphQL schema is extended with a new Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as DockerMutationsResolver
participant S as DockerService
participant D as Docker Engine
C->>R: startContainer(id)
R->>S: startContainer(id)
S->>D: initiate start container
S->>S: refresh container list (getContainers)
S->>R: return started container info
R->>C: deliver container details
C->>R: stopContainer(id)
R->>S: stopContainer(id)
S->>D: initiate stop container
S->>S: refresh container list (getContainers)
S->>R: return stopped container info
R->>C: deliver container details
Suggested reviewers
Poem
🪧 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: 0
🧹 Nitpick comments (17)
api/src/unraid-api/graph/resolvers/array/array.resolver.ts (1)
21-23: Consider using service layerThe
array()method still uses the direct store access pattern withgetArrayData(store.getState). Consider refactoring this to use the newly added ArrayService for consistency across the resolver.public async array() { - return getArrayData(store.getState); + return this.arrayService.getArrayData(); }api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
31-31: Refactored containers methodThe containers method now uses the injected DockerService instead of a dynamic import, which is a cleaner approach. The
falseparameter should be documented for clarity on what it represents.public async containers() { - return this.dockerService.getContainers(false); + // Parameter `false` indicates non-detailed container info + return this.dockerService.getContainers(false); }api/src/unraid-api/graph/resolvers/array/array.resolver.spec.ts (1)
15-29: Good test setup for ArrayService integration.The mock implementation properly provides all required methods for array management functionality. This setup enables proper testing of the ArrayResolver with isolated dependencies.
Consider adding tests to verify interactions between the resolver and the mocked service methods, similar to the approach used in the DockerResolver tests.
api/src/core/utils/clients/emcmd.ts (1)
31-40: Improved error handling for socket operations.The addition of enableUnixSockets and specific error handling for ENOENT provides better diagnostics when the emhttpd socket is unavailable.
There's an inconsistency in how options are handled. The code constructs
optionsobject withqsproperty on lines 16-22 but then doesn't use it in the got.get call. Instead, it directly sets searchParams. Consider removing the unused options construction or updating it to match the actual usage.- const options = { - qs: { - ...commands, - csrf_token: csrfToken, - }, - };And update the DRY_RUN logger to:
- logger.debug(url, options); + logger.debug(url, { searchParams: { ...commands, csrf_token: csrfToken } });api/src/graphql/schema/types/array/array.graphql (2)
6-11: Consider additional array states.
While these two states are likely sufficient for now, you may eventually need to handle a "RESTART" or other transitional states.
23-25: Confirm array state before disk operations.
The schema implies these operations may fail if the array is started. Advise verifying or blocking calls when the array is in a running state.api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.spec.ts (3)
33-35: Testing if resolver is defined.
Add more checks to ensure necessary dependencies, likedockerService, are also properly initialized.
37-54: Test for startContainer is solid.
Consider adding a negative test case: e.g., container not found or API error.
56-73: Test for stopContainer is similarly robust.
A negative scenario test (like the container is already stopped) can help confirm proper error handling.api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
13-22: startContainer method.
Consider checking if the container is already running to avoid redundant calls.api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (1)
43-51: mountArrayDisk field.
This approach is good; consider clarifying any prerequisites for mounting.api/dev/Unraid.net/myservers.cfg (1)
7-15: Confirm disabling remote access and clearing credentials.
Resetting these fields to empty strings disables remote access and clears API keys and user info. Ensure this is intended and doesn't disrupt remote management or break existing integrations.Shall I open a new issue to document & track reintroducing secure remote access if needed?
api/src/unraid-api/graph/resolvers/array/array.service.ts (5)
19-20: Validate concurrency handling forpendingState.
ThependingStatefield prevents reentry, but additional concurrency checks (e.g., locks) might be advisable if multiple requests hit simultaneously.
29-68: Enhance logging and error recovery inupdateArrayState.
Overall logic is solid, but consider logging the old and new states for audit. Also, ifemcmdfails silently, you might want to handle or retry.
87-102: Extend validation forremoveDiskFromArray.
Similar to adding a disk, removing a disk could fail if the disk isn’t part of the array. A direct check could improve error reporting.
104-116: Handle potential mount errors inmountArrayDisk.
You confirm the array is running, which is good practice. Consider capturing and surfacingemcmderrors more robustly in case mounting fails.
118-130: Improve error context forunmountArrayDisk.
Ifemcmdfails, diagnosing the unmount issue becomes challenging. Enhance error messages or logs to clarify the reason.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
api/src/graphql/generated/api/operations.tsis excluded by!**/generated/**api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (19)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/dev/states/myservers.cfg(2 hunks)api/src/core/modules/array/get-array-data.ts(0 hunks)api/src/core/utils/clients/emcmd.ts(2 hunks)api/src/graphql/schema/types/array/array.graphql(1 hunks)api/src/graphql/schema/types/docker/docker.graphql(1 hunks)api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)
💤 Files with no reviewable changes (1)
- api/src/core/modules/array/get-array-data.ts
🧰 Additional context used
🧬 Code Definitions (5)
api/src/unraid-api/graph/resolvers/array/array.resolver.ts (2)
api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (1)
Resolver(9-72)api/src/graphql/generated/api/types.ts (1)
Resolver(1886-1886)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts (1)
Resolver(8-31)
api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
Resolver(8-40)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
docker(18-22)
api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (3)
api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts (1)
Resolver(3-11)api/src/unraid-api/graph/resolvers/array/array.resolver.ts (1)
Resolver(11-34)api/src/graphql/generated/api/types.ts (3)
Resolver(1886-1886)ArrayStateInput(312-315)ArrayDiskInput(194-199)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (57)
api/dev/states/myservers.cfg (2)
2-2: API version updateThe API version has been updated from "4.1.3" to "4.4.1". Make sure this version change is intentional and compatible with the rest of the codebase.
23-23: Connection status changeThe minigraph connection status has been changed from "PRE_INIT" to "ERROR_RETRYING". This suggests the system is now attempting to recover from an error state rather than being in pre-initialization. Verify this is the intended behavior with the new docker controls being introduced.
api/src/unraid-api/graph/resolvers/array/array.resolver.ts (2)
9-9: Service import addedGood addition of the ArrayService import to implement proper service layering.
13-13: Service injectionThe ArrayService is now properly injected into the ArrayResolver constructor, allowing for better testing and separation of concerns.
api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts (1)
1-11: New mutation resolver implementationThe implementation of the MutationResolver with the array() method that returns a typed object is clean and follows GraphQL resolver patterns. This provides a proper entrypoint for array mutations.
For completeness, you might want to add a method for Docker mutations as well since this PR is about Docker controls:
@ResolveField() public async docker() { return { __typename: 'DockerMutations', }; }api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (3)
6-6: Service import addedGood addition of the DockerService import to implement proper service layering.
10-10: Service injectionThe DockerService is now properly injected into the DockerResolver constructor, allowing for better testing and separation of concerns.
34-39: New mutations methodGood addition of the mutations method that provides an entrypoint for Docker mutations. Based on the related code snippets, this will enable start and stop container operations.
api/src/graphql/schema/types/docker/docker.graphql (2)
11-14: Well-implemented Docker container control mutations.The new DockerMutations type provides clear and concise operations for starting and stopping containers. The interface is straightforward with appropriate parameter and return types.
16-17: Good extension of the Docker type.Extending the Docker type to include mutations follows good GraphQL practices by logically grouping related functionality.
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (3)
36-39: Good test coverage for the docker method.The test verifies that the docker method returns an object with the expected ID.
41-71: Comprehensive test for container retrieval.This test effectively verifies that:
- The containers method correctly calls the service
- The correct arguments are passed to getContainers
- The result is properly returned from the resolver
The mock data includes all relevant container properties, making this a thorough test.
73-76: Good test for mutations object.The test verifies that the mutations method returns an object with the expected ID.
api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
6-8: Feature addition looks well-organized.You've properly added the new Array and Docker mutation resolvers along with their required services. The imports and provider registrations follow the established pattern in the module.
Also applies to: 13-15, 21-21, 35-36, 43-44, 46-46
api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts (3)
8-11: Resolver implementation looks good.The class is properly decorated and follows the standard NestJS resolver pattern with dependency injection.
12-20: Permission protection is appropriate for container start operation.The method correctly implements a GraphQL field resolver with proper permission checks for Docker resource updates. The implementation delegates to the service layer as expected.
22-30: Permission protection is appropriate for container stop operation.Similar to the start operation, this method correctly implements permission checks and delegates to the service layer.
api/src/unraid-api/graph/resolvers/array/array.service.spec.ts (6)
13-25: Mocking approach is clean and effective.The test properly mocks external dependencies to isolate the unit tests.
31-67: Test setup is comprehensive with good mock data.The test module setup and mock data initialization provide a solid foundation for the tests. The mock array data includes all necessary properties for testing.
73-83: Array state update test looks good.Test verifies both the result and that the correct command was sent to the emcmd client.
85-109: Disk addition and removal tests are thorough.Both tests properly verify that the correct parameters are passed to emcmd when adding and removing disks.
111-157: Mount, unmount, and clear statistics tests are comprehensive.These tests properly mock the array as running before testing operations that require a running array. They verify both the return value and the parameters passed to emcmd.
159-209: Error handling tests cover important edge cases.The tests verify that operations throw appropriate errors when attempted in invalid array states, which is critical for preventing corrupted states.
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (6)
12-21: Clean mocking approach for Docker dependencies.The test properly mocks the Docker client and getDockerContainers function to isolate the service tests.
27-43: Good test setup with mock container.The test module setup includes creating a mock container with the necessary methods for testing.
45-64: Container retrieval test is thorough.The test verifies both the returned result and that getDockerContainers was called with the correct parameters.
66-87: Container start operation test is well-implemented.The test verifies the returned container data, that the correct container was retrieved, and that start was called on the container.
89-110: Container stop operation test is comprehensive.Similar to the start test, this one verifies all the expected method calls and the returned data.
112-124: Error handling tests cover important edge cases.The tests verify that appropriate errors are thrown when containers aren't found after operations, which would help with debugging.
api/src/graphql/schema/types/array/array.graphql (4)
13-16: No issues here.
The input type naming and structure look good.
18-21: Validate edge cases in setState.
Ensure you handle scenarios where the array cannot transition to the requested state, such as transitioning fromSTARTtoSTOPwhen it’s already stopping.
33-35: Mutation bundling is consistent.
Exposingarray: ArrayMutationsat the rootMutationtype is aligned with recommended GraphQL patterns.
41-41: Validate disk slot if provided.
Consider whether negative or out-of-range slot values might cause unexpected behavior. Add validation where appropriate.api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.spec.ts (5)
1-2: Import statements look fine.
This introduces the NestJS testing module type correctly.
4-4: Added vitest imports.
All good for writing streamlined tests.
6-9: Docker-related imports are consistent.
No apparent issues with type or service imports.
11-14: Descriptive test suite naming.
Defining'DockerMutationsResolver'as the describe block is clear and straightforward.
15-31: Set up test module with mocks.
The approach to mock DockerService methods is correct. The module creation is straightforward.api/src/unraid-api/graph/resolvers/docker/docker.service.ts (5)
1-2: Injectable service import.
No issues.
3-6: Dependencies from core modules.
Looks good, minimal overhead.
7-8: DockerService class definition.
Implementation is straightforward and readable.
9-11: getContainers method looks fine.
It properly delegates togetDockerContainersand accommodates caching.
24-33: stopContainer method.
Similar logic tostartContainer. Also consider checking if it’s already stopped.api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (9)
1-2: GraphQL imports are appropriate.
No issues found.
3-4: Authorization imports.
This is consistent with the existing pattern for NestJS permission handling.
5-7: Array service import.
No issues. The usage is straightforward.
9-11: ArrayMutationsResolver defined.
Clear naming of'ArrayMutations'as the resolver type is consistent with the schema.
13-21: setState field.
VerifyupdateArrayStategracefully handles invalid transitions (like trying to stop an already stopped array).
23-31: addDiskToArray field.
Implementation is consistent with permission checks.
33-41: removeDiskFromArray field.
Looks aligned with the schema requirement (array must be stopped).
53-61: unmountArrayDisk field.
Implementation matches the pattern of mount/disk operations.
63-71: clearArrayDiskStatistics field.
No issues. Straightforward permission check.api/dev/Unraid.net/myservers.cfg (2)
5-5: Check potential side effects of changingsandboxto "no".
If logic depends onsandbox="yes"for safe testing or feature toggles, this change might unexpectedly disable features or testing flows.
2-3:❓ Verification inconclusive
Verify the impact of clearing
versionandextraOrigins.
By removing these configuration values, downstream logic dependent on them (e.g., version handling, cross-origin requests) may behave unexpectedly.Consider running:
🏁 Script executed:
#!/bin/bash # Searching usage of `version` and `extraOrigins` across the code rg -A 3 -B 3 "version" rg -A 3 -B 3 "extraOrigins"Length of output: 87872
Action: Verify Impact on Version Handling and Cross-Origin Logic
The grep search confirms that the
"version"property is referenced extensively across the codebase (e.g., in web/types/server.ts, GraphQL schemas, and update/changelog logic), indicating that clearing this value in api/dev/Unraid.net/myservers.cfg could disrupt components dependent on it. Although"extraOrigins"wasn’t prominently found in the search results, please double-check that there isn’t any hidden or future usage that might affect cross-origin requests.
- Impact on version handling: Ensure that resetting
versionto an empty string won’t break downstream modules that rely on a valid version for update checks, changelog generation, or API responses.- Cross-origin configuration: Confirm that clearing
extraOriginsdoes not remove a required override for cross-origin requests, even if current evidence shows minimal usage.api/src/unraid-api/graph/resolvers/array/array.service.ts (3)
24-27: Consider additional error states inarrayIsRunning.
While this method checks forSTARTED, ensure that uninitialized or error states can’t incorrectly be treated as “running.”
70-85: Add validation for disk uniqueness inaddDiskToArray.
You check the array is stopped before adding a disk, which is good. Still, consider an explicit check to ensure the disk isn’t already assigned or invalid.
132-144: Ensure thorough disk stats reset inclearArrayDiskStatistics.
Clearing stats requires the array to be running. Consider verifying the disk truly has stats to clear and handle partial resets (e.g., if id is invalid).
Sorry, changed the base branch to be correct. |
elibosley
force-pushed
the
feat/array-controls
branch
from
3416667 to
994fb49
Compare
elibosley
force-pushed
the
feat/basic-docker-controls
branch
from
bb24904 to
300cae5
Compare
elibosley
force-pushed
the
feat/basic-docker-controls
branch
from
044e2e6 to
3804367
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (2)
66-87: Good test for startContainer method.The test correctly verifies:
- The container is retrieved with the right ID
- The start method is called
- Updated containers are retrieved with cache disabled
- The result matches expectations
However, verify that the implementation correctly passes { useCache: false } to getDockerContainers.
89-110: Good test for stopContainer method.The test correctly verifies:
- The container is retrieved with the right ID
- The stop method is called
- Updated containers are retrieved with cache disabled
- The result matches expectations
However, verify that the implementation correctly passes { useCache: false } to getDockerContainers.
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (3)
16-19: Consider graceful error handling.If
docker.getContainer(id).start()fails, the raised exception could use more contextual logging or a custom message for debugging. Graceful error handling can help users understand whether the container doesn't exist or Docker is unreachable.
27-29: Mirror the same consideration for stopping containers.Similarly, if stopping the container fails, consider adding a try-catch or logging to pinpoint the reason for the failure.
16-36: Eliminate repeated logic.The
startContainerandstopContainermethods share similar patterns (retrieve container, perform an action, refresh list, find updated container). Factoring the shared steps into a helper method (e.g.,updateContainerState(id, action)) can reduce duplication and simplify future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (10)
api/src/core/modules/docker/get-docker-containers.ts(1 hunks)api/src/graphql/schema/types/docker/docker.graphql(1 hunks)api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- api/src/unraid-api/graph/resolvers/resolvers.module.ts
- api/src/graphql/schema/types/docker/docker.graphql
- api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts
- api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.spec.ts
🧰 Additional context used
🧬 Code Definitions (3)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (2)
api/src/core/modules/docker/get-docker-containers.ts (2)
ContainerListingOptions(13-15)getDockerContainers(21-90)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (2)
docker(18-22)containers(30-32)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts (1)
Resolver(8-31)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
docker(18-22)
🪛 GitHub Actions: CI - Main (API)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
[error] 31-31: Type 'false' has no properties in common with type 'ContainerListingOptions'.
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
[error] 63-63: AssertionError: expected "spy" to be called with arguments: [ { useCache: false } ]. Received: [ { useCache: undefined } ]
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
[failure] 63-63: src/unraid-api/graph/resolvers/docker/docker.service.spec.ts > DockerService > should get containers
AssertionError: expected "spy" to be called with arguments: [ { useCache: false } ]
Received:
1st spy call:
[
{
-
"useCache": false,
-
},
"useCache": undefined,
]
Number of calls: 1
❯ src/unraid-api/graph/resolvers/docker/docker.service.spec.ts:63:37
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (18)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (3)
6-6: Good addition of DockerService import.The import of DockerService is correctly added to support the new dependency injection pattern.
10-11: Constructor properly initializes DockerService dependency.The constructor uses proper dependency injection pattern with the private readonly modifier.
34-39: Good implementation of mutations field resolver.The mutations method properly returns an object with an ID that can be used for GraphQL resolver composition.
api/src/core/modules/docker/get-docker-containers.ts (2)
13-15: Good interface definition for container listing options.The ContainerListingOptions interface properly defines the expected input structure with an optional useCache property.
22-22: Parameter correctly typed with new interface.The function parameter is properly updated to use the new ContainerListingOptions interface while maintaining the same default value.
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (7)
4-4: Good import of testing utilities.The imports from vitest are properly organized.
6-9: Good organization of imports.The imports are correctly organized with type imports separated from regular imports.
13-13: Added service mock variable.The dockerService variable is correctly added to access the mocked service in tests.
17-25: Well-structured mock provider for DockerService.The DockerService provider is properly configured with a mock implementation of getContainers.
29-29: Service instance correctly retrieved.The dockerService is properly retrieved from the testing module.
36-39: Good test for docker method.This test effectively verifies that the docker method returns the expected object with ID.
73-76: Good test for mutations method.This test effectively verifies that the mutations method returns the expected object with ID.
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (4)
12-21: Good mock setup for Docker client and functions.The mocks for Docker client and getDockerContainers are set up correctly.
27-39: Proper setup of test environment.The test module is correctly configured, and the mockContainer with necessary methods is properly set up.
112-124: Good error handling tests.The tests properly verify that appropriate errors are thrown when containers aren't found after starting or stopping operations.
45-64:❓ Verification inconclusive
Fix parameter structure in getContainers test.
The test fails because it expects getDockerContainers to be called with { useCache: false }, but the implementation is passing undefined.
- const result = await service.getContainers(false); + const result = await service.getContainers({ useCache: false });Also ensure that the DockerService implementation properly handles the parameter:
🏁 Script executed:
#!/bin/bash # Check the implementation of getContainers in DockerService rg -A 5 "getContainers.*useCache" --type typescriptLength of output: 88
Fix parameter structure in getContainers test
- Update the test call in
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsby replacing:- const result = await service.getContainers(false); + const result = await service.getContainers({ useCache: false });- Ensure that the
DockerService.getContainersimplementation correctly handles an object parameter (i.e. reads theuseCachevalue). Note that the automated search for usage of “useCache” in the implementation did not yield results due to the file type flag issue—please manually verify that the service handles the parameter as expected.🧰 Tools
🪛 GitHub Check: Test API
[failure] 63-63: src/unraid-api/graph/resolvers/docker/docker.service.spec.ts > DockerService > should get containers
AssertionError: expected "spy" to be called with arguments: [ { useCache: false } ]Received:
1st spy call:
[
{
"useCache": false,
"useCache": undefined,
]Number of calls: 1
❯ src/unraid-api/graph/resolvers/docker/docker.service.spec.ts:63:37
🪛 GitHub Actions: CI - Main (API)
[error] 63-63: AssertionError: expected "spy" to be called with arguments: [ { useCache: false } ]. Received: [ { useCache: undefined } ]
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (2)
1-9: Well-structured imports.All imports, including the NestJS
Injectable, appear to be properly referenced for Docker-related functionalities.
10-14: Direct passthrough of container listing is clear.The
getContainers({ useCache })method is neatly delegating togetDockerContainers, aligning well with single-responsibility principles.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (3)
23-39: Consider using typed mock instead of 'any'The test setup is good, but consider using a more specific type for the mockContainer instead of 'any' to improve type safety.
- let mockContainer: any; + let mockContainer: { + start: ReturnType<typeof vi.fn>; + stop: ReturnType<typeof vi.fn>; + };
112-124: Error handling tests are valuableThe tests for error scenarios are important to ensure the service fails gracefully when containers aren't found after operations.
Consider adding tests for when the Docker API itself throws errors during container operations.
// Example test for API error handling it('should handle errors when starting a container', async () => { mockContainer.start.mockRejectedValue(new Error('Docker API error')); await expect(service.startContainer('1')).rejects.toThrow('Docker API error'); expect(docker.getContainer).toHaveBeenCalledWith('1'); expect(mockContainer.start).toHaveBeenCalled(); });
1-125: Test coverage is thorough but could be expandedThe test suite covers basic operations and error cases well. Consider adding these scenarios:
- Test behavior with empty container lists
- Test with various container states (creating, running, paused)
- Test parameter validation for invalid inputs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
docker(18-22)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (10)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (5)
36-39: Good test for the docker method.This test properly verifies that the docker method returns an object with the expected ID. Clean and concise.
41-71: Well-structured test for the containers method.This test is comprehensive:
- Creates mock container data with realistic properties
- Properly configures the mocked service response
- Verifies the result matches the mock data
- Correctly checks that the service was called with
{ useCache: false }The test properly uses the updated parameter format as suggested in a previous review.
73-76: Good test for the mutations method.This test correctly verifies that the mutations method returns an object with the expected ID. Simple and effective.
13-13: Good service mocking implementation.The test properly sets up the DockerService as a mock provider and retrieves it for use in tests. This allows for effective isolation of the resolver from its dependencies.
Also applies to: 17-26, 29-29
4-9: Appropriate imports for testing.All necessary testing utilities from Vitest are imported, along with the required types and services. The imports are organized logically.
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (5)
1-21: Imports and mocks are well structuredThe imports are properly organized and the necessary Docker client mocks are correctly set up. This approach isolates the service for proper unit testing.
41-43: Basic existence check is good practiceThis simple test ensures the service is properly instantiated through DI.
45-64: Container retrieval test is comprehensiveThe test properly verifies that getContainers passes through the correct parameters and returns the expected data.
66-87: Start container functionality well testedThe test verifies all aspects of container starting: correct ID is used, start method is called, and updated container data is returned.
89-110: Stop container functionality thoroughly testedThe test properly verifies all aspects of container stopping with appropriate assertions.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.5.0](v4.4.1...v4.5.0) (2025-04-02) ### Features * add webgui theme switcher component ([#1304](#1304)) ([e2d00dc](e2d00dc)) * api plugin system & offline versioned dependency vendoring ([#1252](#1252)) ([9f492bf](9f492bf)) * **api:** add `unraid-api --delete` command ([#1289](#1289)) ([2f09445](2f09445)) * basic array controls ([#1291](#1291)) ([61fe696](61fe696)) * basic docker controls ([#1292](#1292)) ([12eddf8](12eddf8)) * copy to webgui repo script docs + wc build options ([#1285](#1285)) ([e54f189](e54f189)) ### Bug Fixes * additional url fixes ([4b2763c](4b2763c)) * **api:** redirect benign pnpm postinstall warning to log file ([#1290](#1290)) ([7fb7849](7fb7849)) * **deps:** update dependency chalk to v5 ([#1296](#1296)) ([6bed638](6bed638)) * **deps:** update dependency diff to v7 ([#1297](#1297)) ([3c6683c](3c6683c)) * disable all config watchers ([#1306](#1306)) ([5c1b435](5c1b435)) * extract callbacks to library ([#1280](#1280)) ([2266139](2266139)) * OEM plugin issues ([#1288](#1288)) ([d5a3d0d](d5a3d0d)) * replace files lost during pruning ([d0d2ff6](d0d2ff6)) --- 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