refactor: playbooks file output - #63
Conversation
Playbooks now write YAML output to DEPLOYER_OUTPUT_FILE instead of stdout, allowing progress messages to be displayed separately from structured data. This improves UX by showing real-time progress while keeping YAML parsing reliable. Changes: - PlaybookHelpersTrait generates temp file and passes as env var - Display playbook stdout as progress messages - Read YAML from file after execution and clean up - Update server-info.sh to write to DEPLOYER_OUTPUT_FILE - Refactor server-info.sh to use main() function pattern - Add error checking for file write operations
Document the new playbook pattern where YAML output is written to DEPLOYER_OUTPUT_FILE instead of stdout. This allows progress messages to be displayed separately from structured data. Updates: - Add Structure section with main() function pattern - Document DEPLOYER_OUTPUT_FILE environment variable - Update Output section with file-based pattern - Add error handling patterns for file writes - Update complete example to match new pattern
WalkthroughThe PR restructures playbook guidelines, changes playbook output to write parsable YAML to a designated remote file, updates the trait to read/parse that remote YAML tempfile, and modifies the server-info playbook to validate environment, improve permission handling, rename a function, and write progress + YAML to DEPLOYER_OUTPUT_FILE. A cosmetic blank line was added to server info command output. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as ServerInfoCommand
participant Trait as PlaybookHelpersTrait
participant Remote as Remote Host
participant Script as server-info.sh
User->>CLI: request server info
CLI->>Trait: executePlaybook(playbook, vars)
activate Trait
Trait->>Trait: create remote temp file path (DEPLOYER_OUTPUT_FILE)
Trait->>Remote: ssh/run playbook with DEPLOYER_OUTPUT_FILE + env
activate Remote
Remote->>Script: start server-info.sh
activate Script
Script->>Script: validate DEPLOYER_OUTPUT_FILE
Script->>Script: detect distro & permissions
Script->>Script: discover services (get_listening_services)
Script->>Remote: write YAML lines to DEPLOYER_OUTPUT_FILE (with checks)
Script->>Remote: emit progress to stdout/stderr
Script-->>Remote: exit with code
deactivate Script
Remote->>Trait: stream stdout/stderr lines + exit code
deactivate Remote
Trait->>Remote: cat DEPLOYER_OUTPUT_FILE
activate Remote
Remote->>Trait: return YAML content
deactivate Remote
Trait->>Remote: rm DEPLOYER_OUTPUT_FILE
Trait->>Trait: parse YAML content
Trait->>CLI: return parsed array or failure
deactivate Trait
CLI->>User: print a blank line and formatted info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 0
🧹 Nitpick comments (1)
playbooks/server-info.sh (1)
68-70: Keep sudo non-interactive and single-shot
run_cmdstill relies on the[[ … ]] && "$@" || sudo "$@"pattern. When a root-level call fails we end up invokingsudoanyway, and without-nthe command can block waiting for a password if a PTY gets allocated or the sudoers policy differs from the initial probe. That violates the “never prompt” rule and makes failures harder to diagnose. Please branch explicitly and forcesudo -nso we fail fast while remaining non-interactive.run_cmd() { - [[ $DEPLOYER_PERMS == 'root' ]] && "$@" || sudo "$@" + if [[ $DEPLOYER_PERMS == 'root' ]]; then + "$@" + else + sudo -n "$@" + fi }As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.cursor/rules/06-playbooks.mdc(3 hunks)app/Console/Server/ServerInfoCommand.php(1 hunks)app/Traits/PlaybookHelpersTrait.php(3 hunks)playbooks/server-info.sh(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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
**/*.php: Adhere to PSR-12, enable strict_types, and leverage PHP 8.x features (union types, match, attributes, readonly)
Use explicit return types, including generic-like annotations where applicable (e.g., Collection<int, User>)
Prefer Symfony components (e.g., Filesystem, Process) over native PHP functions for testability
Always import classes via use statements; only use root FQDNs for core exceptions (e.g., \InvalidArgumentException, \RuntimeException). Do not use inline FQDNs for non-root namespaces
Create objects via $container->build(ClassName::class) everywhere except DTOs, value objects, and pure data structures
Use minimalist DocBlocks documenting description, parameters, and return types for classes and functions
Follow the specified comment structure with section headers/subheaders and spacing; remove obsolete comments with removed code
Files:
app/Console/Server/ServerInfoCommand.phpapp/Traits/PlaybookHelpersTrait.php
**/*Command.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
**/*Command.php: Commands handle user interaction (I/O), orchestration, styling, error formatting, and prompts using SymfonyStyle
Commands must not contain business logic; delegate to Services
Commands must not invoke other commands (no proxy commands)
Files:
app/Console/Server/ServerInfoCommand.php
**/*{Command,Service}.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
All dependencies should be expressed in constructor signatures; avoid circular dependencies
Files:
app/Console/Server/ServerInfoCommand.php
🧠 Learnings (3)
📚 Learning: 2025-10-24T19:59:22.873Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-10-24T19:59:22.873Z
Learning: Applies to **/*Command.php : Commands handle user interaction (I/O), orchestration, styling, error formatting, and prompts using SymfonyStyle
Applied to files:
app/Traits/PlaybookHelpersTrait.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} : Target rule density per file: Project-specific (Playbooks) 100–140 lines
Applied to files:
.cursor/rules/06-playbooks.mdc
📚 Learning: 2025-10-24T19:59:22.873Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-10-24T19:59:22.873Z
Learning: Applies to bin/deployer : Application entry point is bin/deployer
Applied to files:
.cursor/rules/06-playbooks.mdc
🧬 Code graph analysis (2)
app/Console/Server/ServerInfoCommand.php (2)
app/Services/IOService.php (1)
writeln(463-469)app/Traits/ServerInfoTrait.php (1)
displayServerInfo(40-80)
app/Traits/PlaybookHelpersTrait.php (2)
app/Services/IOService.php (3)
writeln(463-469)error(498-501)promptSpin(439-452)app/Services/SSHService.php (1)
executeCommand(69-86)
🔇 Additional comments (2)
app/Console/Server/ServerInfoCommand.php (1)
72-73: Output spacing reads betterThe extra blank line keeps the server info section visually separated from the preceding status checks. Nicely done.
app/Traits/PlaybookHelpersTrait.php (1)
104-147: Robust remote YAML retrievalAppreciate how the new block wraps the remote cat/remove inside a spinner, checks for empty payloads, and surfaces parse errors with the raw YAML. This makes diagnosing playbook failures much easier.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
playbooks/server-info.sh (2)
90-90: Remove redundant DEBIAN_FRONTEND export.Line 16 already exports DEBIAN_FRONTEND=noninteractive globally. The re-export at line 90 is unnecessary and adds noise.
case $distro in debian) - export DEBIAN_FRONTEND=noninteractive run_cmd apt-get update -q 2> /dev/null
109-115: Clarify the intent of the 'else' branches in ss/netstat command construction.The 'else' branches (lines 114–115, 138–139) fallback to running ss/netstat without sudo when DEPLOYER_PERMS is neither 'root' nor 'sudo'. However, due to the early return at line 84 when perms=='none', these branches should theoretically never execute in normal flow. If this is intentional as a safety fallback, a comment would improve clarity.
if [[ $DEPLOYER_PERMS == 'root' ]]; then cmd='ss' elif [[ $DEPLOYER_PERMS == 'sudo' ]]; then cmd='sudo ss' else + # Fallback: attempt without sudo (limited output but non-blocking) cmd='ss' fiAlso applies to: 133-139
.cursor/rules/06-playbooks.mdc (1)
177-191: Helper functions section documents the standard run_cmd() pattern, but lacks guard for 'none' case.The documentation correctly explains the run_cmd() helper and -n flag behavior. However, both documentation and actual implementation (server-info.sh line 69-74) leave the DEPLOYER_PERMS='none' case implicit. Consider adding an explicit guard as mentioned in the server-info.sh review.
To align documentation with defensive coding:
run_cmd() { + if [[ $DEPLOYER_PERMS == 'none' ]]; then + echo "Error: run_cmd called without elevated permissions" >&2 + exit 1 + fi if [[ $DEPLOYER_PERMS == 'root' ]]; then "$@" else - sudo -n "$@" + sudo -n "$@" # -n ensures non-interactive operation fi }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.cursor/rules/06-playbooks.mdc(3 hunks)playbooks/server-info.sh(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T19:59:22.873Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-10-24T19:59:22.873Z
Learning: Applies to bin/deployer : Application entry point is bin/deployer
Applied to files:
.cursor/rules/06-playbooks.mdc
🔇 Additional comments (9)
playbooks/server-info.sh (4)
179-202: Robust YAML output flow with proper error handling.The output sequence correctly writes the YAML header, appends service lines with per-write error checks, and handles the empty services case with an explicit placeholder. The pattern prevents partial/corrupt output and aligns with documentation guidelines.
105-105: Function rename is internally consistent.The rename from get_listening_ports() to get_listening_services() is applied at both definition (line 105) and call site (line 195). The new name better reflects its actual behavior of discovering listening services, not just ports.
Also applies to: 195-195
167-173: Progress messages are clear and consistent with guidelines.The use of checkmark indicators and ellipsis syntax aligns with the documentation patterns and provides good user feedback during script execution.
69-74: ****The code is not fragile. The early return guard in
ensure_tools()at line 84 ([[ $perms == 'none' ]] && return 0) is sufficient and properly structured. This guard exits the function before the case statement that contains allrun_cmd()invocations (lines 91–92, 95–96). Sincerun_cmd()is only called withinensure_tools()and after this guard, it is structurally guaranteed to never execute whenDEPLOYER_PERMS='none'. The suggested defensive check inrun_cmd()is unnecessary and would obscure the actual control flow logic.Likely an incorrect or invalid review comment.
.cursor/rules/06-playbooks.mdc (5)
20-66: Structure section is well-defined with clear template pattern.The standardized playbook structure using main() function, mandatory set -o pipefail, DEBIAN_FRONTEND export, and DEPLOYER_OUTPUT_FILE validation provides a solid foundation. The template clearly shows YAML output with error handling.
67-77: Environment variables guidance correctly distinguishes detection vs. provisioning playbooks.The documentation accurately separates validation concerns: detection playbooks (like server-info.sh) validate only DEPLOYER_OUTPUT_FILE, while provisioning playbooks validate DEPLOYER_DISTRO and DEPLOYER_PERMS. This aligns with the actual server-info.sh implementation.
121-142: Idempotency guidance is clear with practical examples of correct and incorrect patterns.The CORRECT examples (existence checks before resource creation/modification) and WRONG examples (useradd duplication, file append duplication) provide clear guardrails. server-info.sh's ensure_tools() appropriately uses
command -vchecks before installation (line 85).
144-175: Error handling section provides clear guidance on validation vs. runtime errors, and emphasizes file write checks.The distinction between stdout (validation errors) and stderr (runtime errors), combined with the set -o pipefail pattern and explicit YAML write checks, is well-explained and properly implemented in server-info.sh.
227-299: Complete example is comprehensive and properly templates all documented patterns.The example integrates validation, helper functions, distro-specific branching, idempotency checks, and proper YAML output with error handling. The reference to
playbooks/server-info.shgrounds the documentation in actual implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
playbooks/server-info.sh (1)
162-186: YAML format will parse correctly—consider inline format for clarity.The format
ports:\n {}is valid YAML and will parse correctly with Symfony's Yaml parser because the explicit{}syntax is unambiguous. Symfony Yaml Component recognizes explicit empty mapping{}and parses it as an empty array.However, the inline format
ports: {}is the recommended approach per Symfony documentation for clarity and consistency. Consider standardizing toports: {}(single line, line 165) for better readability and adherence to conventional YAML formatting practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
playbooks/server-info.sh(7 hunks)
🔇 Additional comments (3)
playbooks/server-info.sh (3)
1-22: Header, environment setup, and validation are solid. Environment variable validation before use prevents silent failures downstream. The documentation clearly specifies the YAML output contract.
68-98: run_cmd() and ensure_tools() logic is correct. DEPLOYER_PERMS is set before early returns, and the script's execution order (line 157 calls ensure_tools() before line 178 calls get_listening_services()) ensures the variable is always initialized before use.
104-178: get_listening_services() function and service output logic look correct. Process substitution, sort/uniq deduplication, and per-service error checking are all sound. Regex patterns for parsing ss/netstat output are appropriate.
1 participant
Summary by CodeRabbit
Bug Fixes
Style
Documentation