feat: site atomic deployments - #98
Conversation
…releases - Add site:deploy command for orchestrating deployments - Implement site-deploy.sh playbook with git-based releases - Create timestamped releases with shared resources - Add deployment hooks support (1-building, 2-releasing, 3-finishing) - Register command in SymfonyApp - Include PHP version selection and release cleanup
- Add site-link-shared.sh playbook for linking shared files - Enhance site:shared:push to automatically link uploaded files - Add link_shared_resources() helper function in helpers.sh - Support symlinking shared resources to current release
- Add PHP-FPM log viewing for all detected PHP versions - Add site access log viewing for Caddy sites - Enhance service detection to include PHP and sites - Add error highlighting in log output - Update command description to reflect expanded capabilities
…te support - Use DEPLOYER_PHP environment variable in 1-building.sh hook - Add SQLite database setup example in 2-releasing.sh hook - Ensure hooks use correct PHP binary for composer/artisan commands
- Install Composer automatically in install-php.sh playbook - Improve directory permissions in site-add.sh (755 for proper execution) - Ensure deployment infrastructure supports PHP tooling
WalkthroughAdds a SiteDeployCommand and atomic site deployment playbooks/scripts; enhances ServerLogsCommand to detect PHP‑FPM and per‑site file logs with error highlighting; updates SiteSharedPushCommand to link shared files after upload; adds playbook helpers and tweaks install/site scaffolds and hook scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SiteDeployCmd as SiteDeployCommand
participant PlaybooksTrait
participant SiteScript as site-deploy.sh
participant Hooks as Hook Scripts
participant PHP as PHP-FPM
User->>SiteDeployCmd: run site:deploy
SiteDeployCmd->>SiteDeployCmd: select site, validate hooks, resolve branch/php/keep
SiteDeployCmd->>User: request confirmation
User-->>SiteDeployCmd: approve
SiteDeployCmd->>PlaybooksTrait: executePlaybook(site-deploy) with DEPLOYER_*
PlaybooksTrait->>SiteScript: start deployment
SiteScript->>SiteScript: prepare_directories(), detect PHP, clone_or_update_repo(), build_release()
SiteScript->>SiteScript: link_shared_resources(), run_hook(1-building.sh), run_hook(2-releasing.sh)
SiteScript->>SiteScript: activate_release(), run_hook(3-finishing.sh)
SiteScript->>PHP: reload_php_fpm()
SiteScript->>SiteScript: cleanup_releases(), write_output(YAML)
SiteScript-->>PlaybooksTrait: success
PlaybooksTrait-->>SiteDeployCmd: result
SiteDeployCmd->>User: display summary & next steps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
app/Console/Server/ServerLogsCommand.php (2)
123-214: Processed services/php versions/sites wiring looks solid; consider deduping versionsThe extended
getProcessedServices()correctly:
- Filters and normalizes detected services.
- Tracks Docker separately.
- Extracts PHP versions from both scalar and array-shaped entries.
- Derives site names from
sites_config.- Exposes a single
optionsmap plus structuredservices/phpVersions/sites/hasDockerfor downstream use.One minor improvement: if
info['php']['versions']can contain duplicates, you may end up with duplicate"phpX.Y-fpm"entries in$options. A simplearray_unique()on$phpVersionsbefore building options would keep the select list clean and deterministic.For example:
- $phpVersions = []; + $phpVersions = []; if (isset($info['php']) && is_array($info['php']) && isset($info['php']['versions']) && is_array($info['php']['versions'])) { foreach ($info['php']['versions'] as $versionData) { // ... } } + + $phpVersions = array_values(array_unique($phpVersions));
321-337: File-based log retrieval and error highlighting are well factored; minor perf nit onlyThe new pieces work together nicely:
retrieveServiceLogs()andtryTraditionalLogs()now consistently route output throughhighlightErrors(), giving a unified view for both journalctl and file logs.retrieveFileLogs()provides a clear, reusable abstraction for PHP-FPM and site logs, including a helpful “File:” line and graceful “No logs found” messaging.highlightErrors()is simple and effective at flagging likely-problematic lines.One tiny micro-optimisation (non-blocking): in
highlightErrors(), you recomputestrtolower($keyword)on every iteration. Pre-lowering the keywords once would avoid that inner-loop work:- $keywords = [ + $keywords = [ 'error', 'exception', 'fail', 'failed', 'fatal', 'panic', ' 500 ', ' 502 ', ' 503 ', ' 504 ', ]; - - $lines = explode("\n", $content); + $keywords = array_map('strtolower', $keywords); + $lines = explode("\n", $content); @@ - foreach ($keywords as $keyword) { - if (str_contains($lowerLine, strtolower($keyword))) { + foreach ($keywords as $keyword) { + if (str_contains($lowerLine, $keyword)) { $hasError = true; break; } }Not required, just a small efficiency tweak if logs get large.
Also applies to: 344-365, 392-392, 422-463
playbooks/helpers.sh (2)
33-39: run_as_deployer helper is fine; relies on callers to set PRESERVE_ENV_VARSThe
run_as_deployerwrapper correctly routes throughsudo -n -u deployerwhenDEPLOYER_PERMSisroot|sudo, and falls back to direct execution otherwise. It assumesPRESERVE_ENV_VARSis set by the calling playbook, whichsite-deploy.shandsite-link-shared.shnow do, so the preserved env set is under the caller’s control.Nothing to fix here; just keep the contract (“caller must define PRESERVE_ENV_VARS”) in mind for future playbooks.
182-207: link_shared_resources is robust; consider minimal guard on RELEASE_PATH/SHARED_PATHThe shared-resource linker is careful about:
- No-op when
$SHARED_PATHis missing or empty- Per-item rm-then-symlink with
run_cmd/run_as_deployer- Hard-failing via
fail()if removal or linking failsGiven the use of
rm -rf "$release_item", it’s important that all call sites guarantee saneSHARED_PATH/RELEASE_PATHvalues (whichsite-deploy.shandsite-link-shared.shdo). As a belt-and-suspenders safety net, you could add a quick sanity check at the start:[[ -z $SHARED_PATH || -z $RELEASE_PATH ]] && fail "SHARED_PATH and RELEASE_PATH must be set"Not strictly required with the current callers, but reduces risk if this helper gets reused elsewhere.
playbooks/site-link-shared.sh (1)
42-46: PRESERVE_ENV_VARS ShellCheck warning is a false positive in this context
PRESERVE_ENV_VARSis set here and then consumed indirectly byrun_as_deployerfrom the inlinedhelpers.sh(via--preserve-env="$PRESERVE_ENV_VARS"). ShellCheck doesn’t see that use when analyzing this file in isolation, hence SC2034.You don’t need to change behavior, but adding a brief comment can make the intent clearer and quiet future confusion, e.g.:
# Used by run_as_deployer in helpers.sh to preserve these vars under sudo PRESERVE_ENV_VARS="DEPLOYER_SHARED_PATH,DEPLOYER_RELEASE_PATH,DEPLOYER_DISTRO,DEPLOYER_PERMS"playbooks/site-deploy.sh (2)
49-61: Env validation and preserved-variable setup look consistent with helpersThe required
DEPLOYER_*variables are validated early andDEPLOYER_KEEP_RELEASESis sanitized to a positive integer, which matches the PHP-side validation inSiteDeployCommand. ExportingDEPLOYER_RELEASE_PATH,DEPLOYER_SHARED_PATH,DEPLOYER_CURRENT_PATH,DEPLOYER_REPO_PATH, etc. plus thePRESERVE_ENV_VARSlist aligns withrun_as_deployer’s--preserve-envusage.ShellCheck’s SC2034 about
PRESERVE_ENV_VARSbeing unused is again a per-file false positive; it is consumed by the helper function. No behavioral change needed.Also applies to: 73-81
181-237: End-to-end deployment flow is coherent; watch for assumptions about PHP-FPM service presenceThe overall sequence:
clone_or_update_repowith host key managementbuild_releaseviagit archiveinto a timestamped directorylink_shared_resources(from helpers)- Hook chain (
1-building.sh,2-releasing.sh,3-finishing.sh)activate_release+reload_php_fpm+cleanup_releasesis well-structured and matches the atomic layout created by
site-add.sh. The use offail()keeps failures clear and early.One assumption to keep in mind is
reload_php_fpm()unconditionally doing:run_cmd systemctl reload "php${DEPLOYER_PHP_VERSION}-fpm" || fail "Failed to reload PHP-FPM"If a given PHP version is installed without a matching
php*-fpmunit, deployments will hard-fail here. That may be acceptable given yourserver:installflow, but if you ever support CLI-only PHP installs, you might want to downgrade this to a warning (similar to howsite-add.shhandles optional restarts).Also applies to: 249-266, 284-301, 308-311, 341-357, 369-374
app/Console/Site/SiteDeployCommand.php (2)
41-49: Command options cover expected deployment knobs
domain,keep-releases, andyesmap cleanly to the underlying playbook parameters and UX. The description onkeep-releasesmatchesDEFAULT_KEEP_RELEASES = 5; just ensure any future change to the constant is reflected in this help text.
235-364: Multiple single-use helpers vs. guideline about inlining
displayDeploymentSummary(),resolveKeepReleases(),resolvePhpVersion(), andcheckRemoteHooksExist()are all currently single-use private helpers. Per the PHP guidelines you shared (“Eliminate single-use methods: inline if a method is called only once”), you could inline some of this logic back intoexecute()to reduce indirection.Given the size and distinct responsibilities of these blocks, I’d personally keep them extracted for readability/testability, but if you want to strictly follow the guideline,
displayDeploymentSummary()andcheckRemoteHooksExist()are the easiest candidates to inline with minimal impact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/Console/Server/ServerLogsCommand.php(9 hunks)app/Console/Site/SiteDeployCommand.php(1 hunks)app/Console/Site/SiteSharedPushCommand.php(2 hunks)app/SymfonyApp.php(2 hunks)playbooks/helpers.sh(5 hunks)playbooks/install-php.sh(1 hunks)playbooks/site-add.sh(4 hunks)playbooks/site-deploy.sh(1 hunks)playbooks/site-link-shared.sh(1 hunks)scaffolds/hooks/1-building.sh(1 hunks)scaffolds/hooks/2-releasing.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use methods: inline if a method is called only once
Cache computed values: initialize expensive calculations in the constructor
Avoid method call overhead: prefer direct property access when appropriate
Files:
app/Console/Site/SiteSharedPushCommand.phpapp/SymfonyApp.phpapp/Console/Server/ServerLogsCommand.phpapp/Console/Site/SiteDeployCommand.php
🧠 Learnings (12)
📚 Learning: 2025-09-21T08:52:38.782Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/00-main.mdc:0-0
Timestamp: 2025-09-21T08:52:38.782Z
Learning: Applies to **/{composer.json,package.json} : Check composer.json and package.json for installed packages before starting any task
Applied to files:
scaffolds/hooks/1-building.shplaybooks/install-php.sh
📚 Learning: 2025-10-24T19:58:34.899Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/00-main.mdc:0-0
Timestamp: 2025-10-24T19:58:34.899Z
Learning: Applies to {composer.json,package.json} : Check composer.json and package.json for installed packages before any task
Applied to files:
scaffolds/hooks/1-building.shplaybooks/install-php.sh
📚 Learning: 2025-09-21T08:52:38.782Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/00-main.mdc:0-0
Timestamp: 2025-09-21T08:52:38.782Z
Learning: Applies to **/{composer.json,package.json} : Plan implementation using features supported by the major versions specified in composer.json and package.json
Applied to files:
scaffolds/hooks/1-building.sh
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Commands must not duplicate orchestration logic—extract to shared Services
Applied to files:
app/SymfonyApp.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Only Commands perform console input/output operations
Applied to files:
app/SymfonyApp.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Commands are responsible for console styling, error formatting, and user prompts
Applied to files:
app/SymfonyApp.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Commands may only depend on Services (not other Commands)
Applied to files:
app/SymfonyApp.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Commands must not contain business logic—delegate to Services
Applied to files:
app/SymfonyApp.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Commands receive Services via constructor injection
Applied to files:
app/SymfonyApp.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Commands handle user interaction (input/output) and orchestrate Services
Applied to files:
app/SymfonyApp.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/@(Service|Services)/**/*.php : Extract complex orchestration shared by multiple Commands into dedicated Services
Applied to files:
app/SymfonyApp.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Commands should not invoke other commands (no proxy commands)
Applied to files:
app/SymfonyApp.php
🧬 Code graph analysis (6)
playbooks/site-add.sh (1)
playbooks/helpers.sh (1)
run_cmd(19-25)
app/Console/Site/SiteSharedPushCommand.php (4)
app/Traits/PlaybooksTrait.php (2)
executePlaybook(45-187)PlaybooksTrait(26-189)app/Traits/ServersTrait.php (2)
serverInfo(47-71)validateServerDistribution(110-131)app/Console/Server/ServerInstallCommand.php (2)
execute(44-248)AsCommand(17-540)app/Console/Site/SiteSharedPullCommand.php (2)
AsCommand(19-248)execute(48-178)
playbooks/install-php.sh (1)
playbooks/helpers.sh (1)
run_cmd(19-25)
playbooks/site-deploy.sh (2)
app/Services/InventoryService.php (1)
set(54-60)playbooks/helpers.sh (4)
fail(51-54)run_as_deployer(33-39)run_cmd(19-25)link_shared_resources(182-208)
app/Console/Server/ServerLogsCommand.php (3)
app/Services/IOService.php (2)
info(474-477)writeln(463-469)app/Contracts/BaseCommand.php (1)
nay(186-190)app/DTOs/ServerDTO.php (1)
ServerDTO(7-19)
playbooks/site-link-shared.sh (1)
playbooks/helpers.sh (2)
fail(51-54)link_shared_resources(182-208)
🪛 Shellcheck (0.11.0)
playbooks/site-deploy.sh
[warning] 81-81: PRESERVE_ENV_VARS appears unused. Verify use (or export if used externally).
(SC2034)
playbooks/site-link-shared.sh
[warning] 45-45: PRESERVE_ENV_VARS appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (10)
scaffolds/hooks/2-releasing.sh (1)
50-61: Commented SQLite snippet looks good and consistentThe optional SQLite section is clearly documented and follows the same shared-path + symlink pattern as the rest of the Laravel hook. No functional or safety concerns since it’s fully commented out.
app/Console/Server/ServerLogsCommand.php (1)
221-291:displayServiceLogs()branching for all/system/site/PHP-FPM looks correctThe refactored
displayServiceLogs()cleanly separates cases:
all→ system, each detected service, each PHP-FPM log file, each site’s access log, and Docker when present.system→ just system logs.$servicematching a site key → that site’s Caddy access log.$servicestarting withphpand ending with-fpm→ corresponding/var/log/{service}.log.- Fallback → generic journalctl lookup for the named service.
The use of the processed
$detectedServices,$phpVersions,$sites, and$hasDockerkeeps the logic easy to follow and avoids recomputing. I don’t see correctness issues in the branching or paths given the surrounding conventions.app/SymfonyApp.php (1)
22-27: SiteDeployCommand is wired correctly into SymfonyAppImporting
SiteDeployCommandand adding it to the$commandsarray alongside the other site commands is consistent with the existing registration pattern and keeps all site operations grouped together.No issues from an architectural or wiring standpoint.
Also applies to: 166-173
app/Console/Site/SiteSharedPushCommand.php (1)
88-95: Destructuring server info for distro/permissions is consistent and type-safeUsing keyed array destructuring for
'distro'and'permissions'matchesServersTrait::serverInfo()usage (as inServerInstallCommand) and theserver-infoplaybook output. The@varannotations are helpful for static analysis; no functional issues here.playbooks/site-add.sh (1)
48-71: Atomic directory structure setup aligns with site-deploy expectationsCreating
releases/,shared/,repo/, andcurrent/publicunder/home/deployer/sites/{domain}matches the atomic model used bysite-deploy.sh(which later replacescurrentwith a symlink). Therun_cmd-wrappedmkdir -pand existence checks look safe and idempotent.playbooks/site-link-shared.sh (1)
32-41: Good safety checks around current release resolutionRequiring
currentto be a symlink and validatingRELEASE_PATHviareadlink -fbefore linking avoids corrupting a non-atomic layout (e.g., a still-provision-onlycurrentdirectory). Combined with the earlyfail()calls, this should produce clear errors ifsite:shared:pushis run before any successful deployment.app/Console/Site/SiteDeployCommand.php (4)
77-107: Missing-hooks flow is clear; reuse of--yessemantics is reasonableChecking for
.deployer/hooks/{1,2,3}-*.shviacheckRemoteHooksExist()and warning with explicit next steps (scaffold:hooksvs continue) is a nice touch. Reusing theyesoption viagetOptionOrPrompt('yes', ...)to auto-accept deploying without hooks when--yesis passed matches the “non-interactive” semantics of the flag; behavior looks intentional and consistent.
127-159: Server info handling and playbook invocation align with new shell scriptsDestructuring
$infointo'distro'and'permissions'mirrors the pattern inServerInstallCommandand ensures thesite-deployplaybook receives the same distro/perms context as other flows. PassingDEPLOYER_SITE_DOMAIN,DEPLOYER_SITE_REPO,DEPLOYER_SITE_BRANCH,DEPLOYER_PHP_VERSION, andDEPLOYER_KEEP_RELEASESdirectly from validated values keeps the Symfony and bash sides in sync. Control flow on integer (failure) vs array (success) fromexecutePlaybook()is correct.Also applies to: 184-202
263-285: keep-releases resolution and validation are sound
resolveKeepReleases():
- Treats empty / missing
--keep-releasesas the constant default.- Uses
ctype_digitand an explicit>= 1check to prevent invalid values.This matches the validation in
site-deploy.shand should prevent edge cases like negative or non-numeric input. No changes needed.
287-335: Code is correct; no changes neededThe web search confirms that
promptSelect()with a simple array accepts the option's value as thedefaultparameter. The code correctly passes$defaultSelection(a version string) rather than an index, so the implementation aligns with the documented behavior. The minor concern is resolved.
- Fix composer execution in build hooks to use proper PHP binary - Add error handling for Composer installation in PHP playbook - Move success message in site shared push to after complete operation
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Console/Server/ServerLogsCommand.php(9 hunks)app/Console/Site/SiteSharedPushCommand.php(2 hunks)playbooks/install-php.sh(1 hunks)scaffolds/hooks/1-building.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- scaffolds/hooks/1-building.sh
- playbooks/install-php.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use methods: inline if a method is called only once
Cache computed values: initialize expensive calculations in the constructor
Avoid method call overhead: prefer direct property access when appropriate
Files:
app/Console/Site/SiteSharedPushCommand.phpapp/Console/Server/ServerLogsCommand.php
🧬 Code graph analysis (2)
app/Console/Site/SiteSharedPushCommand.php (2)
app/Traits/PlaybooksTrait.php (1)
executePlaybook(45-187)app/Contracts/BaseCommand.php (1)
yay(177-181)
app/Console/Server/ServerLogsCommand.php (3)
app/Services/IOService.php (2)
info(474-477)writeln(463-469)app/Contracts/BaseCommand.php (1)
nay(186-190)app/DTOs/ServerDTO.php (1)
ServerDTO(7-19)
🔇 Additional comments (7)
app/Console/Site/SiteSharedPushCommand.php (2)
88-94: LGTM! Clean array destructuring with type hints.The array destructuring cleanly extracts distro and permissions from the server info, and the @var annotations provide helpful type hints for static analysis.
148-167: LGTM! Previous review concern addressed.The success message is now correctly displayed only after both the upload and linking steps complete successfully. This resolves the issue flagged in the previous review where success was declared prematurely.
app/Console/Server/ServerLogsCommand.php (5)
19-19: LGTM!The updated description accurately reflects the enhanced functionality for viewing PHP-FPM and site logs.
158-181: Robust handling of PHP versions and sites extraction.The defensive type checking for PHP versions handles various data formats gracefully, ensuring the code won't break with unexpected input structures. The sites extraction is straightforward and efficient.
346-366: LGTM!The
retrieveFileLogsmethod is well-structured with proper null handling and consistent error highlighting. The file path display is helpful for debugging.
325-325: LGTM!Error highlighting is consistently applied across all log retrieval paths, improving the user experience by making errors more visible in log output.
Also applies to: 338-338, 360-360, 393-393
246-262: The suggestion to add Caddy error logs is based on incorrect assumptions about Caddy's logging behavior.Caddy does not create per-site error log files by default — error logs default to stderr unless explicitly configured in the Caddyfile with the
errorsdirective. The code currently retrieves access logs from/var/log/caddy/{$site}-access.log, which is the appropriate default for Caddy's standard behavior. Error log files at/var/log/caddy/{$site}-error.logwould only exist if your deployment explicitly configured Caddy's error directive to write to those file paths—they are not a standard Caddy convention.The current implementation correctly retrieves the logs that Caddy generates by default.
Likely an incorrect or invalid review comment.
Use regex word boundaries for HTTP status codes (500, 502, 503, 504) to match edge cases like 'Status: 500', '[500]', or 'error_code:500' while avoiding false positives for numbers containing these codes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/Console/Server/ServerLogsCommand.php (4)
17-21: Help text and option descriptions mostly aligned; consider refiningservicehelp stringThe updated command description and
"All logs (System, Services, PHP, Sites)"label correctly reflect the new PHP-FPM and site log support. However, theserviceoption help ('Service name (all|system|detected service name)') is now slightly out of date, since users can also select PHP-FPM (phpX.Y-fpm) and site names. Consider expanding that string to mention PHP-FPM and site options so CLI help matches actual behavior.Also applies to: 38-41, 183-187
123-216: Processed-services aggregation is sound; tighten the return PHPDoc for better tooling supportThe updated
getProcessedServices()logic for detecting services, PHP versions, and sites is defensive and correctly guards against missing/malformed$infokeys, and the caching via$this->processedServiceskeeps it cheap at runtime. The return type, however, is now a well-defined structure (options, services, phpVersions, sites, hasDocker), but the PHPDoc still uses a genericarray<string, mixed>. Tightening that docblock would help static analysis and future readers.For example:
- /** - * Process detected services, PHP versions, and sites to build options. - * - * @param array<string, mixed> $info Server information from server-info playbook - * @return array<string, mixed> - */ + /** + * Process detected services, PHP versions, and sites to build options. + * + * @param array<string, mixed> $info Server information from server-info playbook + * @return array{ + * options: array<string, string>, + * services: list<string>, + * phpVersions: list<string>, + * sites: list<string>, + * hasDocker: bool + * } + */
223-293: Display logic is correct; you may want to avoid double‑printing PHP‑FPM logs in “all” modeThe branching in
displayServiceLogs()forall,system, site names, PHP-FPM units, and generic services is coherent and should behave as intended. One small behavioral nit: in theallbranch you iterate all$detectedServicesviaretrieveServiceLogs()and then separately show PHP-FPM file logs for each$phpVersionsentry. If a PHP-FPM unit is also present in the detected services list, users will see PHP-FPM logs twice.If you prefer to surface only the file-based PHP-FPM logs in the
allview (as your comment suggests), you could skip FPM-like entries when looping detected services, e.g.:- foreach ($detectedServices as $serviceName) { - $this->retrieveServiceLogs($server, $serviceName, $serviceName, $lines); - } + foreach ($detectedServices as $serviceName) { + $lower = strtolower($serviceName); + if (str_starts_with($lower, 'php') && str_ends_with($lower, '-fpm')) { + // Prefer file-based PHP-FPM logs handled below + continue; + } + + $this->retrieveServiceLogs($server, $serviceName, $serviceName, $lines); + }
349-365: Quote log file paths when building thetailcommand to handle spaces/special chars safely
retrieveFileLogs()now callsreadLogFile()with paths derived from PHP versions and site names, andtryTraditionalLogs()also feeds file paths found viafindintoreadLogFile(). Currently,readLogFile()interpolates$logFiledirectly into thetailcommand, which can break on paths with spaces or other shell‑significant characters.Even though your current file names are likely well‑formed, it’s safer and more robust to shell‑escape the path:
protected function readLogFile(ServerDTO $server, string $logFile, int $lines): ?string { try { - $result = $this->ssh->executeCommand($server, "tail -n {$lines} {$logFile} 2>/dev/null"); + $safeLogFile = escapeshellarg($logFile); + $safeLines = (int) $lines; + $result = $this->ssh->executeCommand($server, "tail -n {$safeLines} {$safeLogFile} 2>/dev/null");This keeps behavior identical while making the command resilient to unexpected characters in file paths.
Also applies to: 371-388, 409-418
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Console/Server/ServerLogsCommand.php(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use methods: inline if a method is called only once
Cache computed values: initialize expensive calculations in the constructor
Avoid method call overhead: prefer direct property access when appropriate
Files:
app/Console/Server/ServerLogsCommand.php
🧬 Code graph analysis (1)
app/Console/Server/ServerLogsCommand.php (3)
app/Services/IOService.php (2)
info(474-477)writeln(463-469)app/Contracts/BaseCommand.php (1)
nay(186-190)app/DTOs/ServerDTO.php (1)
ServerDTO(7-19)
🔇 Additional comments (1)
app/Console/Server/ServerLogsCommand.php (1)
298-344: Centralised error highlighting is a nice improvement and correctly fixes 5xx status detectionUsing
highlightErrors()fromretrieveServiceLogs(),retrieveFileLogs(), andtryTraditionalLogs()gives you a consistent highlighting pipeline for both journalctl and file-based logs. The helper itself is straightforward and efficient: it lowercases for keyword checks, then falls back to a word‑boundary regex for 500/502/503/504, which will correctly pick up codes at line boundaries and in bracketed/colon-separated forms (e.g.[500],HTTP status: 500) without matching1500etc.The implementation looks good as-is; no correctness issues stand out.
Also applies to: 346-366, 384-395, 424-472
- Update help text to mention PHP-FPM and site log options - Improve PHPDoc with structured array shape for better type safety - Prevent duplicate PHP-FPM logs in 'all' mode by filtering detected services - Secure file paths using escapeshellarg to prevent shell injection - Apply rector fix for first-class callable syntax
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Console/Server/ServerLogsCommand.php (1)
172-187: Consider simplifying the PHP version extraction logic.The nested conditionals with multiple type checks are complex and reduce readability. Consider extracting a helper method or simplifying with early returns.
Example refactor:
- if (isset($info['php']) && is_array($info['php']) && isset($info['php']['versions']) && is_array($info['php']['versions'])) { - foreach ($info['php']['versions'] as $versionData) { - $version = null; - if (is_array($versionData) && isset($versionData['version'])) { - /** @var string|int|float $rawVersion */ - $rawVersion = $versionData['version']; - $version = (string) $rawVersion; - } elseif (is_string($versionData) || is_numeric($versionData)) { - $version = (string) $versionData; - } - - if ($version !== null) { - $phpVersions[] = $version; - } - } - } + $versions = $info['php']['versions'] ?? []; + if (is_array($versions)) { + foreach ($versions as $versionData) { + $version = $this->extractVersion($versionData); + if ($version !== null) { + $phpVersions[] = $version; + } + } + }Then add a helper:
private function extractVersion(mixed $versionData): ?string { if (is_array($versionData) && isset($versionData['version'])) { return (string) $versionData['version']; } if (is_string($versionData) || is_numeric($versionData)) { return (string) $versionData; } return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Console/Server/ServerLogsCommand.php(11 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use methods: inline if a method is called only once
Cache computed values: initialize expensive calculations in the constructor
Avoid method call overhead: prefer direct property access when appropriate
Files:
app/Console/Server/ServerLogsCommand.php
🧬 Code graph analysis (1)
app/Console/Server/ServerLogsCommand.php (4)
app/Contracts/BaseCommand.php (2)
BaseCommand(30-244)nay(186-190)app/Services/IOService.php (1)
writeln(463-469)app/DTOs/ServerDTO.php (1)
ServerDTO(7-19)app/Services/SSHService.php (1)
executeCommand(69-102)
🔇 Additional comments (5)
app/Console/Server/ServerLogsCommand.php (5)
26-35: LGTM! Clear documentation of the expanded structure.The updated docblock accurately reflects the new shape with PHP versions and sites support.
236-312: LGTM! Well-structured service log routing.The logic correctly handles different service types and appropriately uses file-based logs for PHP-FPM and site logs while avoiding duplication in 'all' mode.
365-385: LGTM! Clean abstraction for file-based log retrieval.The new method is appropriately reused across PHP-FPM and site log retrievals, and properly integrates error highlighting.
428-442: LGTM! Proper shell escaping prevents injection.Good security practice using
escapeshellarg()to safely handle log file paths.
444-492: LGTM! HTTP status code matching properly addresses previous review.The regex pattern
/\b(500|502|503|504)\b/with word boundaries correctly matches status codes in various contexts (line boundaries, brackets, etc.) and fixes the issue raised in the previous review about space-padded literals.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.