[EPIC-4-3] Add YAML configuration system for sync-service#24
Conversation
- What: Created YAML-based configuration with 4 sections (vault, api, sync, logging) - Why: Enables flexible deployment configuration without code changes (Issue #23) - How: serde_yaml for deserialization, shellexpand for ~ path expansion Acceptance criteria addressed: - [x] Config struct with VaultConfig, ApiConfig, SyncConfig, LoggingConfig - [x] Config::load() method reads and parses YAML files - [x] Config::vault_path() expands ~ to home directory - [x] config.example.yaml template with inline documentation - [x] .gitignore excludes user-specific config.yaml - [x] README.md with comprehensive configuration guide - [x] Dependencies: serde_yaml (existing), shellexpand 3.1.1 (new) Testing: - Unit tests for config deserialization (4 tests in src/config.rs) - Integration tests for file loading (3 tests in tests/config_test.rs) - Coverage: config.rs 98.92%, overall 84.57% (target: 80%+) Documentation: - README.md: 545 lines covering all config options, ranges, defaults - config.example.yaml: 207 lines with development-oriented defaults - Inline docs: doctests for Config::load() and Config::vault_path() Additional fixes: - Fixed doctest bug in vault/lock.rs (Issue #12 code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive YAML-based configuration system for the sync-service, which is a great step towards flexible deployment. The changes include the configuration module itself, an example configuration file, extensive documentation in the README, and thorough unit and integration tests.
My review has identified a couple of areas for improvement:
- In
src/config.rs, using enums for logging level and format would improve type safety and configuration validation. - The
.gitignorefile should be updated to exclude generated code coverage reports likelcov.info.
Overall, this is a very well-executed feature with excellent documentation and testing. The implementation plan is also very detailed. Great work!
|
|
||
| # Local environment variables | ||
| .env | ||
| .env.local |
There was a problem hiding this comment.
| pub level: String, | ||
| /// Log format: text (human-readable) or json (structured) | ||
| pub format: String, |
There was a problem hiding this comment.
Using String for level and format is flexible but error-prone. A typo in config.yaml (e.g., "inf" instead of "info") would not be caught at deserialization time and could lead to unexpected logging behavior.
To improve robustness and provide immediate feedback on invalid configuration, I recommend using enums for these fields. This allows serde to validate the values when parsing the YAML file.
You can define the enums like this, for example above the LoggingConfig struct:
#[derive(Debug, Clone, Deserialize, Serialize)]
#[serde(rename_all = "lowercase")]
pub enum LogLevel {
Trace,
Debug,
Info,
Warn,
Error,
}
#[derive(Debug, Clone, Deserialize, Serialize)]
#[serde(rename_all = "lowercase")]
pub enum LogFormat {
Text,
Json,
}Then, you can update LoggingConfig to use them.
| pub level: String, | |
| /// Log format: text (human-readable) or json (structured) | |
| pub format: String, | |
| pub level: LogLevel, | |
| /// Log format: text (human-readable) or json (structured) | |
| pub format: LogFormat, |
- Add LogLevel and LogFormat enums for type-safe config (addresses @gemini-code-assist comment) * Validates log level/format at deserialization time * Prevents typos in config.yaml (e.g., 'inf' instead of 'info') * Uses serde(rename_all = lowercase) for YAML compatibility - Add coverage reports to .gitignore (addresses @gemini-code-assist comment) * Exclude lcov.info and *.lcov files * Exclude /target/llvm-cov/ directory * Remove lcov.info from git tracking Testing: - Added test_invalid_log_level() - verifies invalid level rejected - Added test_invalid_log_format() - verifies invalid format rejected - Updated all tests to use enum variants (LogLevel::Info, LogFormat::Text) - All 42 tests pass (40 unit + 2 integration) - Coverage maintained: config.rs still at 98%+ Quality checks: - cargo fmt: ✓ passed - cargo clippy: ✓ passed (no warnings) - cargo check: ✓ passed Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Review Comments AddressedThank you @gemini-code-assist for the thorough review! I've addressed both comments: ✅ Comment 1: Coverage Reports in .gitignoreFixed: Added coverage section to # Coverage reports
lcov.info
*.lcov
/target/llvm-cov/
✅ Comment 2: Type-Safe Enums for LoggingConfigFixed: Replaced New enums: #[derive(Debug, Clone, Deserialize, Serialize, PartialEq)]
#[serde(rename_all = "lowercase")]
pub enum LogLevel {
Trace,
Debug,
Info,
Warn,
Error,
}
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq)]
#[serde(rename_all = "lowercase")]
pub enum LogFormat {
Text,
Json,
}Benefits:
Testing:
Quality Checks:
Commit: a043ba1 Ready for re-review! 🚀 |
|
Thank you, @deserat, for the detailed update and for addressing the feedback from the previous review! I appreciate you taking the time to implement these changes. I'm particularly pleased to see the implementation of type-safe enums ( I've noted the update to the I will now proceed with a re-review of the pull request to verify these changes and ensure everything is in order. Great work! 🚀 |
Summary
Implements YAML-based configuration system for sync-service with four sections: vault (file watching), api (HTTP client), sync (behavior), and logging. Enables flexible deployment configuration without code changes.
Related Issue
Closes #23 (EPIC-4-3)
Changes
src/config.rs- Configuration module with 4 sub-structsconfig.example.yaml- Comprehensive YAML template with inline docs.gitignore- Excludes user-specific config.yamlREADME.md- 545 lines covering all configuration optionstests/config_test.rs- Integration tests for config loadingCargo.toml- Added shellexpand dependency for ~ path expansionsrc/lib.rs- Exposed config modulevault/lock.rs(from Issue Vault Directory Structure and Markdown Utilities #12)Configuration Sections
Testing
Documentation
Verification Steps
Checklist