feat: site management add delete - #90
Conversation
- Add validateSiteDomain() for domain format and uniqueness - Add validateSiteBranch() for branch name validation - Add validateSiteRepo() for git repository URL format
- Add server validation (Caddy and PHP installation checks) - Implement PHP version selection for site-specific configuration - Add git repository and branch input handling with validation - Create site-add.sh playbook for Capistrano-style directory structure - Configure Caddy virtual host with PHP-FPM integration - Add site to inventory after successful provisioning
- Add double confirmation (type domain name + yes/no prompt) - Gracefully handle missing servers in inventory - Create site-delete.sh playbook for server cleanup - Remove Caddy virtual host configuration and reload service - Remove site files and directory structure - Update inventory after successful removal
…ection - Improve PHP version selection logic for multi-version environments - Add support for PHP version-specific extension selection - Handle default PHP version setting when multiple versions installed - Better integration with site-specific PHP version requirements
WalkthroughThe pull request introduces server-side site provisioning and deletion capabilities. It adds playbook execution to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as site:add CLI
participant Validator
participant PhpSelector as PHP Selector
participant SiteGatherer as Site Info
participant Provisioner as Playbook Executor
participant Server
participant OutputHandler
User->>CLI: Execute site:add
CLI->>Validator: validateServerReady()
Validator-->>CLI: Check for Caddy & PHP
alt Server Not Ready
CLI-->>User: Error: Services missing
end
CLI->>PhpSelector: selectPhpVersion()
alt Multiple Versions
PhpSelector->>User: Prompt for PHP version
User-->>PhpSelector: Select version
else Single Version
PhpSelector-->>CLI: Auto-select
end
CLI->>SiteGatherer: gatherSiteInfo()
SiteGatherer->>User: Prompt for domain, path, etc.
User-->>SiteGatherer: Provide details
SiteGatherer-->>CLI: Return SiteDTO
CLI->>Provisioner: executePlaybook(site-add)
Provisioner->>Server: site-add.sh with env vars
Server->>Server: setup_site_directories()
Server->>Server: setup_demo_page()
Server->>Server: configure_caddy_vhost()
Server->>Server: reload_services()
Server->>OutputHandler: Write YAML to DEPLOYER_OUTPUT_FILE
Provisioner-->>CLI: Parse YAML output
CLI-->>User: Success message + next steps
sequenceDiagram
participant User
participant CLI as site:delete CLI
participant SiteSelector
participant ServerLocator
participant Provisioner as Playbook Executor
participant Server
participant Inventory
participant OutputHandler
User->>CLI: Execute site:delete --domain=example.com
CLI->>SiteSelector: Select site from inventory
SiteSelector-->>CLI: Return SiteDTO
CLI->>ServerLocator: Locate server
alt Server Found
ServerLocator-->>CLI: Server connection ready
CLI->>Provisioner: executePlaybook(site-delete)
Provisioner->>Server: site-delete.sh with env vars
Server->>Server: remove_caddy_vhost()
Server->>Server: reload_caddy()
Server->>Server: remove_site_files()
Server->>OutputHandler: Write success YAML
Provisioner-->>CLI: Removed from server
else Server Not Found/Unreachable
ServerLocator-->>CLI: Server unavailable
CLI->>User: Prompt: Remove from inventory anyway?
end
alt User Confirms
CLI->>Inventory: Delete site from inventory
Inventory-->>CLI: Deleted
CLI-->>User: Success message
else User Cancels
CLI-->>User: Abort
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (5)
playbooks/site-delete.sh (1)
1-107: Site deletion playbook flow is solid; only minor cleanliness nitThe overall sequence (remove Caddy vhost → reload Caddy → remove site files → write YAML status) is sound, with good use of
run_cmdand explicit error checks per step. Validation of required env vars and exportingDEPLOYER_PERMSbefore calling helpers also looks correct.The only small nit:
DEPLOYER_DISTROis required but not used anywhere in this script. If there’s no distro-specific behavior expected here, you could drop that requirement to avoid confusion; if it will be used later, consider a brief comment to that effect.app/Console/Site/SiteDeleteCommand.php (1)
101-190: Server-side removal flow is robust; only minor UX considerationThe added server-removal logic is well thought out:
- Looks up the server from the site record and gracefully handles a missing server in inventory.
- Verifies connectivity via
serverInfo()before running thesite-deleteplaybook.- Distinguishes between “removed from server” and “removed from inventory only” in messaging.
- Uses a second confirmation (
Remove site from inventory anyway?) when server-side removal didn’t happen, which is controlled by the same--yesoption (no extra prompts in non-interactive runs).Return codes (FAILURE when you don’t remove from inventory due to server issues, SUCCESS when deletion proceeds) are reasonable. The replay options, especially always setting
'force' => true, make replays non-interactive and safe for scripting.If you want, you could add a short warning before returning
Command::FAILUREwhen the user declines “inventory only” removal, to make it more obvious in logs why nothing was deleted, but that’s purely UX polish.app/Console/Site/SiteAddCommand.php (3)
54-63: execute() flow is sound with good short‑circuiting; watch for partial‑failure consistencyThe stepwise flow—select server → fetch/validate server info → check readiness → choose PHP version → gather site info → run
site-addplaybook → persist to inventory → show next steps/replay—reads clearly, and every operation that can return anintfailure code is short‑circuited appropriately.One thing to be aware of is consistency on partial failures: if the playbook succeeds but
$this->sites->create($site)throws, the site will be provisioned on the server but missing from inventory. If that becomes a practical issue, consider either:
- Ensuring the
site-addplaybook is idempotent so rerunningsite:addis safe, or- Adding a compensating rollback (e.g. invoking
site:delete/site-deleteplaybook) when inventory writes fail.Not a blocker, but worth deciding on a convention for server vs. inventory consistency.
Also applies to: 66-103, 133-155, 170-193
198-231: Server readiness check is defensive; consider more specific messaging and potential reuseThe Caddy/PHP presence checks and early
Command::FAILUREreturn are good guards before attempting provisioning. Two small improvements to consider:
- The error message is quite generic; explicitly mentioning which component(s) are missing (Caddy, PHP, or both) would make troubleshooting easier.
- If this logic ends up shared with other commands (e.g. delete/deploy flows), it might be worth moving into a shared trait/helper instead of keeping it as a single-use private method here; otherwise, inlining into
execute()would align with the “no single-use methods” guideline.
233-297: PHP version selection is robust; minor polish possible for defaults and duplicatesHandling both structured (
['version' => '8.2']) and legacy scalar version entries, plus only prompting when multiple versions exist, is nicely done. A couple of low-impact refinements you might consider:
- Deduplicate
$installedPhpVersionsbefore sorting to avoid repeated entries in the select prompt:$installedPhpVersions = array_values(array_unique($installedPhpVersions)); rsort($installedPhpVersions, SORT_NATURAL);- When deriving
$defaultVersionStr, guard against aphp['default']value that isn’t actually in$installedPhpVersions, so the prompt’s default always corresponds to a real option:if ($defaultVersion !== null && in_array((string) $defaultVersion, $installedPhpVersions, true)) { $defaultVersionStr = (string) $defaultVersion; } else { $defaultVersionStr = $installedPhpVersions[0]; }These are purely quality-of-life tweaks; the current logic is functionally sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/Console/Server/ServerInstallCommand.php(1 hunks)app/Console/Site/SiteAddCommand.php(7 hunks)app/Console/Site/SiteDeleteCommand.php(3 hunks)app/Traits/SitesTrait.php(1 hunks)playbooks/site-add.sh(1 hunks)playbooks/site-delete.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/SiteAddCommand.phpapp/Console/Server/ServerInstallCommand.phpapp/Traits/SitesTrait.phpapp/Console/Site/SiteDeleteCommand.php
🧬 Code graph analysis (4)
playbooks/site-delete.sh (1)
playbooks/helpers.sh (1)
run_cmd(18-24)
playbooks/site-add.sh (1)
playbooks/helpers.sh (1)
run_cmd(18-24)
app/Console/Site/SiteAddCommand.php (3)
app/Contracts/BaseCommand.php (2)
BaseCommand(30-244)nay(186-190)app/Services/IOService.php (3)
info(474-477)writeln(463-469)getOptionOrPrompt(84-137)app/Traits/PlaybooksTrait.php (1)
executePlaybook(45-187)
app/Console/Site/SiteDeleteCommand.php (3)
app/Repositories/ServerRepository.php (1)
delete(119-133)app/Repositories/SiteRepository.php (1)
delete(133-147)app/Services/InventoryService.php (1)
delete(78-84)
🔇 Additional comments (7)
app/Traits/SitesTrait.php (1)
84-99: Domain option wiring inselectSitelooks consistentUsing
'domain'as the key forgetOptionOrPrompt()and then resolving viafindByDomain()aligns with the new--domainCLI option and keeps the flow coherent across add/delete commands.app/Console/Server/ServerInstallCommand.php (1)
17-20: Command description tweak is safeThe updated
AsCommanddescription is clearer and does not affect behavior; no further changes needed here.app/Console/Site/SiteDeleteCommand.php (2)
7-25: Traits and command metadata are wired correctlyAdding
PlaybooksTraitandServersTraitalongsideSitesTrait, and updating theAsCommanddescription to mention both server and inventory deletion, cleanly reflects the new behavior without altering the base execution pattern.
31-39: Domain-based option aligns with selection flowSwitching the primary option from
--siteto--domainand labelling it “Domain name” matchesSitesTrait::selectSite()(which now looks up sites by domain) and keeps the CLI consistent with the add/delete playbook expectations.app/Console/Site/SiteAddCommand.php (3)
9-26: Trait usage and command metadata integration look correctAdding
PlaybooksTraithere and updatingAsCommandwith a more descriptive description cleanly wires this command into the playbook-based provisioning flow; nothing problematic stands out.
40-42: Newserverandphp-versionoptions are correctly wired for non‑interactive useDefining both as
VALUE_REQUIREDmatches howselectServer()andselectPhpVersion()consume them viaIOService::getOptionOrPrompt, which keeps the command easily scriptable while still supporting interactive prompts.
303-309: gatherSiteInfo() naming and contract are clear and align with usageRenaming to
gatherSiteInfo()with a precise array shape docblock (domain,repo,branch) matches howexecute()destructures the result and keeps the command’s main flow readable. The use ofgetValidatedOptionOrPrompt()for each field ensures both CLI options and interactive use are handled consistently.
1 participant
Summary by CodeRabbit
New Features
Improvements