feat: server management commands - #68
Conversation
- server-install.sh: Install Caddy, PHP 8.4, PHP-FPM, Git, Bun on Debian/Ubuntu - demo-site.sh: Create deployer user, configure permissions, setup demo site
- server:install - Install and prepare server for PHP applications - server:logs - View server logs (system and detected services) - server:run - Execute arbitrary commands on remote servers
- Add server management methods to ServersTrait - Extend Distribution enum with family() and isSupported() methods - Update DistributionFamily enum for better organization
Register ServerInstallCommand, ServerLogsCommand, and ServerRunCommand in the Symfony application command registry
…ment - Modify ServerAddCommand, ServerDeleteCommand, ServerInfoCommand - Update ServerProvisionDigitalOceanCommand integration - Enhance DigitalOceanAccountService for better server provisioning
- Refine comment structure guidelines in architecture rules - Update exception handling patterns and layer responsibilities - Improve documentation consistency across codebase
WalkthroughAdds three server CLI commands (install, logs, run), two shell playbooks (server-install, demo-site), enum-driven distribution validation, new server-info helpers in ServersTrait, updates several server command flows to use playbook-based server-info, and tightens messaging and error/display paths. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant InstallCmd as ServerInstallCommand
participant Servers as ServersTrait
participant Playbooks as PlaybooksTrait
participant SSH as SSHHost
participant Verifier as verifyInstallation()
User->>InstallCmd: run server:install
InstallCmd->>Servers: selectServer()
InstallCmd->>Servers: getServerInfo(server)
Servers->>Playbooks: executePlaybook("server-info")
Playbooks->>SSH: run playbook → returns info
Servers->>Servers: validateServerDistribution(info)
InstallCmd->>Playbooks: executePlaybook("server-install", env)
Playbooks->>SSH: provision packages/services
InstallCmd->>Playbooks: executePlaybook("demo-site", env)
Playbooks->>SSH: configure demo site
InstallCmd->>Verifier: verifyInstallation(url)
Verifier->>SSH: HTTP GET → response
Verifier-->>InstallCmd: success | warning | failure
InstallCmd-->>User: display results + replay
sequenceDiagram
actor User
participant LogsCmd as ServerLogsCommand
participant Servers as ServersTrait
participant ProcSvc as getProcessedServices()
participant Retriever as retrieveServiceLogs()
participant SSH as SSHHost
User->>LogsCmd: run server:logs
LogsCmd->>Servers: selectServer()
LogsCmd->>Servers: getServerInfo(server)
LogsCmd->>ProcSvc: getProcessedServices(info)
User-->>LogsCmd: choose service (all|system|specific)
LogsCmd->>Retriever: retrieveServiceLogs(...selected...)
Retriever->>SSH: run journalctl
alt journalctl returns data
SSH-->>Retriever: logs
else
Retriever->>SSH: read /var/log/* fallback
SSH-->>Retriever: traditional logs
end
LogsCmd-->>User: display logs + replay
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (3)
app/Enums/DistributionFamily.php (1)
12-24: Consider removing single-case enum.
DistributionFamilynow only has one case (DEBIAN) after removing FEDORA, REDHAT, and AMAZON. A single-case enum provides limited value over a string constant. Thenames()helper also adds complexity for a single-value array.If only Debian-family distributions are supported currently, consider:
- Option 1: Remove the enum entirely and use string literals
- Option 2: Keep the enum if additional families are planned soon
playbooks/server-install.sh (2)
41-47: Code duplication with demo-site.sh.The
run_cmd()function is identical to the one inplaybooks/demo-site.sh(lines 35-41). Consider extracting common helpers to a shared library file that both playbooks can source.# shared-helpers.sh run_cmd() { if [[ $DEPLOYER_PERMS == 'root' ]]; then "$@" else sudo -n "$@" fi }Then source it in both playbooks:
source "$(dirname "$0")/shared-helpers.sh"
129-331: Significant code duplication between Ubuntu and Debian branches.The Ubuntu (lines 133-211) and Debian (lines 212-299) branches share ~90% identical logic. The main differences are:
- Ubuntu:
add-apt-repository -y ppa:ondrej/php(line 168)- Debian: Sury PHP repository setup (lines 245-260)
Consider extracting common logic into helper functions:
setup_base_repos() { # Caddy repo setup (common) } setup_php_repo_ubuntu() { # Ubuntu-specific PPA } setup_php_repo_debian() { # Debian-specific Sury } install_all_packages() { setup_base_repos case $DEPLOYER_DISTRO in ubuntu) setup_php_repo_ubuntu ;; debian) setup_php_repo_debian ;; esac # Common package installation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.cursor/rules/01-architecture.mdc(1 hunks).cursor/rules/04-exceptions.mdc(8 hunks)app/Console/Server/ServerAddCommand.php(3 hunks)app/Console/Server/ServerDeleteCommand.php(3 hunks)app/Console/Server/ServerInfoCommand.php(1 hunks)app/Console/Server/ServerInstallCommand.php(1 hunks)app/Console/Server/ServerLogsCommand.php(1 hunks)app/Console/Server/ServerProvisionDigitalOceanCommand.php(1 hunks)app/Console/Server/ServerRunCommand.php(1 hunks)app/Enums/Distribution.php(1 hunks)app/Enums/DistributionFamily.php(1 hunks)app/Services/DigitalOcean/DigitalOceanAccountService.php(1 hunks)app/SymfonyApp.php(2 hunks)app/Traits/PlaybooksTrait.php(1 hunks)app/Traits/ServersTrait.php(3 hunks)playbooks/demo-site.sh(1 hunks)playbooks/server-install.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/Enums/Distribution.phpapp/Traits/PlaybooksTrait.phpapp/Console/Server/ServerInfoCommand.phpapp/Console/Server/ServerAddCommand.phpapp/SymfonyApp.phpapp/Services/DigitalOcean/DigitalOceanAccountService.phpapp/Traits/ServersTrait.phpapp/Enums/DistributionFamily.phpapp/Console/Server/ServerLogsCommand.phpapp/Console/Server/ServerProvisionDigitalOceanCommand.phpapp/Console/Server/ServerRunCommand.phpapp/Console/Server/ServerInstallCommand.phpapp/Console/Server/ServerDeleteCommand.php
🧠 Learnings (21)
📚 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 : Use SymfonyStyle consistently for all user-facing console output
Applied to files:
app/SymfonyApp.phpapp/Console/Server/ServerLogsCommand.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.phpapp/Console/Server/ServerLogsCommand.php.cursor/rules/04-exceptions.mdc
📚 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.phpapp/Console/Server/ServerLogsCommand.php.cursor/rules/04-exceptions.mdc
📚 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.phpapp/Console/Server/ServerLogsCommand.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.phpapp/Console/Server/ServerLogsCommand.phpapp/Console/Server/ServerRunCommand.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 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 must not contain business logic—delegate to Services
Applied to files:
app/SymfonyApp.php
📚 Learning: 2025-10-24T20:03:23.947Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/rules.mdc:0-0
Timestamp: 2025-10-24T20:03:23.947Z
Learning: Applies to **/*.{md,mdc} : Total rule corpus target: <3000 tokens (~600–800 lines)
Applied to files:
.cursor/rules/01-architecture.mdc
📚 Learning: 2025-10-24T20:03:23.947Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/rules.mdc:0-0
Timestamp: 2025-10-24T20:03:23.947Z
Learning: Applies to **/*.{md,mdc} : Target rule density per file: Architecture/Patterns 80–120 lines
Applied to files:
.cursor/rules/01-architecture.mdc
📚 Learning: 2025-10-24T20:03:23.947Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/rules.mdc:0-0
Timestamp: 2025-10-24T20:03:23.947Z
Learning: Applies to **/*.{md,mdc} : After each section header, include: "All rules MANDATORY unless marked optional."
Applied to files:
.cursor/rules/01-architecture.mdc.cursor/rules/04-exceptions.mdc
📚 Learning: 2025-10-24T20:03:23.947Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/rules.mdc:0-0
Timestamp: 2025-10-24T20:03:23.947Z
Learning: Applies to **/*.{md,mdc} : Before commit: confirm critical rules are still emphasized (without repetition)
Applied to files:
.cursor/rules/01-architecture.mdc.cursor/rules/04-exceptions.mdc
📚 Learning: 2025-09-22T11:10:45.309Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/02-code-quality.mdc:0-0
Timestamp: 2025-09-22T11:10:45.309Z
Learning: Applies to {app,tests}/**/*.php : Format comment sections as headers/subheaders/paragraphs and separate them with a single newline
Applied to files:
.cursor/rules/01-architecture.mdc
📚 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: Group related functions into comment-separated sections
Applied to files:
.cursor/rules/01-architecture.mdc
📚 Learning: 2025-09-22T11:10:45.309Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/02-code-quality.mdc:0-0
Timestamp: 2025-09-22T11:10:45.309Z
Learning: Applies to {app,tests}/**/*.php : Use comments to separate sections of code and to explain or summarize complex logic; avoid commenting the obvious
Applied to files:
.cursor/rules/01-architecture.mdc
📚 Learning: 2025-10-24T20:03:23.947Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/rules.mdc:0-0
Timestamp: 2025-10-24T20:03:23.947Z
Learning: Applies to **/*.{md,mdc} : Include a single "All rules MANDATORY" statement per file
Applied to files:
.cursor/rules/04-exceptions.mdc
📚 Learning: 2025-10-24T20:03:23.947Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/rules.mdc:0-0
Timestamp: 2025-10-24T20:03:23.947Z
Learning: Applies to **/*.{md,mdc} : Start each rule file with YAML front matter containing alwaysApply: true
Applied to files:
.cursor/rules/04-exceptions.mdc
📚 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 : Validation errors and business exceptions should bubble up to Commands for display
Applied to files:
.cursor/rules/04-exceptions.mdc
📚 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 : Services return exceptions or structured data for Commands to handle
Applied to files:
.cursor/rules/04-exceptions.mdc
📚 Learning: 2025-10-24T20:00:14.534Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-24T20:00:14.534Z
Learning: Layer strategy: CLI Commands → integration tests; Business Services → unit tests (mocked); Utilities/Helpers → unit tests
Applied to files:
.cursor/rules/04-exceptions.mdc
🧬 Code graph analysis (8)
app/Console/Server/ServerAddCommand.php (3)
app/Traits/ServersTrait.php (1)
getServerInfo(48-64)app/Repositories/ServerRepository.php (1)
create(48-65)app/Contracts/BaseCommand.php (1)
nay(190-194)
app/Services/DigitalOcean/DigitalOceanAccountService.php (1)
app/Enums/Distribution.php (1)
isSupported(37-43)
app/Traits/ServersTrait.php (5)
app/DTOs/ServerDTO.php (1)
ServerDTO(7-19)app/Services/IOService.php (3)
info(469-472)displayDeets(538-561)writeln(458-464)app/Traits/PlaybooksTrait.php (1)
executePlaybook(46-181)app/Contracts/BaseCommand.php (1)
nay(190-194)app/Enums/Distribution.php (2)
displayName(48-60)isSupported(37-43)
app/Enums/DistributionFamily.php (1)
app/Enums/Distribution.php (1)
family(26-32)
playbooks/server-install.sh (1)
playbooks/demo-site.sh (1)
run_cmd(36-42)
app/Console/Server/ServerLogsCommand.php (5)
app/DTOs/ServerDTO.php (1)
ServerDTO(7-19)app/Console/Server/ServerRunCommand.php (3)
AsCommand(15-118)configure(27-33)execute(39-117)app/Traits/ServersTrait.php (2)
selectServer(181-219)displayServerDeets(224-244)app/Services/IOService.php (4)
getOptionOrPrompt(84-132)promptText(200-216)promptSelect(296-312)writeln(458-464)app/Services/SSHService.php (1)
executeCommand(69-102)
app/Console/Server/ServerRunCommand.php (4)
app/Contracts/BaseCommand.php (3)
BaseCommand(29-235)nay(190-194)showCommandReplay(201-234)app/Traits/ServersTrait.php (2)
selectServer(181-219)displayServerDeets(224-244)app/Services/IOService.php (4)
getOptionOrPrompt(84-132)promptText(200-216)writeln(458-464)write(448-451)app/Services/SSHService.php (1)
executeCommand(69-102)
app/Console/Server/ServerDeleteCommand.php (2)
app/Contracts/BaseCommand.php (1)
yay(181-185)app/Services/IOService.php (1)
writeln(458-464)
🔇 Additional comments (8)
.cursor/rules/01-architecture.mdc (1)
127-139: LGTM! Documentation structure improvements.The hierarchical comment markers (h1, h2, h3, p) provide clearer structure for the coding guidelines documentation.
app/Traits/PlaybooksTrait.php (1)
114-114: LGTM! Improved timeout message clarity.The simplified introductory sentence makes the timeout notification more direct while the subsequent bullet points still provide actionable guidance.
app/Enums/Distribution.php (1)
34-43: LGTM! Clear distribution support gating.The method correctly identifies UBUNTU and DEBIAN as the only supported distributions, which aligns with the current implementation scope.
app/Services/DigitalOcean/DigitalOceanAccountService.php (1)
100-111: LGTM! Improved type safety with enum-based filtering.The refactoring from
in_array()toDistribution::tryFrom()+isSupported()centralizes distribution validation logic and improves type safety.playbooks/server-install.sh (2)
87-123: LGTM! Robust retry logic with proper error discrimination.The implementation correctly:
- Differentiates lock-related errors from other failures
- Uses exponential backoff to prevent tight retry loops
- Fails fast on non-lock errors
- Provides clear user feedback
348-410: LGTM! Clean validation and YAML output generation.The PHP version validation correctly uses
awkfor numeric comparison, and the YAML output generation properly handles errors and provides structured results..cursor/rules/04-exceptions.mdc (1)
1-220: LGTM! Documentation aligned with error handling conventions.The updates consistently reflect the migration from
$this->io->error()to$this->nay()and emphasize avoiding redundant "Failed to" prefixes. These changes align with the architectural guidelines.app/Console/Server/ServerProvisionDigitalOceanCommand.php (1)
183-190: LGTM! Enhanced validation with server info retrieval.Replacing SSH verification with
getServerInfo()provides more comprehensive validation by confirming both SSH connectivity and distribution support in a single step. The error handling correctly propagates failure codes.
Add cleanup of DigitalOcean droplet when server creation fails after droplet provisioning, preventing orphaned cloud resources.
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 (1)
app/Console/Server/ServerProvisionDigitalOceanCommand.php(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/Server/ServerProvisionDigitalOceanCommand.php
🧬 Code graph analysis (1)
app/Console/Server/ServerProvisionDigitalOceanCommand.php (3)
app/Traits/ServersTrait.php (1)
getServerInfo(48-64)app/Repositories/ServerRepository.php (1)
create(48-65)app/Contracts/BaseCommand.php (1)
nay(190-194)
🔇 Additional comments (1)
app/Console/Server/ServerProvisionDigitalOceanCommand.php (1)
199-199: LGTM: Cleaner error message.The simplified error message is appropriate since
ServerRepository::create()already throws exceptions with descriptive messages like "Server '{$server->name}' already exists".
The php_fastcgi directive requires a double slash (unix//) prefix for Unix socket paths. Without the second slash, Caddy would fail to connect to PHP-FPM, causing all PHP requests to return 502 errors. This ensures the socket path is correctly formatted as unix//run/php/... instead of unix/run/php/...
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Enums/Distribution.php (1)
23-43: Consider reducing duplication betweenfamily()andisSupported().Both methods encode the same knowledge (UBUNTU and DEBIAN are supported) using identical match patterns. This creates a maintenance burden—adding support for a new distribution family requires updating both methods in sync.
Consider refactoring to establish a single source of truth. One approach:
public function family(): DistributionFamily { + if (!$this->isSupported()) { + throw new \RuntimeException("Distribution '{$this->value}' is not supported. Use isSupported() to check before calling family()"); + } + return match ($this) { self::UBUNTU, self::DEBIAN => DistributionFamily::DEBIAN, - default => throw new \RuntimeException("Distribution '{$this->value}' is not supported. Use isSupported() to check before calling family()"), }; }Alternatively, define a private static array of supported distributions that both methods reference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Enums/Distribution.php(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/Enums/Distribution.php
🧬 Code graph analysis (1)
app/Enums/Distribution.php (1)
app/Enums/DistributionFamily.php (1)
DistributionFamily(12-19)
🔇 Additional comments (1)
app/Enums/Distribution.php (1)
23-32: Past concern addressed - exception-based approach is correct.The previous review flagged the misleading
DistributionFamily::DEBIANfallback. The current implementation correctly throws aRuntimeExceptionfor unsupported distributions, which ensures fail-fast behavior and prevents bugs from bypassed validation.
Refactor post-provisioning steps into a single try-finally block with automatic rollback on any failure. This ensures droplets are never left orphaned if errors occur during: - Waiting for droplet activation - Retrieving IP address - Verifying server connectivity and distribution - Adding server to inventory Uses a $shouldKeepDroplet flag that only becomes true when all steps complete successfully. Converts getServerInfo() int returns to exceptions to ensure they trigger the catch block and rollback. Consolidates 4 separate rollback calls into 1 location for easier maintenance and guaranteed cleanup.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
playbooks/demo-site.sh (1)
234-234: Fix Caddy php_fastcgi unix socket path.The
php_fastcgidirective requires the unix socket path in the formunix//…. The current code producesunix/run/...(single slash), which prevents Caddy from connecting to PHP-FPM, causing all PHP requests to return 502 errors.Apply this diff:
- php_fastcgi unix/${php_fpm_socket} + php_fastcgi unix//${php_fpm_socket}Based on learnings from past review comments.
🧹 Nitpick comments (2)
playbooks/demo-site.sh (2)
21-21: Consider addingset -efor stricter error handling.While
set -o pipefailis good for catching pipeline failures, addingset -ewould make the script exit immediately on any command failure, providing additional safety. This is especially important for a provisioning script where partial execution can leave the system in an inconsistent state.Apply this diff:
-set -o pipefail +set -eo pipefail
54-54: Strengthen PHP-FPM user parsing.The
awk '{print $3}'assumes the config format isuser = value ;with spaces. If the format varies (e.g.,user=value;), this will fail silently and fall back to 'apache', which may be incorrect.Consider using a more robust pattern:
- user=$(grep -E '^\s*user\s*=' "$config_file" | awk '{print $3}' | tr -d ';') + user=$(grep -E '^\s*user\s*=' "$config_file" | sed -E 's/^\s*user\s*=\s*([^;[:space:]]+).*/\1/')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Console/Server/ServerProvisionDigitalOceanCommand.php(1 hunks)playbooks/demo-site.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/Server/ServerProvisionDigitalOceanCommand.php
🔇 Additional comments (9)
playbooks/demo-site.sh (4)
36-42: LGTM!The
run_cmdhelper cleanly abstracts permission handling and the use ofsudo -nprevents password prompts, which is appropriate for automated provisioning.
80-134: LGTM!The user and group management logic is well-structured with proper error handling. The approach of adding web server users to the deployer group and restarting services to apply group membership changes is correct. The warning for missing PHP-FPM users prevents hard failures while alerting operators.
136-184: LGTM!The demo site setup correctly creates the directory structure, generates a simple PHP file, and applies appropriate ownership and permissions (750 for directories, 640 for files) to enable group-based access by Caddy and PHP-FPM.
266-282: LGTM!The main execution flow is clean and sequential. The YAML output properly reports the provisioning status and key details as documented in the header comments.
app/Console/Server/ServerProvisionDigitalOceanCommand.php (5)
140-144: LGTM!The
shouldKeepDropletflag with a fail-closed default (false) ensures automatic rollback unless all provisioning steps succeed. This is a safe and correct approach.
145-168: LGTM!The droplet initialization flow correctly sequences: wait for readiness → retrieve IP → create server DTO → display details. Moving the server DTO creation inside the try block ensures it's only created for active droplets and is available for both validation and display.
170-181: Excellent rollback protection.The flow now properly validates the server and adds it to inventory within the try block. The
shouldKeepDropletflag is only set to true after successful inventory addition, ensuring automatic rollback if any step fails (SSH validation, distribution check, or inventory persistence). This addresses the orphaned droplet concerns from previous reviews.Based on learnings from past review comments about missing rollback logic.
182-188: LGTM!The exception handling and finally block work together correctly: exceptions are caught and displayed, then the finally block ensures rollback occurs if
shouldKeepDropletis still false. This guarantees cleanup even if an unexpected exception occurs.
190-192: LGTM!The final failure check correctly prevents the command from showing success output and command replay when rollback occurred. This ensures the command's exit code accurately reflects the provisioning outcome.
| get_php_fpm_service() { | ||
| if [[ $DEPLOYER_FAMILY == 'debian' ]]; then | ||
| echo 'php8.4-fpm' | ||
| else | ||
| echo 'php-fpm' | ||
| fi | ||
| } |
There was a problem hiding this comment.
Avoid hardcoding PHP version.
The function returns php8.4-fpm for Debian, but the installed PHP version may differ (e.g., 8.3, 8.5). This would cause service restart commands to fail silently or target the wrong service.
Consider dynamically detecting the PHP version:
get_php_fpm_service() {
if [[ $DEPLOYER_FAMILY == 'debian' ]]; then
- echo 'php8.4-fpm'
+ # Detect installed PHP-FPM version
+ local php_version
+ php_version=$(php -r 'echo PHP_MAJOR_VERSION . "." . PHP_MINOR_VERSION;' 2>/dev/null || echo "8.4")
+ echo "php${php_version}-fpm"
else
echo 'php-fpm'
fi
}
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation