-
Notifications
You must be signed in to change notification settings - Fork 0
feat: [#3] Setup logging for production CLI with configurable options #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [#3] Setup logging for production CLI with configurable options #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, It looks good. I have added only two things to fix.
Add comprehensive specification for removing ANSI color codes from log files when using compact and pretty formats. Recommends independent format control (--log-file-format, --log-stderr-format) to give users full control over logging behavior. Discovered during PR #4 review. JSON format already works correctly. Related to #2, #3
|
@copilot rebase this branch. |
Rebased the branch onto origin/main. The rebase completed successfully with no conflicts. The commits now include the latest changes from main (commit b5ab209). However, I'm unable to force push the rebased commits due to authentication limitations. The rebased branch is ready locally with commits:
The branch needs to be force-pushed to origin to update the PR. |
Co-authored-by: josecelano <[email protected]>
…ributors Co-authored-by: josecelano <[email protected]>
… gitignore Co-authored-by: josecelano <[email protected]>
…ory from gitignore Co-authored-by: josecelano <[email protected]>
1d1d60c to
94cb2bc
Compare
|
ACK 94cb2bc |
580f24e
into
3-setup-logging-for-production-cli
29c3d8d refactor: [#61] standardize method ordering (Proposal #6) (Jose Celano) 7fbc1e6 refactor: [#61] standardize logging patterns (Proposal #5) (Jose Celano) f4b0eb8 refactor: [#61] remove pub(crate) from build_failure_context methods (Proposal #4) (Jose Celano) 99f86a1 refactor: [#61] add help methods to error types (Proposal #3) (Jose Celano) 1bf684a refactor: [#61] extract step execution result type (Proposal #2) (Jose Celano) fb532eb fix: [#61] prevent unnecessary cleanup when infrastructure already destroyed (Jose Celano) 5f77b57 refactor: [#61] extract typed repository wrapper (Proposal #1) (Jose Celano) c946679 refactor: [#61] extract common failure context builder (Proposal #0) (Jose Celano) Pull request description: ## 🎯 Overview This PR completes a comprehensive refactoring of the command handlers codebase to improve maintainability, reduce code duplication, and standardize patterns across all handlers. Closes #61 ## 📋 Changes Summary ### ✅ Completed Proposals (7/7) **Phase 0 - Quick Wins (High Impact, Low Effort)** - ✅ **Proposal #0**: Common failure context builder - Extracted duplicated `build_failure_context` logic into shared helper - ✅ **Proposal #1**: `TypedEnvironmentRepository` wrapper - Created wrapper for type-safe environment state persistence - ✅ **Proposal #2**: `StepResult` type alias - Simplified step execution return types - ✅ **Proposal #3**: Error help methods - Added `.help()` methods to all error types for detailed troubleshooting **Phase 1 - Structural Improvements (High Impact, Medium Effort)** - ✅ **Proposal #4**: Remove `pub(crate)` test exposure - Made internal methods private, deleted tests that tested implementation details **Phase 2 - Consistency & Polish (Medium Impact, Low Effort)** - ✅ **Proposal #5**: Consistent logging patterns - Standardized field naming, simplified logging, added comprehensive documentation - ✅ **Proposal #6**: Standardize method ordering - Reordered methods to follow module organization conventions ## 🔧 Key Improvements ### Code Quality - ✅ Eliminated code duplication in failure context building - ✅ Improved type safety with `TypedEnvironmentRepository` - ✅ Simplified step result types across all handlers - ✅ Consistent method ordering following project conventions ### Developer Experience - ✅ Enhanced error messages with actionable `.help()` methods - ✅ Standardized logging patterns with comprehensive documentation - ✅ All internal implementation details properly encapsulated - ✅ Clear separation between public API and private helpers ### Documentation - ✅ Added "Command Handler Logging Patterns" section to logging guide - ✅ Documented standard patterns with examples for all handlers - ✅ Documented anti-patterns to avoid - ✅ Updated refactoring plan tracking all completed proposals ## 🧪 Testing All quality gates passed: - ✅ **1002 unit tests** - All passing - ✅ **203 integration tests** (doc tests) - All passing - ✅ **45 E2E tests** across all test suites - All passing - ✅ **All linters** passing (markdown, yaml, toml, cspell, clippy, rustfmt, shellcheck) - ✅ **Documentation builds** successfully - ✅ **Zero behavioral changes** - Pure refactoring ### Test Coverage - Unit tests: 1002 passing - Integration tests (AI enforcement): 5 passing - E2E create command: 4 passing - E2E destroy command: 4 passing - Multiprocess file lock: 8 passing - Logging integration: 11 passing - SSH client integration: 9 passing - Template integration: 4 passing ## 📊 Impact ### Before - Duplicated `build_failure_context` across 3 handlers (~90% identical code) - Inconsistent logging patterns (different field names, varying verbosity) - Internal methods exposed as `pub(crate)` for testing - Methods ordered inconsistently across handlers ### After - Single shared `build_failure_context` helper - Consistent logging with standardized field naming (`environment` not `environment_name`) - Proper encapsulation - internal methods are private - Predictable method ordering following module organization conventions ## 🔄 Migration Notes No breaking changes - this is a pure refactoring. All public APIs remain unchanged. ## ✅ Pre-commit Verification ```bash ./scripts/pre-commit.sh ``` **Result**: All checks passed (4m 48s) - ✅ Dependency check (cargo machete) - ✅ All linters (stable & nightly) - ✅ All tests - ✅ Documentation build - ✅ Full E2E test suite ## 📝 Commits 1. `refactor: [#61] extract common failure context builder (Proposal #0)` 2. `refactor: [#61] add TypedEnvironmentRepository wrapper (Proposal #1)` 3. `refactor: [#61] add StepResult type alias (Proposal #2)` 4. `refactor: [#61] add error help methods (Proposal #3)` 5. `refactor: [#61] remove pub(crate) test exposure (Proposal #4)` 6. `refactor: [#61] standardize logging patterns (Proposal #5)` 7. `refactor: [#61] standardize method ordering (Proposal #6)` ## 🎉 Summary This refactoring successfully: - Reduces code duplication - Improves maintainability - Enhances developer experience - Standardizes patterns across all handlers - Maintains 100% backward compatibility Ready for review and merge! 🚀 ACKs for top commit: josecelano: ACK 29c3d8d Tree-SHA512: fa360abfb7519178c4a30e20d61b976dd737fadc15aa4a31d2a801702d9f9d1c1908095c58f4da0d72ef56d7544fdc86ceb67c02a2561eb77ab804764f1cc117
… VMs The 'Allow SSH service by name' task was using a non-existent UFW application profile 'ssh' which caused the playbook to fail before enabling the firewall. This task was redundant because SSH is already explicitly allowed by port number in the previous task. Fixes Issue #4 identified in PR review - UFW fails with 'Could not find a profile matching ssh'
Setup Logging for Production CLI ✅
Complete implementation of CLI logging initialization for the main application binary.
Implementation Checklist - ALL COMPLETE ✅
Phase 1: Basic CLI Structure ✅
src/app.rsmodule with CLI struct and run() functionsrc/main.rsto call app::run()Phase 2: Argument Validation ✅
Phase 3: Documentation ✅
docs/user-guide/logging.mdfor end usersdocs/contributing/logging-guide.mdfor contributorsPhase 4: Integration Testing ✅
Phase 5: Final Validation ✅
cargo run --bin linter all)Summary of Changes
Code Changes
Created
src/app.rs- Main application module with CLI structure:--log-format: pretty, json, or compact (default: compact)--log-output: file-only or file-and-stderr (default: file-only)--log-dir: path to log directory (default: ./data/logs)Updated
src/main.rs- Minimal entry point:app::run()Code Quality:
Documentation Changes
README.md - Simplified logging section:
docs/user-guide/logging.md- Complete user guide (380+ lines):docs/contributing/logging-guide.md- Updated contributor guide:.gitignore- Cleaned up:custom-logs/)Testing Results
Linting ✅
Manual Testing ✅
Default Configuration:
Format Options:
Output Modes:
Custom Log Directories:
RUST_LOG Environment Variable:
Log File Persistence:
Error Handling:
Help and Version:
Acceptance Criteria - ALL MET ✅
Functional Requirements ✅
--log-format,--log-output,--log-dir)RUST_LOGenvironment variable is respectedCode Quality ✅
cargo run --bin linter all)Documentation ✅
docs/user-guide/logging.mddocs/contributing/logging-guide.md--help) is clear and comprehensiveUser Experience ✅
Key Features
Production-Safe Defaults
./data/logs/log.txtFlexible Configuration
Observability
Developer Experience
Related Documentation
Issue: #3 - Setup logging for production CLI
Estimated Effort: 4-5 hours
Actual Effort: ~4 hours
Status: ✅ Complete - All phases finished, all tests passing, fully documented
Original prompt
Fixes #3
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.