feat: add requirements:health task for service health checks#20
feat: add requirements:health task for service health checks#20konradmichalik merged 7 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a new health-check task to the deployer requirements module, plus configuration keys and a helper to detect active services; the task checks PHP-FPM, webserver, database server, and an HTTP endpoint and renders a requirements table. Changes
Sequence DiagramsequenceDiagram
participant Task as Health Check Task
participant Sys as System (systemctl/pgrep)
participant DBAdmin as DB Admin Tool (mysqladmin/mariadb-admin)
participant Curl as HTTP Client (curl)
participant Table as Requirements Table
Task->>Sys: detect phpX.Y-fpm (systemctl is-active / pgrep)
Sys-->>Task: php-fpm status/result
Task->>Sys: detect webserver (nginx/apache2/httpd)
Sys-->>Task: webserver status/result
Task->>DBAdmin: ping DB admin tool (if available)
DBAdmin-->>Task: ping success/fail
Task->>Sys: fallback check mysqld/mariadbd process (pgrep)
Sys-->>Task: db process status/result
Task->>Curl: request requirements_health_url
Curl-->>Task: http response / error
Task->>Table: render collected results
Table-->>Task: display health status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
deployer/requirements/config/set.php (1)
111-113:requirements_check_health_enabledis misplaced relative to the otherrequirements_check_*_enabledflagsAll other per-category enable flags are grouped together at lines 11–20.
requirements_check_health_enabledis placed far down the file next to the URL config. Keeping it with the other flags improves discoverability and consistency.♻️ Proposed fix
set('requirements_check_database_grants_enabled', true); +set('requirements_check_health_enabled', true); // Locales// Health check -set('requirements_check_health_enabled', true); set('requirements_health_url', 'http://localhost');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployer/requirements/config/set.php` around lines 111 - 113, Move the set('requirements_check_health_enabled', true) so it sits with the other per-category enable flags (the group of set('requirements_check_*_enabled') entries) instead of next to the URL; leave set('requirements_health_url', 'http://localhost') in its current location. Edit the file to relocate the requirements_check_health_enabled flag into the enable-flags block (alongside the other requirements_check_*_enabled settings) to improve consistency and discoverability, ensuring no other keys or order-dependent logic are changed.deployer/requirements/task/health.php (1)
71-72: HTTP 4xx responses classified asREQUIREMENT_OK— consider adding a clarifying comment
$httpCode >= 200 && $httpCode < 500marks all 2xx, 3xx, and 4xx responses as healthy. For a "is the server responding at all" check this is reasonable, but a 404 from the configured URL could mask a misconfiguredrequirements_health_url. A short inline comment documenting the intent would prevent future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployer/requirements/task/health.php` around lines 71 - 72, The HTTP status check in health.php currently treats any $httpCode >= 200 && $httpCode < 500 (2xx–4xx) as REQUIREMENT_OK which can hide misconfigured requirements_health_url; update the code around the conditional that calls addRequirementRow('HTTP response', REQUIREMENT_OK, "HTTP $httpCode from $url") to include a short inline comment explaining the intent (e.g., that 4xx responses mean the server responded but the resource may be missing and therefore are considered "server reachable" for this health check) so future maintainers understand why 4xx are acceptable here; mention the relevant symbols $httpCode, $url, addRequirementRow and REQUIREMENT_OK in the comment.deployer/requirements/functions.php (1)
362-374:pgrepis never tried whensystemctlis inPATHbut non-functional (e.g., containers)The
elseifmeans pgrep is only used when systemctl is entirely absent. If the target host has thesystemctlbinary inPATH(e.g., an Ubuntu/Debian image or VM where systemd isn't PID 1),$hasSystemctlistrue,systemctl is-activereturns empty / "failed" output (stderr is discarded),$status !== 'active', and the function iterates through all names and returnsnull— pgrep is never attempted. The docblock says "pgrep fallback" but the implementation provides it only at the system level, not the per-service level.Consider falling through to pgrep whenever systemctl doesn't confirm the service is active:
♻️ Proposed fix
if ($hasSystemctl) { $status = trim(run("systemctl is-active $name 2>/dev/null || true")); if ($status === 'active') { return $name; } - } elseif (test("pgrep -x $name > /dev/null 2>&1")) { - return $name; - } + } + + // Fallback: pgrep when systemctl is absent, non-functional, or the + // service isn't registered as a systemd unit but the process is running. + if (!$hasSystemctl || $status === 'unknown' || $status === '') { + if (test("pgrep -x $name > /dev/null 2>&1")) { + return $name; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployer/requirements/functions.php` around lines 362 - 374, The code uses elseif so pgrep is only attempted when systemctl is completely absent; change the flow to try pgrep per-service whenever systemctl exists but does not report the service as active. Concretely, in the loop that sets $status via run("systemctl is-active $name...") (and uses $hasSystemctl), replace the elseif branch with a separate if that runs test("pgrep -x $name > /dev/null 2>&1") when either $hasSystemctl is false or when $hasSystemctl is true but $status !== 'active' (i.e., fall through to pgrep if systemctl didn't confirm active). Ensure you reference the same variables ($hasSystemctl, $status, $name) and commands (systemctl is-active, pgrep -x) so the per-service pgrep fallback executes correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployer/requirements/task/health.php`:
- Around line 45-51: The DB ping currently always succeeds because the shell
string passed to run() includes "|| true" and redirects stderr with "2>&1";
remove the "|| true" so run("$adminCmd ping --silent 2>/dev/null") will return
non-zero and trigger the RunException catch when the admin tool is missing or
the DB is unreachable, and change the redirect to "2>/dev/null" (not "2>&1")
since output is ignored; update the call using the $adminCmd variable and leave
the catch(RunException) and $dbChecked logic intact so
addRequirementRow('Database server', ...) is only set on a genuine zero exit.
- Around line 17-19: Validate and sanitize the result of run('php -r "echo
PHP_MAJOR_VERSION.\'.\'.PHP_MINOR_VERSION;"') before using it in the service
name: after assigning $phpVersion, strip any non-digit-and-dot characters and
ensure it matches the X.Y pattern (e.g. using a regex like '/^\d+\.\d+$/'), and
if it fails validation fall back to the generic service identifier used by
isServiceActive (e.g. 'php-fpm') so that isServiceActive("php$phpVersion-fpm",
'php-fpm') is only called with a well-formed version string or the safe
fallback; keep references to $phpVersion, run(...) and isServiceActive(...) to
locate the change.
---
Nitpick comments:
In `@deployer/requirements/config/set.php`:
- Around line 111-113: Move the set('requirements_check_health_enabled', true)
so it sits with the other per-category enable flags (the group of
set('requirements_check_*_enabled') entries) instead of next to the URL; leave
set('requirements_health_url', 'http://localhost') in its current location. Edit
the file to relocate the requirements_check_health_enabled flag into the
enable-flags block (alongside the other requirements_check_*_enabled settings)
to improve consistency and discoverability, ensuring no other keys or
order-dependent logic are changed.
In `@deployer/requirements/functions.php`:
- Around line 362-374: The code uses elseif so pgrep is only attempted when
systemctl is completely absent; change the flow to try pgrep per-service
whenever systemctl exists but does not report the service as active. Concretely,
in the loop that sets $status via run("systemctl is-active $name...") (and uses
$hasSystemctl), replace the elseif branch with a separate if that runs
test("pgrep -x $name > /dev/null 2>&1") when either $hasSystemctl is false or
when $hasSystemctl is true but $status !== 'active' (i.e., fall through to pgrep
if systemctl didn't confirm active). Ensure you reference the same variables
($hasSystemctl, $status, $name) and commands (systemctl is-active, pgrep -x) so
the per-service pgrep fallback executes correctly.
In `@deployer/requirements/task/health.php`:
- Around line 71-72: The HTTP status check in health.php currently treats any
$httpCode >= 200 && $httpCode < 500 (2xx–4xx) as REQUIREMENT_OK which can hide
misconfigured requirements_health_url; update the code around the conditional
that calls addRequirementRow('HTTP response', REQUIREMENT_OK, "HTTP $httpCode
from $url") to include a short inline comment explaining the intent (e.g., that
4xx responses mean the server responded but the resource may be missing and
therefore are considered "server reachable" for this health check) so future
maintainers understand why 4xx are acceptable here; mention the relevant symbols
$httpCode, $url, addRequirementRow and REQUIREMENT_OK in the comment.
|
@coderabbitai Addressing the nitpick comments from the review: set.php — enable flag placement: Fixed. Moved health.php:71-72 — HTTP 4xx comment: Skipped. The check name "HTTP response" (not "HTTP success") makes the intent clear — this is a reachability check, not a status validation. functions.php — pgrep fallback in containers: Fixed. Changed |
|
Tip For best results, initiate chat on the files or code changes.
Fixes confirmed:
Skipped item:
The changes look good! 🐰✨ |
1 participant
Summary
requirements:healthDeployer task that checks the availability of critical services on the target hostChanges
deployer/requirements/task/health.php- New task checking four service categories:deployer/requirements/functions.php- AddisServiceActive()helper (systemctl with pgrep fallback)deployer/requirements/config/set.php- Addrequirements_check_health_enabledandrequirements_health_urlconfig optionsautoload.php- Register the new task fileSummary by CodeRabbit
New Features
Chores
Documentation