fix(k8s): check daemon reachability by deploy id - #983
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
Merge Risk: 🔵 Low · up to The PR makes Kubernetes daemon reachability checks fail fast by using the deployment ID, restoring availability of the ExApp deployment flow. A bounded follow-up remains: accessibility failures are logged at error level instead of warning level, which may create misleading operational noise. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0124ee00-a9c3-4529-ba9f-c66f598d4185
📒 Files selected for processing (2)
lib/Service/ExAppsPageService.phplib/Settings/Admin.php
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
| if (!$daemonConfigAccessible) { | ||
| $this->logger->error(sprintf('Deploy daemon "%s" is not accessible by Nextcloud: %s', $daemonConfig->getName(), $pingError)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a warning for an unreachable Kubernetes daemon.
Line 52 emits an error-level log for the same reachability state that lib/Service/ExAppsPageService.php records as a warning. The PR objective specifies warning logging for inaccessible daemons. Use warning() here to keep both UI flows consistent.
Proposed fix
- $this->logger->error(sprintf('Deploy daemon "%s" is not accessible by Nextcloud: %s', $daemonConfig->getName(), $pingError));
+ $this->logger->warning(sprintf('Deploy daemon "%s" is not accessible by Nextcloud: %s', $daemonConfig->getName(), $pingError));📝 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.
| if (!$daemonConfigAccessible) { | |
| $this->logger->error(sprintf('Deploy daemon "%s" is not accessible by Nextcloud: %s', $daemonConfig->getName(), $pingError)); | |
| } | |
| if (!$daemonConfigAccessible) { | |
| $this->logger->warning(sprintf('Deploy daemon "%s" is not accessible by Nextcloud: %s', $daemonConfig->getName(), $pingError)); | |
| } |
Four call sites always ran the Docker ping: ExAppsPageService, Settings\Admin, and the two DaemonConfigController endpoints behind the admin UI's connection checks. On a kubernetes-install daemon that probes a Docker Engine path HaRP does not serve, and it blocks until cURL times out rather than failing fast — so a healthy daemon reads as unreachable, the ExApp deploy button stays disabled and "Check Connection" reports failure. Mirrors the dispatch SetupChecks\DaemonCheck already does.
a478c56 to
85fa3b7
Compare
1 participant
ExAppsPageService and Settings\Admin always ran the Docker ping. On a kubernetes-install daemon that probes a Docker Engine path HaRP does not serve, and it blocks until cURL times out rather than failing fast — so a healthy daemon reads as unreachable and the ExApp deploy button stays disabled.
Mirrors the dispatch SetupChecks\DaemonCheck already does.
🤖 AI (if applicable)