fix: move docker mutations to the mutations resolver #1333
Conversation
WalkthroughThis PR refactors the Docker GraphQL mutations by renaming methods and restructuring the schema. The Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (9)
🪧 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 (
|
elibosley
changed the title
![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 (3)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (3)
154-154: Fix formatting - remove extra line break.The static analysis tools have flagged an extra line break that should be removed to comply with the project's style guidelines.
name: container.Status, - },🧰 Tools
🪛 GitHub Check: Test API
[failure] 154-154:
Delete⏎🪛 GitHub Check: Build API
[failure] 154-154:
Delete⏎
160-160: Improve method parameter formatting.The method parameters should be formatted according to the project's style guidelines, with each parameter on a new line for better readability.
- public async getContainers({ useCache = false, all = true, size = true, ...listOptions }: Partial<ContainerListingOptions> = { useCache: false }): Promise<DockerContainer[]> { + public async getContainers( + { + useCache = false, + all = true, + size = true, + ...listOptions + }: Partial<ContainerListingOptions> = { useCache: false } + ): Promise<DockerContainer[]> {🧰 Tools
🪛 GitHub Check: Test API
[failure] 160-160:
Replace{·useCache·=·false,·all·=·true,·size·=·true,·...listOptions·}:·Partial<ContainerListingOptions>·=·{·useCache:·false·}with⏎········{⏎············useCache·=·false,⏎············all·=·true,⏎············size·=·true,⏎············...listOptions⏎········}:·Partial<ContainerListingOptions>·=·{·useCache:·false·}⏎····🪛 GitHub Check: Build API
[failure] 160-160:
Replace{·useCache·=·false,·all·=·true,·size·=·true,·...listOptions·}:·Partial<ContainerListingOptions>·=·{·useCache:·false·}with⏎········{⏎············useCache·=·false,⏎············all·=·true,⏎············size·=·true,⏎············...listOptions⏎········}:·Partial<ContainerListingOptions>·=·{·useCache:·false·}⏎····
222-222: Fix spacing in template expression.Add a space around the
+operator in the template string for consistent formatting.- this.logger.debug(`Container ${id} state after stop attempt ${i+1}: ${updatedContainer?.state}`); + this.logger.debug(`Container ${id} state after stop attempt ${i + 1}: ${updatedContainer?.state}`);🧰 Tools
🪛 GitHub Check: Test API
[failure] 222-222:
ReplaceContainer·${id}·state·after·stop·attempt·${i+1}:·${updatedContainer?.state}with⏎················Container·${id}·state·after·stop·attempt·${i·+·1}:·${updatedContainer?.state}⏎············🪛 GitHub Check: Build API
[failure] 222-222:
ReplaceContainer·${id}·state·after·stop·attempt·${i+1}:·${updatedContainer?.state}with⏎················Container·${id}·state·after·stop·attempt·${i·+·1}:·${updatedContainer?.state}⏎············
📜 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.ts(5 hunks)
🧰 Additional context used
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/docker/docker.service.ts
[failure] 154-154:
Delete ⏎
[failure] 160-160:
Replace {·useCache·=·false,·all·=·true,·size·=·true,·...listOptions·}:·Partial<ContainerListingOptions>·=·{·useCache:·false·} with ⏎········{⏎············useCache·=·false,⏎············all·=·true,⏎············size·=·true,⏎············...listOptions⏎········}:·Partial<ContainerListingOptions>·=·{·useCache:·false·}⏎····
[failure] 222-222:
Replace Container·${id}·state·after·stop·attempt·${i+1}:·${updatedContainer?.state} with ⏎················Container·${id}·state·after·stop·attempt·${i·+·1}:·${updatedContainer?.state}⏎············
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/docker/docker.service.ts
[failure] 154-154:
Delete ⏎
[failure] 160-160:
Replace {·useCache·=·false,·all·=·true,·size·=·true,·...listOptions·}:·Partial<ContainerListingOptions>·=·{·useCache:·false·} with ⏎········{⏎············useCache·=·false,⏎············all·=·true,⏎············size·=·true,⏎············...listOptions⏎········}:·Partial<ContainerListingOptions>·=·{·useCache:·false·}⏎····
[failure] 222-222:
Replace Container·${id}·state·after·stop·attempt·${i+1}:·${updatedContainer?.state} with ⏎················Container·${id}·state·after·stop·attempt·${i·+·1}:·${updatedContainer?.state}⏎············
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (5)
14-14: New import for enhanced functionality.The addition of the
sleeputility is necessary for the improved retry mechanism in thestopmethod.
129-158: Good refactoring of container transformation logic.Extracting the container transformation logic into a dedicated method improves code maintainability and reusability. This transformation can now be used in multiple places without duplicating the logic.
🧰 Tools
🪛 GitHub Check: Test API
[failure] 154-154:
Delete⏎🪛 GitHub Check: Build API
[failure] 154-154:
Delete⏎
175-179: Good use of the new transformContainer method.The implementation now uses the extracted
transformContainermethod, which simplifies this code segment and improves maintainability.
201-210: Method rename aligns with Docker terminology.Renaming
startContainertostartprovides better consistency with Docker's native terminology and makes the API more intuitive.
212-226: Excellent enhancement to container stop operation.The improvements to the
stopmethod are very good:
- Adding a 10-second timeout for graceful container shutdown
- Implementing a retry mechanism to verify the container has actually stopped
- Adding debug logging to help troubleshoot stop operations
- Using early return for efficiency when the container has exited
These changes will make container stopping more reliable and easier to debug.
🧰 Tools
🪛 GitHub Check: Test API
[failure] 222-222:
ReplaceContainer·${id}·state·after·stop·attempt·${i+1}:·${updatedContainer?.state}with⏎················Container·${id}·state·after·stop·attempt·${i·+·1}:·${updatedContainer?.state}⏎············🪛 GitHub Check: Build API
[failure] 222-222:
ReplaceContainer·${id}·state·after·stop·attempt·${i+1}:·${updatedContainer?.state}with⏎················Container·${id}·state·after·stop·attempt·${i·+·1}:·${updatedContainer?.state}⏎············
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
| await container.stop({ t: 10 }); | ||
| let containers = await this.getContainers({ useCache: false }); | ||
| let updatedContainer: DockerContainer | undefined; | ||
| for (let i = 0; i < 5; i++) { |
There was a problem hiding this comment.
Wondering if we should have messaging here if the attempts exceed the 5 attempts.
🤖 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
Thanks to @s3ppo on Discord for flagging this issue
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests