chore: move to independent docker events #1334
Conversation
WalkthroughThis pull request updates several Docker-related components. It upgrades the Changes
Possibly related PRs
Suggested labels
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
🔭 Outside diff range comments (1)
api/package.json (1)
51-136: 💡 Verification agent🧩 Analysis chain
Confirm docker-event-emitter removal
According to the PR objectives and AI summary, the docker-event-emitter dependency has been removed. This aligns with the new
DockerEventServiceimplementation.
🏁 Script executed:
#!/bin/bash # Verify that all docker-event-emitter usage has been removed or replaced echo "Searching for any remaining docker-event-emitter references..." rg "docker-event-emitter" --type tsLength of output: 370
Action Required: Update Test Mocks for DockerEventEmitter
The removal from production code is confirmed in the dependencies list. However, the test file still mocks "docker-event-emitter":
- File:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
- Remove or update the mock to reflect the changes from the new
DockerEventServiceimplementation.
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts (1)
102-124: Consider using more deterministic synchronization instead of time delays.Relying on
setTimeout(viaawait new Promise((resolve) => setTimeout(resolve, ...))) to process stream data can potentially introduce test flakiness. A more robust approach is to wait on an event or a promise that confirms the event has been handled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
api/package.json(1 hunks)api/src/unraid-api/app/app.module.ts(0 hunks)api/src/unraid-api/graph/graph.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(2 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)
💤 Files with no reviewable changes (1)
- api/src/unraid-api/app/app.module.ts
🧰 Additional context used
🧬 Code Definitions (2)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
Injectable(22-167)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
api/src/store/index.ts (1)
getters(38-49)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (13)
api/src/unraid-api/graph/resolvers/docker/docker.module.ts (1)
1-20: Well-structured Docker module following NestJS conventionsThis new module provides a clean encapsulation of all Docker-related functionality by gathering the services and resolvers into a single module. The organization with commented sections for services and resolvers improves readability.
The exports of
DockerServiceandDockerEventServicemake these components available to other modules that importDockerModule.api/package.json (1)
90-90:❓ Verification inconclusive
Verify compatibility with dockerode v4 upgrade
The dockerode package has been updated from v3 to v4, which is a major version update that may include breaking changes. The PR objective confirms this was intentional.
🏁 Script executed:
#!/bin/bash # Check for any breaking changes that might impact existing code # Search for any dockerode usage in the codebase echo "Searching for dockerode usage in the codebase..." rg -A 3 "dockerode" --type ts # Check the changelog or release notes for dockerode v4 echo "Checking dockerode v4 release notes..." curl -s "https://api.github.com/repos/apocas/dockerode/releases/tags/v4.0.0" | jq '.body'Length of output: 2742
Dockerode v4 Upgrade: Preliminary Verification Complete
- Dockerode usage is present in several files (e.g.,
docker.service.ts,docker.service.spec.ts,docker-event.service.ts,docker-event.service.spec.ts,clients/docker.ts), with no usage patterns immediately indicating incompatibility.- The fetched release notes returned only the version string ("v4.0.0") without additional breaking change details.
- While the intentional upgrade appears sound, please ensure that comprehensive integration tests covering Docker interactions are executed to catch any subtle issues.
api/src/unraid-api/graph/graph.module.ts (1)
32-33:❓ Verification inconclusive
Auth-related dependencies removed from GraphModule
The AuthZService injection and parameter have been removed, simplifying this module's dependencies. This appears to be part of the refactoring for Docker events handling.
🏁 Script executed:
#!/bin/bash # Verify if auth functionality has been moved elsewhere # Check for AuthZService usage in Docker-related files echo "Checking if AuthZService is used in Docker services:" rg "AuthZService" --type ts api/src/unraid-api/graph/resolvers/docker/Length of output: 185
GraphModule Dependency Refactoring – Please Verify Global Auth References
The removal of the AuthZService injection and parameter from
api/src/unraid-api/graph/graph.module.ts(lines 32–33) appears correct for the Docker events refactoring. Our initial search in the Docker resolvers folder found no usage of AuthZService. However, since the check was limited to a specific directory, please verify—ideally via a global repository search—that no residual usage of AuthZService remains before finalizing this change.api/src/unraid-api/graph/resolvers/resolvers.module.ts (2)
16-16: Added DockerModule importThe import for DockerModule has been added, which will provide access to the encapsulated Docker functionality.
37-37:❓ Verification inconclusive
Docker functionality now imported from DockerModule
The module now imports DockerModule rather than directly including Docker-related providers. This improves modularity and encapsulation.
🏁 Script executed:
#!/bin/bash # Verify that Docker providers were removed from the providers array # Get the old version of the file to confirm the removed providers echo "Checking the previous version of resolvers.module.ts for removed Docker providers:" git show HEAD~1:api/src/unraid-api/graph/resolvers/resolvers.module.ts | grep -E "Docker(Service|Resolver|MutationsResolver)"Length of output: 302
DockerModule Usage Update – Please Verify Removal of Legacy Docker Providers
The module now properly imports
DockerModule, which is intended to replace directly included Docker providers for better modularity. However, our automated check did not yield evidence of Docker-related providers (e.g., DockerService, DockerResolver, DockerMutationsResolver) in the previous commit. Please manually verify that no legacy Docker providers remain in theprovidersarray.api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (1)
12-56: All tests look well-structured and comprehensive.They correctly verify the module's successful compilation and ensure that each relevant service/resolver is provided. This coverage helps confirm that
DockerModuleis properly configured and exports the necessary providers.api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts (2)
53-96: Solid use of setup and teardown in this suite.Clearing mocks and calling
service.stopEventStream()inafterEachensures that tests start with a clean state. This approach minimizes the risk of cross-test contamination. Great job keeping tests atomic.
136-196: Nice handling of multiple events per chunk.Splitting the chunk by newline and parsing each JSON object individually is both straightforward and effective. This ensures multiple events in a single chunk are processed accordingly without missing any.
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
55-75: Debounced container cache updates provide efficient batching.Calling
debouncedContainerCacheUpdate()on module init (line 56) and exposing it as a public method (line 72) can help smooth out frequent updates. Keep in mind that debouncing may delay updates if frequent container actions occur in rapid succession, which likely matches the intended behavior. This approach looks well-thought-out.api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (3)
63-85: Good use of chokidar for Docker socket detection.Watching the
var-rundirectory and reacting to the appearance/disappearance of the socket is a neat approach. This ensures the event stream is opened only when Docker is available, avoiding unnecessary errors or manual checks.
99-114: Well-targeted container event filtering.Checking if
actionNamestarts with the watched actions before updating the container cache is clear and readable. This ensures efficient event handling without the overhead of processing irrelevant actions.
116-164: Robust event stream setup and error handling.Using listeners for errors, stream ends, and data parsing makes
setupDockerWatch()resilient to malformed data or stream anomalies. Logging detailed messages helps with debugging if issues arise. This is a strong implementation practice.api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (1)
527-550: Well-documented test migration with clear explanationsThe skipped tests clearly indicate what happened to the functionality they were testing. The comments provide valuable context that:
- These tests have been moved to
docker-event.service.spec.ts- The methods no longer exist in
DockerServiceThis aligns with the PR objective of moving to independent Docker events by relocating the Docker event handling functionality from
DockerServiceto a dedicatedDockerEventService.
- update dockerode to v4 - remove docker-event-emitter dependency
elibosley
force-pushed
the
chore/docker-event-emitter-refactor
branch
from
2500f21 to
5ff652a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (2)
4-5: Consider reusing a shared TestingModule in beforeEach.
While each test individually creates and compiles a TestingModule, you might load the module once in a beforeEach hook and reuse it across your test cases. This can improve test performance and readability by reducing repetitive boilerplate.
48-55: Add negative/edge case tests.
You already confirm that DockerMutationsResolver is provided, but consider adding tests validating scenarios where required dependencies are missing or incorrectly configured. Ensuring the module fails gracefully can boost resilience.api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
74-77: Debounce usage to throttle frequent container updates.
The current code calls a 500ms debounce, which is usually sufficient. Consider verifying that this won't skip any critical container state transitions. If you do need real-time updates, you could reduce or remove the debounce. Otherwise, this is an effective approach to avoid excessive updates.api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (3)
10-22: Ensure DockerEventAction covers full usage scenarios.
You have enumerated multiple container lifecycles. Confirm that all relevant actions for your environment are included. If new Docker actions appear in upcoming versions of Docker, consider an extensibility mechanism or fallback logic for unknown actions.
99-114: Use stronger typing for events.
Currently,handleDockerEventprocesses any JSON object as an event. If you have known event structures, consider using interfaces or type guards to make the event object safer to parse, reducing the chances of runtime errors.
116-169: Add reconnection strategies for event streaming.
Should the stream disconnect unexpectedly, you may want logic that automatically attempts to reconnect the event stream. Right now, you stop everything onendanderror. A reconnection approach can improve reliability for handling Docker restarts or transient socket unavailability.
📜 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 (10)
api/package.json(1 hunks)api/src/unraid-api/app/app.module.ts(0 hunks)api/src/unraid-api/graph/graph.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(0 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(2 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)
💤 Files with no reviewable changes (2)
- api/src/unraid-api/app/app.module.ts
- api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- api/src/unraid-api/graph/resolvers/docker/docker.module.ts
- api/package.json
- api/src/unraid-api/graph/graph.module.ts
- api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts
- api/src/unraid-api/graph/resolvers/resolvers.module.ts
🧰 Additional context used
🧬 Code Definitions (2)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
Injectable(23-190)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
api/src/store/index.ts (1)
getters(38-49)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (1)
12-19: Great coverage for module compilation.
Verifying that DockerModule compiles ensures that all necessary providers and imports are in place. This test is straightforward and is good to have in regression coverage.api/src/unraid-api/graph/resolvers/docker/docker.service.ts (3)
34-38: Review exposure of Docker client as public.
ChanginggetDockerClient()from private to public expands the Docker client usage beyond this service. This can be helpful, but also be mindful of potential misuse or concurrency concerns by external callers. Validate that external consumers handle the Docker client responsibly.
58-59: Good call to debounce container cache update on module init.
You are now triggeringdebouncedContainerCacheUpdateat initialization, ensuring the container cache is populated upfront without spawning watchers. This appears consistent with your new approach.
61-66: Docstring for getAutoStarts is helpful.
Explicitly citing the GitHub issue clarifies the dependency on an external file that may or may not exist. This is a neat reference for debugging or further enhancements.
| async onModuleInit() { | ||
| this.setupVarRunWatch(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Possible fallback if docker-socket is already present at startup.
At module init, you watch for the socket file to appear, but if the socket already exists when the app starts, you may miss the event. Consider checking for the file’s presence immediately before setting up the watch, ensuring you don’t skip any events.
There was a problem hiding this comment.
The ignoreInitial: false in here should cause this to trigger on app startup (it does on my dev server)
There was a problem hiding this comment.
🧩 Analysis chain
@elibosley You're absolutely right! I see the ignoreInitial: false option in your setupVarRunWatch method:
private setupVarRunWatch() {
this.logger.log('Setting up var run watch');
const watcher = watch('/var/run/docker.sock', {
ignoreInitial: false,
});
// ...
}This setting will indeed trigger the watcher for existing files when the app starts up, which addresses the concern about the Docker socket already being present at startup. Thanks for the clarification!
🏁 Script executed:
#!/bin/bash
# Find the complete implementation of DockerEventService, particularly looking for setupVarRunWatch
cat api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts
Length of output: 6286
🏁 Script executed:
#!/bin/bash
# Search for the setupVarRunWatch method implementation in DockerEventService
grep -A 20 "setupVarRunWatch" api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts
Length of output: 883
@elibosley You're absolutely right! I see in your implementation that you're using ignoreInitial: false in the chokidar watch options:
private setupVarRunWatch() {
const paths = getters.paths();
watch(paths['var-run'], { ignoreInitial: false })
.on('add', async (path) => {
if (path === paths['docker-socket']) {
this.logger.debug('Starting docker event watch');
await this.setupDockerWatch();
}
})
// ...
}This configuration ensures that the watcher will emit events for existing files when it starts up, addressing the scenario where the Docker socket already exists at application startup. Thanks for clarifying!
✏️ Learnings added
Learnt from: elibosley
PR: unraid/api#1334
File: api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts:63-66
Timestamp: 2025-04-07T14:34:47.145Z
Learning: In DockerEventService, the chokidar file watcher is configured with `ignoreInitial: false` to ensure that existing files (like the Docker socket) are detected and processed at application startup, not just when files change after the watcher is initialized.
Learnt from: elibosley
PR: unraid/api#1334
File: api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts:63-66
Timestamp: 2025-04-07T14:34:47.145Z
Learning: In DockerEventService, the chokidar file watcher is configured with `ignoreInitial: false` to ensure that existing files (like the Docker socket) are detected and processed at application startup, not just when files change after the watcher is initialized.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (4)
3-4: Consolidate vitest importsMultiple imports from the same module can be combined into a single import statement for better readability.
-import { describe, expect, it } from 'vitest'; -import { vi } from 'vitest'; +import { describe, expect, it, vi } from 'vitest';🧰 Tools
🪛 GitHub Check: Build API
[failure] 3-3:
Replace·}·from·'vitest';⏎import·{with,🪛 GitHub Check: Test API
[failure] 3-3:
Replace·}·from·'vitest';⏎import·{with,
24-40: Add verification for all mocked propertiesYou've mocked
debouncedContainerCacheUpdatebut aren't verifying its presence in your test.const service = module.get<DockerService>(DockerService); expect(service).toBeDefined(); expect(service).toHaveProperty('getDockerClient'); + expect(service).toHaveProperty('debouncedContainerCacheUpdate');
54-64: Fix provider formatting to match style guidelinesStatic analysis tools are flagging formatting issues with the provider array. Adjust the formatting to keep the test consistent with your style guidelines.
const module: TestingModule = await Test.createTestingModule({ providers: [ - DockerResolver, - { provide: DockerService, useValue: {} }, + DockerResolver, { provide: DockerService, useValue: {} }, ], }).compile();🧰 Tools
🪛 GitHub Check: Build API
[failure] 56-56:
Replace⏎················DockerResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············withDockerResolver,·{·provide:·DockerService,·useValue:·{}·}🪛 GitHub Check: Test API
[failure] 56-56:
Replace⏎················DockerResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············withDockerResolver,·{·provide:·DockerService,·useValue:·{}·}
66-76: Fix provider formatting to match style guidelinesStatic analysis tools are flagging formatting issues with the provider array. Adjust the formatting to keep the test consistent with your style guidelines.
const module: TestingModule = await Test.createTestingModule({ providers: [ - DockerMutationsResolver, - { provide: DockerService, useValue: {} }, + DockerMutationsResolver, { provide: DockerService, useValue: {} }, ], }).compile();🧰 Tools
🪛 GitHub Check: Build API
[failure] 68-68:
Replace⏎················DockerMutationsResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············withDockerMutationsResolver,·{·provide:·DockerService,·useValue:·{}·}🪛 GitHub Check: Test API
[failure] 68-68:
Replace⏎················DockerMutationsResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············withDockerMutationsResolver,·{·provide:·DockerService,·useValue:·{}·}
📜 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.module.spec.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
[failure] 3-3:
Replace ·}·from·'vitest';⏎import·{ with ,
[failure] 56-56:
Replace ⏎················DockerResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············ with DockerResolver,·{·provide:·DockerService,·useValue:·{}·}
[failure] 68-68:
Replace ⏎················DockerMutationsResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············ with DockerMutationsResolver,·{·provide:·DockerService,·useValue:·{}·}
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
[failure] 3-3:
Replace ·}·from·'vitest';⏎import·{ with ,
[failure] 56-56:
Replace ⏎················DockerResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············ with DockerResolver,·{·provide:·DockerService,·useValue:·{}·}
[failure] 68-68:
Replace ⏎················DockerMutationsResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············ with DockerMutationsResolver,·{·provide:·DockerService,·useValue:·{}·}
⏰ 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.module.spec.ts (1)
12-77: Strong test coverage for the Docker module structureThe test suite effectively verifies that the DockerModule compiles properly and provides all necessary Docker-related services and resolvers. This is critical when refactoring components to ensure the dependency injection system works as expected.
🧰 Tools
🪛 GitHub Check: Build API
[failure] 56-56:
Replace⏎················DockerResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············withDockerResolver,·{·provide:·DockerService,·useValue:·{}·}
[failure] 68-68:
Replace⏎················DockerMutationsResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············withDockerMutationsResolver,·{·provide:·DockerService,·useValue:·{}·}🪛 GitHub Check: Test API
[failure] 56-56:
Replace⏎················DockerResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············withDockerResolver,·{·provide:·DockerService,·useValue:·{}·}
[failure] 68-68:
Replace⏎················DockerMutationsResolver,⏎················{·provide:·DockerService,·useValue:·{}·},⏎············withDockerMutationsResolver,·{·provide:·DockerService,·useValue:·{}·}
| } | ||
| } | ||
|
|
||
| private async handleDockerEvent(event: any): Promise<void> { |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (1)
126-128: Address Prettier/Linter spacing concerns.Static analysis indicates a required formatting fix here. Consider adding a line break or adjusting spacing to maintain consistency with lint rules.
Apply this diff to align with the lint suggestions:
- if (dockerEvent.Type === DockerEventType.CONTAINER && - typeof actionName === 'string' && - this.containerActions.includes(actionName as DockerEventAction)) { + if ( + dockerEvent.Type === DockerEventType.CONTAINER && + typeof actionName === 'string' && + this.containerActions.includes(actionName as DockerEventAction) + ) {🧰 Tools
🪛 GitHub Check: Test API
[failure] 126-126:
ReplacedockerEvent.Type·===·DockerEventType.CONTAINER·&&·with⏎················dockerEvent.Type·===·DockerEventType.CONTAINER·&&
[failure] 127-127:
Delete·
[failure] 128-128:
Insert⏎············🪛 GitHub Check: Build API
[failure] 126-126:
ReplacedockerEvent.Type·===·DockerEventType.CONTAINER·&&·with⏎················dockerEvent.Type·===·DockerEventType.CONTAINER·&&
[failure] 127-127:
Delete·
[failure] 128-128:
Insert⏎············
📜 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-event.service.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts
[failure] 126-126:
Replace dockerEvent.Type·===·DockerEventType.CONTAINER·&&· with ⏎················dockerEvent.Type·===·DockerEventType.CONTAINER·&&
[failure] 127-127:
Delete ·
[failure] 128-128:
Insert ⏎············
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts
[failure] 126-126:
Replace dockerEvent.Type·===·DockerEventType.CONTAINER·&&· with ⏎················dockerEvent.Type·===·DockerEventType.CONTAINER·&&
[failure] 127-127:
Delete ·
[failure] 128-128:
Insert ⏎············
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (6)
71-73: Good initialization approach.Invoking
setupVarRunWatchinonModuleInitensures the file watch is set up as soon as the module is initialized, which seems appropriate for automatically handling Docker events.
79-93: Proper use ofignoreInitial: false.Using
ignoreInitial: falsewith chokidar ensures that the Docker socket is recognized on startup, even if it already exists. This addresses potential race conditions where the socket might be present before the watch is established.
98-105: Well-structured stream shutdown.Stopping and cleaning up the Docker event stream gracefully prevents leaks and uncaught event listeners. This is a good practice for ensuring stability.
107-132: Robust event handling.The checks and type guarding around
eventensure unexpected data is logged and handled gracefully. This helps maintain stability and traceability when invalid or malformed data is received.🧰 Tools
🪛 GitHub Check: Test API
[failure] 126-126:
ReplacedockerEvent.Type·===·DockerEventType.CONTAINER·&&·with⏎················dockerEvent.Type·===·DockerEventType.CONTAINER·&&
[failure] 127-127:
Delete·
[failure] 128-128:
Insert⏎············🪛 GitHub Check: Build API
[failure] 126-126:
ReplacedockerEvent.Type·===·DockerEventType.CONTAINER·&&·with⏎················dockerEvent.Type·===·DockerEventType.CONTAINER·&&
[failure] 127-127:
Delete·
[failure] 128-128:
Insert⏎············
134-187: Comprehensive error handling for Docker event stream.The
try/catchblock around the event stream creation, coupled with callbacks that destroy the stream on errors or closures, provides resilient handling for unexpected disconnections or stream failures.
193-195: Useful status check.The
isActivemethod clearly indicates whether the service is actively streaming Docker events, which is helpful for health checks and debugging.
|
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 (4)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (4)
107-111: Consider defining a stronger event type.
You’re using anunknowntype for the Docker event and casting it to aDockerEventinterface. This is acceptable in many cases, but you might minimize runtime checks by strictly typing the event or using runtime validation to ensure correctness.
79-93: Validate repeated “add” events on the same socket.
If the socket is re-created multiple times, you’ll callsetupDockerWatch()for each “add” event. Ensure this scenario is handled gracefully (e.g., by stopping an existing stream before starting another).
140-142: Use a more direct type assertion for the event stream.
Here you’re castingeventStreamtounknownthen toReadable. If possible, checkinstanceof Readableor use a direct cast:- this.dockerEventStream = eventStream as unknown as Readable; + this.dockerEventStream = eventStream as Readable;This can help reduce confusion about intermediate
unknowncasts.
185-189: Consider auto-retry on setup failure.
When Docker is unavailable or an error occurs, you log and stop. Consider an optional backoff or retry mechanism to recover without requiring a file remove/add cycle.
📜 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-event.service.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (2)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
Injectable(23-190)api/src/store/index.ts (1)
getters(38-49)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (2)
117-120: Clarify partial vs. exact action matching.
Currently,shouldProcessusesstartsWith(action)while the container actions check usesincludes(action). If your intent is to catch exact matches only, consider using===for consistency.Also applies to: 127-129
71-73: Overall implementation looks solid!
Your approach for setting upchokidarwatchers, stopping the event stream inonModuleDestroy, and deferring Docker operations toDockerServiceis clean and maintainable. Excellent job!Also applies to: 75-77
Summary by CodeRabbit
New Features
Refactor
Chores