[EPIC-4-2] Configuration: Add YAML config loader with environment variable support - #27
Conversation
github-actions
Bot
added
service/api-service
Summary of ChangesHello @deserat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a foundational configuration management system for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-tested configuration loader for the API service, which is an excellent addition. It supports YAML files, .env files, and environment variable overrides, following the 12-factor app methodology. The implementation is solid and the extensive test suite is commendable.
My review focuses on improving robustness and maintainability. I've identified a critical issue where misconfigured environment variables could crash the application on startup, and I've provided a suggestion to handle this gracefully. I also have a couple of medium-severity suggestions to improve logging practices and reduce code repetition for better long-term maintainability.
Overall, this is a well-executed feature, and with a few adjustments, it will be even more robust.
| if pool_size := os.getenv("DB_POOL_SIZE"): | ||
| self._ensure_section("database") | ||
| self._config["database"]["pool_size"] = int(pool_size) |
There was a problem hiding this comment.
The direct conversion of environment variables to integers using int() can lead to an unhandled ValueError if the environment variable is set to a non-integer string (e.g., DB_POOL_SIZE="abc"). This would crash the application on startup with a generic traceback, which is not user-friendly.
It's better to wrap the conversion in a try...except block and raise a ValueError with a more informative message, guiding the user to fix the configuration.
This same issue applies to other integer conversions in this method for DB_MAX_OVERFLOW (line 90), API_PORT (line 127), SURFACING_INTERVAL_SECONDS (line 135), and SCHEDULER_BATCH_SIZE (line 138).
| if pool_size := os.getenv("DB_POOL_SIZE"): | |
| self._ensure_section("database") | |
| self._config["database"]["pool_size"] = int(pool_size) | |
| if pool_size_str := os.getenv("DB_POOL_SIZE"): | |
| self._ensure_section("database") | |
| try: | |
| self._config["database"]["pool_size"] = int(pool_size_str) | |
| except ValueError: | |
| raise ValueError(f"Invalid value for DB_POOL_SIZE: '{pool_size_str}'. Must be an integer.") |
| # Validate configuration | ||
| self._validate() | ||
|
|
||
| def _apply_env_overrides(self) -> None: |
There was a problem hiding this comment.
This method is quite long and contains a lot of repetitive logic for handling each environment variable. This can make it harder to maintain and add new configuration variables in the future.
Consider refactoring this by using a data structure (like a list of tuples or a dictionary) to map environment variables to their corresponding configuration paths and types. This would allow you to loop through the mapping and apply the overrides in a more generic and concise way.
For example:
_ENV_OVERRIDES = {
"DATABASE_URL": ("database.url", str),
"DB_POOL_SIZE": ("database.pool_size", int),
"DB_MAX_OVERFLOW": ("database.max_overflow", int),
"VAULT_PATH": ("vault.path", str),
# ... and so on
}
def _apply_env_overrides(self) -> None:
# ...
for env_var, (path, cast) in _ENV_OVERRIDES.items():
if value := os.getenv(env_var):
# Logic to set value using path and cast
# This would also be a good place to centralize the try/except for casting
...This approach would make the code more declarative and easier to extend.
|
|
||
| if not vault_path.exists(): | ||
| # Create vault directory with warning | ||
| print(f"Warning: Vault path does not exist, creating: {vault_path}") |
There was a problem hiding this comment.
Using print() for warnings is generally discouraged in library or application code, as it writes directly to standard output and cannot be controlled, redirected, or formatted by a logging configuration. It's better to use the logging module. This allows the application that uses this config loader to control the log level, format, and destination (e.g., file, console, remote service).
To implement this, you'll need to:
- Import the
loggingmodule at the top of the file. - Initialize a module-level logger:
logger = logging.getLogger(__name__). - Change this
print()call tologger.warning(...).
Note: This change will require updating test_config_validation_vault_path_created in test_config.py to use the caplog fixture instead of capsys to assert that the warning was logged.
| print(f"Warning: Vault path does not exist, creating: {vault_path}") | |
| logger.warning(f"Vault path does not exist, creating: {vault_path}") |
… support - What: Created Config class with YAML + environment variable support - Why: Foundation for environment-specific configuration (Issue #15) - How: Python-dotenv for .env, PyYAML for config.yaml, validation **Review Fixes (Gemini Code Assist feedback)**: 1. CRITICAL: Fixed unhandled ValueError in integer conversions - Added _parse_int() helper with try/except and helpful error messages - Applied to all 5 integer conversions (DB_POOL_SIZE, DB_MAX_OVERFLOW, API_PORT, SURFACING_INTERVAL_SECONDS, SCHEDULER_BATCH_SIZE) - Added 5 new tests for invalid integer values 2. MEDIUM: Refactored repetitive logic in _apply_env_overrides - Created declarative _ENV_VAR_MAPPINGS structure - Reduced _apply_env_overrides from ~70 lines to ~20 lines - Added _set_nested() helper for dot-notation path setting - Centralized type conversion and error handling 3. MEDIUM: Replaced print() with proper logging - Added logging module and module-level logger - Changed vault path warning to logger.warning() - Updated test to use caplog fixture instead of capsys Acceptance criteria addressed: - [x] Config loads YAML configuration from file - [x] python-dotenv loads .env file - [x] Environment variables override YAML values - [x] Property accessors (database, vault, ai, etc.) - [x] get() method with dot notation - [x] Singleton pattern via load_config() and get_config() - [x] Configuration validation with helpful errors - [x] Robust error handling for invalid values Testing: - Unit tests for all configuration features (26 tests) - Error handling tests for invalid integers (5 tests) - Coverage: 94% (target: 90%+) - All quality checks pass (ruff format, ruff check, pyright) Files created: - app/core/config.py - Configuration loader (331 lines) - config.example.yaml - Example configuration (169 lines) - .env.example - Environment variable documentation (107 lines) - tests/unit/core/test_config.py - Test suite (31 tests, 562 lines) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
5418ca2 to
b1f5180
Compare
|
@gemini-code-assist Thank you for the thorough review! All issues have been addressed: 1. ✅ CRITICAL: Integer conversion error handlingFixed: Added
2. ✅ MEDIUM: Refactored repetitive logicFixed: Created declarative
3. ✅ MEDIUM: Replaced print() with loggingFixed: Added proper logging infrastructure
VerificationAll changes maintain backward compatibility and improve code quality:
The code is now more robust, maintainable, and follows Python best practices. |
|
Thank you, @deserat, for the detailed update and for addressing all the points raised in the review! I appreciate your thoroughness in implementing the suggested changes. It's great to see the improvements:
Your verification steps confirm that the changes are well-tested and maintain high code quality. Excellent work! |
1 participant
Summary
Implements configuration loader for api-service that merges YAML files with environment variable overrides, providing a flexible configuration management system following 12-factor app methodology.
Related Issue
Closes #15 (EPIC-4-2: Configuration Loaders with Environment Variable Support)
Changes
New Configuration System
Configuration Sections
Files Created
api-service/app/core/config.py- Configuration loader implementation (348 lines)api-service/config.example.yaml- Comprehensive YAML template with inline documentation (169 lines)api-service/.env.example- Environment variable reference with usage examples (107 lines)api-service/app/core/__init__.py- Public API exports (Config, load_config, get_config)api-service/tests/unit/core/test_config.py- Complete test suite (496 lines, 26 tests)docs/dev/plans/EPIC-4-2-config-loader.md- Implementation planTesting
API Usage
Environment Variable Override Examples
Validation Features
Configuration validation ensures:
Security Practices
Verification Steps
Checklist
Next Steps
After merge:
🤖 Generated with Claude Code