feat: add basic docker network listing #1317
Conversation
WalkthroughThe pull request removes several Docker-related functionalities, including the Changes
Possibly related PRs
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: 1
🧹 Nitpick comments (7)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (4)
1-5: Remove unused OnModuleInit import.The class imports
OnModuleInitbut doesn't implement this interface. Remove the unused import.-import { Injectable, OnModuleInit } from '@nestjs/common'; +import { Injectable } from '@nestjs/common';
15-21: Consider making Docker socket path configurable.The Docker client is hardcoded to use
/var/run/docker.sock, which might not work in all environments. Consider making this configurable.+import { ConfigService } from '@nestjs/config'; @Injectable() export class DockerService { private client: Docker; - constructor() { + constructor(private configService: ConfigService) { this.client = new Docker({ - socketPath: '/var/run/docker.sock', + socketPath: this.configService.get('DOCKER_SOCKET_PATH', '/var/run/docker.sock'), }); }
32-40: Fix indentation in getNetworks method.The static analysis tools are flagging indentation issues that need to be fixed to match the project's style guidelines.
public async getNetworks(): Promise<DockerNetwork[]> { return this.client .listNetworks() .catch(catchHandlers.docker) - .then((networks = []) => - networks.map((object) => - camelCaseKeys(object as unknown as Record<string, unknown>, { deep: true }) - ) as DockerNetwork[] - ); + .then((networks = []) => { + return networks.map((object) => + camelCaseKeys(object as unknown as Record<string, unknown>, { deep: true }) + ) as DockerNetwork[]; + }); }🧰 Tools
🪛 GitHub Check: Test API
[failure] 36-36:
Insert⏎················
[failure] 37-37:
Insert····
[failure] 38-38:
Insert····
[failure] 39-39:
Insert····🪛 GitHub Check: Build API
[failure] 36-36:
Insert⏎················
[failure] 37-37:
Insert····
[failure] 38-38:
Insert····
[failure] 39-39:
Insert····
27-31: Address the TODO comments.There's a TODO about filtering, caching, and proper typing for the networks method. Consider creating a tracking issue for these enhancements.
Would you like me to help implement these TODO items, particularly the caching functionality that's already available for containers?
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (3)
20-20: Improve type assertion.The type assertion at line 20 should use a proper type declaration for better type safety.
-} as unknown as Docker; +} as unknown as typeof Docker;
32-38: Fix import ordering and whitespace.The static analysis tools are flagging issues with import ordering and extra whitespace. Adjust the import order and remove the extra whitespace on line 49.
-import type { TestingModule } from '@nestjs/testing'; -import { Test } from '@nestjs/testing'; -import type { DockerContainer } from '@app/graphql/generated/api/types.js'; -import { getDockerContainers } from '@app/core/modules/docker/get-docker-containers.js'; -import { ContainerState } from '@app/graphql/generated/api/types.js'; -import { DockerService } from '@app/unraid-api/graph/resolvers/docker/docker.service.js'; -import Docker from 'dockerode'; +import { Test } from '@nestjs/testing'; +import type { TestingModule } from '@nestjs/testing'; +import Docker from 'dockerode'; + +import type { DockerContainer } from '@app/graphql/generated/api/types.js'; +import { ContainerState } from '@app/graphql/generated/api/types.js'; +import { getDockerContainers } from '@app/core/modules/docker/get-docker-containers.js'; +import { DockerService } from '@app/unraid-api/graph/resolvers/docker/docker.service.js'; service = module.get<DockerService>(DockerService); - + // Reset mock container methodsAlso applies to: 49-49
🧰 Tools
🪛 GitHub Check: Test API
[failure] 32-32:
Deleteimport·type·{·TestingModule·}·from·'@nestjs/testing';⏎import·{·Test·}·from·'@nestjs/testing';⏎import·type·{·DockerContainer·}·from·'@app/graphql/generated/api/types.js';⏎import·{·getDockerContainers·}·from·'@app/core/modules/docker/get-docker-containers.js';⏎import·{·ContainerState·}·from·'@app/graphql/generated/api/types.js';⏎import·{·DockerService·}·from·'@app/unraid-api/graph/resolvers/docker/docker.service.js';⏎import·Docker·from·'dockerode';⏎⏎🪛 GitHub Check: Build API
[failure] 32-32:
Deleteimport·type·{·TestingModule·}·from·'@nestjs/testing';⏎import·{·Test·}·from·'@nestjs/testing';⏎import·type·{·DockerContainer·}·from·'@app/graphql/generated/api/types.js';⏎import·{·getDockerContainers·}·from·'@app/core/modules/docker/get-docker-containers.js';⏎import·{·ContainerState·}·from·'@app/graphql/generated/api/types.js';⏎import·{·DockerService·}·from·'@app/unraid-api/graph/resolvers/docker/docker.service.js';⏎import·Docker·from·'dockerode';⏎⏎
160-160: Fix line break for error message.The error message needs line breaks according to the static analysis tool.
- await expect(service.stopContainer('abc123def456')).rejects.toThrow('Container abc123def456 not found after stopping'); + await expect(service.stopContainer('abc123def456')).rejects.toThrow( + 'Container abc123def456 not found after stopping' + );🧰 Tools
🪛 GitHub Check: Test API
[failure] 160-160:
Replace'Container·abc123def456·not·found·after·stopping'with⏎············'Container·abc123def456·not·found·after·stopping'⏎········🪛 GitHub Check: Build API
[failure] 160-160:
Replace'Container·abc123def456·not·found·after·stopping'with⏎············'Container·abc123def456·not·found·after·stopping'⏎········
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/src/core/modules/docker/get-docker-containers.ts(1 hunks)api/src/core/modules/docker/get-docker-networks.ts(0 hunks)api/src/core/modules/docker/index.ts(0 hunks)api/src/graphql/schema/types/docker/container.graphql(0 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(5 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(2 hunks)
💤 Files with no reviewable changes (3)
- api/src/core/modules/docker/index.ts
- api/src/graphql/schema/types/docker/container.graphql
- api/src/core/modules/docker/get-docker-networks.ts
🧰 Additional context used
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
[failure] 32-32:
Delete import·type·{·TestingModule·}·from·'@nestjs/testing';⏎import·{·Test·}·from·'@nestjs/testing';⏎import·type·{·DockerContainer·}·from·'@app/graphql/generated/api/types.js';⏎import·{·getDockerContainers·}·from·'@app/core/modules/docker/get-docker-containers.js';⏎import·{·ContainerState·}·from·'@app/graphql/generated/api/types.js';⏎import·{·DockerService·}·from·'@app/unraid-api/graph/resolvers/docker/docker.service.js';⏎import·Docker·from·'dockerode';⏎⏎
[failure] 49-49:
Delete ········
[failure] 160-160:
Replace 'Container·abc123def456·not·found·after·stopping' with ⏎············'Container·abc123def456·not·found·after·stopping'⏎········
api/src/unraid-api/graph/resolvers/docker/docker.service.ts
[failure] 36-36:
Insert ⏎················
[failure] 37-37:
Insert ····
[failure] 38-38:
Insert ····
[failure] 39-39:
Insert ····
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
[failure] 32-32:
Delete import·type·{·TestingModule·}·from·'@nestjs/testing';⏎import·{·Test·}·from·'@nestjs/testing';⏎import·type·{·DockerContainer·}·from·'@app/graphql/generated/api/types.js';⏎import·{·getDockerContainers·}·from·'@app/core/modules/docker/get-docker-containers.js';⏎import·{·ContainerState·}·from·'@app/graphql/generated/api/types.js';⏎import·{·DockerService·}·from·'@app/unraid-api/graph/resolvers/docker/docker.service.js';⏎import·Docker·from·'dockerode';⏎⏎
[failure] 49-49:
Delete ········
[failure] 160-160:
Replace 'Container·abc123def456·not·found·after·stopping' with ⏎············'Container·abc123def456·not·found·after·stopping'⏎········
api/src/unraid-api/graph/resolvers/docker/docker.service.ts
[failure] 36-36:
Insert ⏎················
[failure] 37-37:
Insert ····
[failure] 38-38:
Insert ····
[failure] 39-39:
Insert ····
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
api/src/core/modules/docker/get-docker-containers.ts (1)
3-8: LGTM! Good refactoring approach.The updated function signature now allows for providing a custom Docker client, making the function more flexible while maintaining backward compatibility through the default parameter.
Also applies to: 14-17, 23-24
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (1)
3-26: LGTM! Good mocking approach.The mocks for Docker container and instance are well structured, making the tests more realistic and comprehensive.
| @ResolveField() | ||
| public async networks() { | ||
| return this.dockerService.getNetworks({ useCache: false }); | ||
| } |
There was a problem hiding this comment.
Add permission check to the networks method.
The networks() method doesn't have the @UsePermissions decorator that's present on other methods like containers(). This could allow unauthorized access to Docker network information.
Apply this diff to add the appropriate permissions check:
+ @UsePermissions({
+ action: AuthActionVerb.READ,
+ resource: Resource.DOCKER,
+ possession: AuthPossession.ANY,
+ })
@ResolveField()
public async networks() {
return this.dockerService.getNetworks({ useCache: false });
}📝 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.
| @ResolveField() | |
| public async networks() { | |
| return this.dockerService.getNetworks({ useCache: false }); | |
| } | |
| @UsePermissions({ | |
| action: AuthActionVerb.READ, | |
| resource: Resource.DOCKER, | |
| possession: AuthPossession.ANY, | |
| }) | |
| @ResolveField() | |
| public async networks() { | |
| return this.dockerService.getNetworks({ useCache: false }); | |
| } |
elibosley
force-pushed
the
feat/docker-network-listing
branch
from
7dc4a6a to
8176248
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
27-42: Added getNetworks methodThis new method provides functionality to list Docker networks. The error handling and response transformation are appropriate.
The TODO comments correctly identify future improvements for filtering, caching, and proper typing.
Consider adding an optional
useCacheparameter similar to the getContainers method for consistency:-public async getNetworks(): Promise<DockerNetwork[]> { +public async getNetworks({ useCache = false } = {}): Promise<DockerNetwork[]> { + // Add caching logic here if useCache is true return this.client .listNetworks() .catch(catchHandlers.docker) .then( (networks = []) => networks.map((object) => camelCaseKeys(object as unknown as Record<string, unknown>, { deep: true }) ) as DockerNetwork[] ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
api/src/core/modules/docker/get-docker-containers.ts(1 hunks)api/src/core/modules/docker/get-docker-networks.ts(0 hunks)api/src/core/modules/docker/index.ts(0 hunks)api/src/graphql/schema/types/docker/container.graphql(0 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(6 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(2 hunks)
💤 Files with no reviewable changes (3)
- api/src/core/modules/docker/index.ts
- api/src/graphql/schema/types/docker/container.graphql
- api/src/core/modules/docker/get-docker-networks.ts
🚧 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.ts (3)
api/src/graphql/generated/api/types.ts (3)
Docker(558-564)DockerContainer(566-584)DockerNetwork(602-619)api/src/core/modules/docker/get-docker-containers.ts (2)
ContainerListingOptions(14-17)getDockerContainers(23-92)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (2)
networks(35-37)containers(30-32)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (20)
api/src/core/modules/docker/get-docker-containers.ts (5)
3-3: Import for Dockerode type addedGood addition of the Dockerode type import for proper type checking.
8-8: Updated import for default Docker clientThe import has been properly updated to use the default Docker client from the utility module.
23-25: Function signature updated with docker parameterThe function signature has been properly updated to accept the
dockerinstance with a default value ofdefaultDocker. This provides backward compatibility while allowing for dependency injection.
45-51: Docker client usageThe code now correctly uses the provided Docker client to list containers. The error handling with
catchHandlers.dockeris appropriate.
14-17:✅ Verification successful
Interface change: docker property added to ContainerListingOptions
The interface now requires a
dockerproperty of typeDockerode. This is a breaking change since consumers of this interface will need to provide this parameter.
🏁 Script executed:
#!/bin/bash # Check for usages of ContainerListingOptions that might need updating rg "ContainerListingOptions" --type ts -A 3 -B 3Length of output: 2668
Interface Change Verified:
dockerProperty Correctly HandledThe addition of the
dockerproperty toContainerListingOptionsis correctly integrated. The consumer inapi/src/unraid-api/graph/resolvers/docker/docker.service.tsnow passesdocker: this.clientwhen invokinggetDockerContainers, ensuring that the breaking change is fully accommodated.api/src/unraid-api/graph/resolvers/docker/docker.service.ts (6)
1-5: Import updates for Docker integrationAdded appropriate imports for Docker and camelCaseKeys, which are necessary for the new functionality.
6-6: Added DockerNetwork type importGood addition of the DockerNetwork type import for the new networks functionality.
15-21: Added Docker client initializationThe service now correctly initializes its own Docker client in the constructor. This encapsulates the Docker connection details within the service.
23-25: Updated getContainers methodThe method now properly passes the Docker client to the getDockerContainers function.
44-53: Updated startContainer methodThe method now correctly uses the client instance to get and start the container, and passes the client when fetching updated containers.
55-64: Updated stopContainer methodThe method now correctly uses the client instance to get and stop the container, and passes the client when fetching updated containers.
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (9)
4-4: Import Docker typeAdded import for the Docker type, which is used for type annotation in the mock.
12-29: Created Docker mocksWell-structured mocks for Docker container and Docker instance. The mock Docker instance includes all necessary methods and properties that are used in the tests.
31-35: Mock dockerode moduleProperly mocked the dockerode module to return the mock Docker instance.
51-54: Reset mock methodsGood practice to reset mock methods before each test to ensure test isolation.
60-86: Updated container testsThe container ID has been updated to a more realistic value, and the test now correctly expects the Docker client to be passed to the getDockerContainers function.
The expectations are well-structured and verify the correct behavior.
88-116: Updated startContainer testThe test has been updated to use the mock container's start method and verify that the Docker client is passed correctly.
118-146: Updated stopContainer testThe test has been updated to use the mock container's stop method and verify that the Docker client is passed correctly.
148-155: Updated container not found after start testThe error message has been updated to use the new container ID.
157-164: Updated container not found after stop testThe error message has been updated to use the new container ID.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (4)
17-23: Consider implementing a caching mechanism for networks.
You’ve introducedNetworkListingOptionswith auseCacheproperty, but the property is not actually used ingetNetworks. For consistency with container caching, consider implementing a similar in-memory cache for networks.
145-174: Safeguard names array splitting for autoStart assignment.
If a container name does not contain a slash or the array of Names is empty,names.split('/')[1]could cause errors. Consider adding a fallback or check to avoid runtime issues.- autoStart: autoStarts.includes(names.split('/')[1]), + const containerName = names?.split('/')?.[1] ?? ''; + autoStart: autoStarts.includes(containerName),
184-194: Incorporate network caching for efficiency.
Currently, theuseCacheparameter is unused, causing every call to re-fetch networks from Docker. For large deployments, caching might substantially improve performance.
196-216: Consider handling Docker errors on start/stop operations.
Your catch handler covers container listing but not container start/stop calls. If Docker cannot start or stop a container, it might throw an exception. You could wrap these calls with your existingcatchHandlers.dockerfor more graceful error handling.api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (3)
283-362: Add trailing commas to fix pipeline failures.
Your pipeline logs show multiple warnings about missing trailing commas within the network objects’ definitions. Complying with the style rules can prevent build failures.IPAM: { Driver: 'default', Config: [ { Subnet: '172.17.0.0/16', - Gateway: '172.17.0.1' + Gateway: '172.17.0.1', }, ], },🧰 Tools
🪛 GitHub Check: Build API
[failure] 296-296:
Insert,
[failure] 297-297:
Insert,
[failure] 298-298:
Insert,
[failure] 304-304:
Insert,
[failure] 314-314:
Insert,
[failure] 316-316:
Insert,
[failure] 317-317:
Insert,
[failure] 337-337:
Insert,🪛 GitHub Check: Test API
[failure] 296-296:
Insert,
[failure] 297-297:
Insert,
[failure] 298-298:
Insert,
[failure] 304-304:
Insert,
[failure] 314-314:
Insert,
[failure] 316-316:
Insert,
[failure] 317-317:
Insert,
[failure] 337-337:
Insert,
565-586: Use a more specific callback type instead ofFunction.
UsingFunctionas a type can mask potential errors. Explicit callback signatures clarify the code.- let unlinkCallback: Function = () => {}; + let unlinkCallback: (path: string) => void = () => {};🧰 Tools
🪛 Biome (1.9.4)
[error] 565-565: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 586-586: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
665-703: Replace genericFunctiontype with a typed callback.
Similar to the unlink callback, providing a specific signature forcontainerCallbackmakes the tests more robust and self-documenting.- let containerCallback: Function = () => {}; + let containerCallback: (event: { Type: string; Action: string; from: string }) => Promise<void> = () => Promise.resolve();🧰 Tools
🪛 Biome (1.9.4)
[error] 674-674: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (12)
api/src/core/modules/docker/get-docker-containers.ts(0 hunks)api/src/core/modules/docker/index.ts(0 hunks)api/src/core/modules/index.ts(0 hunks)api/src/index.ts(0 hunks)api/src/store/index.ts(0 hunks)api/src/store/modules/docker.ts(0 hunks)api/src/store/store-sync.ts(0 hunks)api/src/store/sync/info-apps-sync.ts(0 hunks)api/src/store/watch/docker-watch.ts(0 hunks)api/src/store/watch/var-run-watch.ts(0 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(2 hunks)
💤 Files with no reviewable changes (10)
- api/src/core/modules/docker/index.ts
- api/src/core/modules/index.ts
- api/src/index.ts
- api/src/store/store-sync.ts
- api/src/store/watch/var-run-watch.ts
- api/src/store/index.ts
- api/src/core/modules/docker/get-docker-containers.ts
- api/src/store/sync/info-apps-sync.ts
- api/src/store/watch/docker-watch.ts
- api/src/store/modules/docker.ts
🧰 Additional context used
🧬 Code Definitions (2)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (3)
api/src/graphql/generated/api/types.ts (4)
Docker(558-564)DockerContainer(566-584)ContainerPort(448-454)DockerNetwork(602-619)api/src/store/index.ts (1)
getters(38-49)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
networks(35-37)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (1)
api/src/graphql/generated/api/types.ts (1)
Docker(558-564)
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
[failure] 77-77:
Delete ·
[failure] 80-80:
Insert ,
[failure] 296-296:
Insert ,
[failure] 297-297:
Insert ,
[failure] 298-298:
Insert ,
[failure] 304-304:
Insert ,
[failure] 314-314:
Insert ,
[failure] 316-316:
Insert ,
[failure] 317-317:
Insert ,
[failure] 337-337:
Insert ,
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
[failure] 77-77:
Delete ·
[failure] 80-80:
Insert ,
[failure] 296-296:
Insert ,
[failure] 297-297:
Insert ,
[failure] 298-298:
Insert ,
[failure] 304-304:
Insert ,
[failure] 314-314:
Insert ,
[failure] 316-316:
Insert ,
[failure] 317-317:
Insert ,
[failure] 337-337:
Insert ,
🪛 Biome (1.9.4)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
[error] 565-565: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 586-586: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 674-674: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
42-58: Watcher logic appears well-structured.
The setup for watching and unwatching the Docker socket is straightforward, and the reset ofdockerWatcherandcontainerCacheupon socket removal looks correct.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (5)
38-40: Consider extending the DockerError interfaceThe DockerError interface extends NodeJS.ErrnoException but only adds an address property. Consider adding other potential Docker-specific error properties that might be needed for complete testing.
373-383: Comprehensive Docker error handling testThe test properly verifies error handling for Docker socket errors. Consider adding additional tests for other types of Docker errors that might occur.
it('should handle other docker errors', async () => { const error = new Error('Unknown Docker error'); mockListNetworks.mockRejectedValue(error); await expect(service.getNetworks({ useCache: false })).rejects.toThrow( 'Docker error: Unknown Docker error' ); expect(mockListNetworks).toHaveBeenCalled(); });
527-554: Watcher setup test could be improvedThe test mocks setupDockerWatch which is actually the method being indirectly tested. Consider refactoring to verify the actual behavior rather than mocking the implementation.
it('should setup docker watcher when docker socket is added', async () => { // Get the watch function from chokidar const { watch } = await import('chokidar'); // Mock the on method to simulate the add event const mockOn = vi.fn().mockImplementation((event, callback) => { if (event === 'add') { // Simulate the add event with the docker socket path callback('/var/run/docker.sock'); } return { on: vi.fn() }; }); // Replace the watch function's on method (watch as any).mockReturnValue({ on: mockOn, }); // Mock DockerEE const DockerEE = (await import('docker-event-emitter')).default; // Call the setupVarRunWatch method await (service as any).setupVarRunWatch(); // Verify that DockerEE was instantiated with the client expect(DockerEE).toHaveBeenCalledWith(mockDockerInstance); });
670-715: Clarify which container events are watchedThe test for non-watched container events doesn't clearly indicate which events are considered "watched" vs "non-watched". Consider adding a comment or renaming the test to be more explicit.
// The watched container events are: 'start', 'stop', 'die', 'destroy' // Non-watched events like 'create' should not trigger a container cache update it('should not call debouncedContainerCacheUpdate for non-watched container events', async () => { // Test implementation... });
717-744: Avoid hard-coded timeouts in debounce testsThe test uses a hard-coded timeout of 600ms which could make the test flaky. Consider exposing the debounce delay as a configurable parameter for testing.
// In the service class private debounceCacheUpdateDelay = 500; // ms private debouncedContainerCacheUpdate = debounce(async () => { // implementation }, this.debounceCacheUpdateDelay); // In the test // Override the debounce delay for testing (service as any).debounceCacheUpdateDelay = 10; // Use a small value for tests service['debouncedContainerCacheUpdate'](); // Wait just slightly longer than the debounce delay await new Promise((resolve) => setTimeout(resolve, 20));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (1)
api/src/graphql/generated/api/types.ts (1)
Docker(558-564)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Deploy Plugin / Build and Deploy Plugin
- GitHub Check: Cloudflare Pages
🔇 Additional comments (10)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (10)
4-4: Adding dockerode import for Docker typesGood addition of the Docker type import from dockerode, which is necessary for the updated mock implementation.
11-37: Well-structured mocks for external dependenciesThe mock implementations for chokidar, docker-event-emitter and pubsub are comprehensive and properly structured. These mocks will help isolate the tests from external dependencies.
42-65: Well-structured Docker mock implementationThe mock container, functions, and Docker instance are well-organized and properly typed. This approach allows for precise control over test behavior.
67-71: Good module mock for dockerodeThe mock implementation for dockerode is clean and returns the previously defined mockDockerInstance, ensuring consistent behavior across tests.
100-104: Good test isolation in beforeEachProperly resetting mock methods before each test ensures test isolation and prevents test interactions.
132-154: Updated container test implementationThe test now properly uses the mockListContainers function and verifies the transformed container data structure.
282-362: Good test coverage for network retrievalThe new tests for getting networks cover the happy path with proper transformation from Docker API response to application model.
364-371: Good test for empty networks listTesting the empty network list case ensures the service handles this scenario correctly.
385-474: Good test coverage for service gettersThe tests for installed and running container counts validate that the service correctly calculates these values from the container cache.
476-524: Proper testing of app update event structureThe test ensures the service generates the correct event structure for app updates based on container counts.
| this.setupVarRunWatch(); | ||
| } | ||
|
|
||
| public async getAutoStarts(): Promise<string[]> { |
There was a problem hiding this comment.
I think we're missing a test for this method unless I missed it.
🤖 I have created a release *beep* *boop* --- ## [4.7.0](v4.6.6...v4.7.0) (2025-04-24) ### Features * add basic docker network listing ([#1317](#1317)) ([c4fdff8](c4fdff8)) * add permission documentation by using a custom decorator ([#1355](#1355)) ([45ecab6](45ecab6)) * basic vm controls ([#1293](#1293)) ([bc3ca92](bc3ca92)) * code first graphql ([#1347](#1347)) ([f5724ab](f5724ab)) ### Bug Fixes * container names always null ([#1335](#1335)) ([8a5b238](8a5b238)) * **deps:** update all non-major dependencies ([#1337](#1337)) ([2345732](2345732)) * hide reboot notice for patch releases ([#1341](#1341)) ([4b57439](4b57439)) * move docker mutations to the mutations resolver ([#1333](#1333)) ([1bbe7d2](1bbe7d2)) * PR build issue ([457d338](457d338)) * remove some unused fields from the report object ([#1342](#1342)) ([cd323ac](cd323ac)) * sso unreliable if API outputs more than raw json ([#1353](#1353)) ([e65775f](e65775f)) * vms now can detect starting of libvirt and start local hypervisor ([#1356](#1356)) ([ad0f4c8](ad0f4c8)) --- 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>
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor