Skip to content

feat: site atomic deployments - #98

Merged
loadinglucian merged 10 commits into
mainfrom
feat/site-atomic-deployments
Nov 20, 2025
Merged

feat: site atomic deployments#98
loadinglucian merged 10 commits into
mainfrom
feat/site-atomic-deployments

Conversation

@loadinglucian

@loadinglucian loadinglucian commented Nov 20, 2025

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • New Features

    • Interactive site deployment command with PHP-version choice, release retention, hooks handling, replay snippet and post-deploy summary.
    • Log viewer now shows PHP‑FPM and per‑site file logs with error‑line highlighting.
  • Improvements

    • Automatic linking of shared resources during deployments.
    • Ensure Composer is present before PHP package installation.
    • Adopt atomic release layout and add full atomic site‑deploy and site‑linking playbooks/scripts.

✏️ Tip: You can customize this high-level summary in your review settings.

…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
@coderabbitai

coderabbitai Bot commented Nov 20, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Adds 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

Cohort / File(s) Summary
Server logs & helpers
app/Console/Server/ServerLogsCommand.php
Reworked processed result shape to include options, services, phpVersions, sites, hasDocker; added retrieveFileLogs() and highlightErrors(); lowercased/typed detected services; display flow now includes system, service journal logs, PHP‑FPM file logs, site file logs and Docker; file paths escaped when tailing.
Site deploy command
app/Console/Site/SiteDeployCommand.php
New site:deploy Symfony command: selects site, validates hooks, resolves branch/php/keep‑releases, prompts, runs site-deploy.sh via playbooks with DEPLOYER_* env vars, prints deployment summary and replay snippet; new helpers for resolving PHP and keep‑releases and checking hooks.
Command registration
app/SymfonyApp.php
Registers SiteDeployCommand in application command list and adds import.
Site shared push
app/Console/Site/SiteSharedPushCommand.php
After upload, runs site-link-shared.sh with DEPLOYER_DISTRO/DEPLOYER_PERMS/DEPLOYER_SITE_DOMAIN; returns early on playbook error; updates success message to indicate upload + link.
Deployment playbooks
playbooks/site-deploy.sh, playbooks/site-link-shared.sh
site-deploy.sh: atomic release workflow (prepare dirs, detect PHP, clone/update bare repo, build release, link shared, run hooks 1/2/3, activate release, reload PHP‑FPM, cleanup, write YAML output). site-link-shared.sh: validates env, resolves release, calls link_shared_resources, writes YAML output.
Playbook helpers & PHP install
playbooks/helpers.sh, playbooks/install-php.sh, playbooks/site-add.sh
helpers.sh: adds run_as_deployer(), fail(), link_shared_resources(). install-php.sh: preflight installs Composer if missing. site-add.sh: switch to atomic dirs (releases/shared/repo/current) and set directory perms to 755.
Hook scaffolds
scaffolds/hooks/1-building.sh, scaffolds/hooks/2-releasing.sh
1-building.sh: run Composer via discovered composer binary invoked through DEPLOYER_PHP. 2-releasing.sh: adds commented SQLite shared-db scaffold (no active behavior).

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Files warranting extra attention:
    • playbooks/site-deploy.sh (git/bare-repo flow, release activation, cleanup, YAML output)
    • playbooks/helpers.sh / playbooks/site-link-shared.sh (permission-aware linking, run_as_deployer and fail semantics)
    • app/Console/Server/ServerLogsCommand.php (path escaping, retrieveFileLogs integration, highlightErrors regex/output)

Possibly related PRs

  • bigpixelrocket/deployer-php#92 — touches SiteSharedPushCommand; related because both PRs modify shared-file upload/link flow.
  • bigpixelrocket/deployer-php#68 — earlier changes to ServerLogsCommand; directly related to the server log handling refactor here.
  • bigpixelrocket/deployer-php#64 — changes to PlaybooksTrait / executePlaybook signatures; relevant to new SiteDeployCommand playbook execution.

Poem

🐰
I hop between releases, tidy and spry,
Symlinks click, hooks hum — watch code sail by,
PHP wakes, logs shimmer where bugs once lay,
Atomic hops keep messy deploys at bay,
A carrot for each clean, successful try.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: site atomic deployments' directly and clearly summarizes the main change: introducing atomic deployment functionality for sites throughout the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 97.30% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/site-atomic-deployments

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 versions

The 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 options map plus structured services/phpVersions/sites/hasDocker for 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 simple array_unique() on $phpVersions before 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 only

The new pieces work together nicely:

  • retrieveServiceLogs() and tryTraditionalLogs() now consistently route output through highlightErrors(), 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 recompute strtolower($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_VARS

The run_as_deployer wrapper correctly routes through sudo -n -u deployer when DEPLOYER_PERMS is root|sudo, and falls back to direct execution otherwise. It assumes PRESERVE_ENV_VARS is set by the calling playbook, which site-deploy.sh and site-link-shared.sh now 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_PATH

The shared-resource linker is careful about:

  • No-op when $SHARED_PATH is missing or empty
  • Per-item rm-then-symlink with run_cmd/run_as_deployer
  • Hard-failing via fail() if removal or linking fails

Given the use of rm -rf "$release_item", it’s important that all call sites guarantee sane SHARED_PATH/RELEASE_PATH values (which site-deploy.sh and site-link-shared.sh do). 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_VARS is set here and then consumed indirectly by run_as_deployer from the inlined helpers.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 helpers

The required DEPLOYER_* variables are validated early and DEPLOYER_KEEP_RELEASES is sanitized to a positive integer, which matches the PHP-side validation in SiteDeployCommand. Exporting DEPLOYER_RELEASE_PATH, DEPLOYER_SHARED_PATH, DEPLOYER_CURRENT_PATH, DEPLOYER_REPO_PATH, etc. plus the PRESERVE_ENV_VARS list aligns with run_as_deployer’s --preserve-env usage.

ShellCheck’s SC2034 about PRESERVE_ENV_VARS being 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 presence

The overall sequence:

  • clone_or_update_repo with host key management
  • build_release via git archive into a timestamped directory
  • link_shared_resources (from helpers)
  • Hook chain (1-building.sh, 2-releasing.sh, 3-finishing.sh)
  • activate_release + reload_php_fpm + cleanup_releases

is well-structured and matches the atomic layout created by site-add.sh. The use of fail() 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*-fpm unit, deployments will hard-fail here. That may be acceptable given your server:install flow, but if you ever support CLI-only PHP installs, you might want to downgrade this to a warning (similar to how site-add.sh handles 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, and yes map cleanly to the underlying playbook parameters and UX. The description on keep-releases matches DEFAULT_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(), and checkRemoteHooksExist() 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 into execute() 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() and checkRemoteHooksExist() are the easiest candidates to inline with minimal impact.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2718b55 and d7d13c8.

📒 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.php
  • app/SymfonyApp.php
  • app/Console/Server/ServerLogsCommand.php
  • app/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.sh
  • playbooks/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.sh
  • playbooks/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 consistent

The 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 correct

The 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.
  • $service matching a site key → that site’s Caddy access log.
  • $service starting with php and 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 $hasDocker keeps 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 SymfonyApp

Importing SiteDeployCommand and adding it to the $commands array 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-safe

Using keyed array destructuring for 'distro' and 'permissions' matches ServersTrait::serverInfo() usage (as in ServerInstallCommand) and the server-info playbook output. The @var annotations are helpful for static analysis; no functional issues here.

playbooks/site-add.sh (1)

48-71: Atomic directory structure setup aligns with site-deploy expectations

Creating releases/, shared/, repo/, and current/public under /home/deployer/sites/{domain} matches the atomic model used by site-deploy.sh (which later replaces current with a symlink). The run_cmd-wrapped mkdir -p and existence checks look safe and idempotent.

playbooks/site-link-shared.sh (1)

32-41: Good safety checks around current release resolution

Requiring current to be a symlink and validating RELEASE_PATH via readlink -f before linking avoids corrupting a non-atomic layout (e.g., a still-provision-only current directory). Combined with the early fail() calls, this should produce clear errors if site:shared:push is run before any successful deployment.

app/Console/Site/SiteDeployCommand.php (4)

77-107: Missing-hooks flow is clear; reuse of --yes semantics is reasonable

Checking for .deployer/hooks/{1,2,3}-*.sh via checkRemoteHooksExist() and warning with explicit next steps (scaffold:hooks vs continue) is a nice touch. Reusing the yes option via getOptionOrPrompt('yes', ...) to auto-accept deploying without hooks when --yes is 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 scripts

Destructuring $info into 'distro' and 'permissions' mirrors the pattern in ServerInstallCommand and ensures the site-deploy playbook receives the same distro/perms context as other flows. Passing DEPLOYER_SITE_DOMAIN, DEPLOYER_SITE_REPO, DEPLOYER_SITE_BRANCH, DEPLOYER_PHP_VERSION, and DEPLOYER_KEEP_RELEASES directly from validated values keeps the Symfony and bash sides in sync. Control flow on integer (failure) vs array (success) from executePlaybook() is correct.

Also applies to: 184-202


263-285: keep-releases resolution and validation are sound

resolveKeepReleases():

  • Treats empty / missing --keep-releases as the constant default.
  • Uses ctype_digit and an explicit >= 1 check to prevent invalid values.

This matches the validation in site-deploy.sh and should prevent edge cases like negative or non-numeric input. No changes needed.


287-335: Code is correct; no changes needed

The web search confirms that promptSelect() with a simple array accepts the option's value as the default parameter. 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.

Comment thread app/Console/Site/SiteSharedPushCommand.php
Comment thread playbooks/install-php.sh
Comment thread playbooks/site-add.sh
Comment thread scaffolds/hooks/1-building.sh
- 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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d7d13c8 and 8ee93ce.

📒 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.php
  • app/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 retrieveFileLogs method 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 errors directive. 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.log would 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.

Comment thread app/Console/Server/ServerLogsCommand.php Outdated
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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
app/Console/Server/ServerLogsCommand.php (4)

17-21: Help text and option descriptions mostly aligned; consider refining service help string

The updated command description and "All logs (System, Services, PHP, Sites)" label correctly reflect the new PHP-FPM and site log support. However, the service option 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 support

The updated getProcessedServices() logic for detecting services, PHP versions, and sites is defensive and correctly guards against missing/malformed $info keys, and the caching via $this->processedServices keeps 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 generic array<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” mode

The branching in displayServiceLogs() for all, system, site names, PHP-FPM units, and generic services is coherent and should behave as intended. One small behavioral nit: in the all branch you iterate all $detectedServices via retrieveServiceLogs() and then separately show PHP-FPM file logs for each $phpVersions entry. 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 all view (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 the tail command to handle spaces/special chars safely

retrieveFileLogs() now calls readLogFile() with paths derived from PHP versions and site names, and tryTraditionalLogs() also feeds file paths found via find into readLogFile(). Currently, readLogFile() interpolates $logFile directly into the tail command, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8ee93ce and ded49d6.

📒 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 detection

Using highlightErrors() from retrieveServiceLogs(), retrieveFileLogs(), and tryTraditionalLogs() 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 matching 1500 etc.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ded49d6 and 9b0937c.

📒 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.

@loadinglucian
loadinglucian merged commit b59f685 into main Nov 20, 2025
5 checks passed
@loadinglucian
loadinglucian deleted the feat/site-atomic-deployments branch November 20, 2025 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant