feat: config validation hardening - #210
Merged
Merged
Conversation
WalkthroughThe change adds configuration validation before startup, warning accessors for model and profile loading, runtime logging format and output resolution, and documentation for these behaviours. ChangesConfiguration behaviour
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Validation
participant ConfigLoaders
participant Report
CLI->>Validation: run --validate-config
Validation->>ConfigLoaders: load main, model, and profile configuration
ConfigLoaders-->>Validation: configuration results and warnings
Validation->>Report: build validation report
Report-->>CLI: print report and return exit code
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/content/configuration/overview.md`:
- Line 379: Update the logging configuration documentation in
docs/content/configuration/overview.md at lines 379-379 and
docs/content/configuration/reference.md at lines 787-787 to distinguish
bootstrap logging from runtime logging: state that OLLA_FILE_OUTPUT and related
file-rotation variables configure bootstrap logger output, while logging.output:
"file" configures runtime logging alongside stdout and has no
environment-variable equivalent, being YAML-only.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cdbe6ca5-2fec-4751-915f-588ea70d3017
📒 Files selected for processing (12)
docs/content/concepts/model-unification.mddocs/content/configuration/overview.mddocs/content/configuration/reference.mddocs/content/getting-started/installation.mdinternal/adapter/registry/profile/factory.gointernal/adapter/registry/profile/loader.gointernal/adapter/registry/profile/loader_test.gointernal/adapter/unifier/model_config.gomain.gomain_test.govalidate.govalidate_test.go
| !!! warning "TTY always wins for format" | ||
| On an interactive terminal, Olla always prints pretty/text logs, no matter what `logging.format` says in the config file - so `format: "json"` in `config.yaml` (typically set for Docker/production) won't change what you see running `olla` directly in a terminal. Off a TTY (piped, Docker, a service), the configured format is used as written. Set the `OLLA_LOGGING_FORMAT` environment variable to force a format everywhere, TTY or not. | ||
|
|
||
| `output: "file"` adds a rotating file handler alongside stdout logging - it doesn't replace stdout. There's no environment variable for `output`; it's YAML-only. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked docs files:"
git ls-files 'docs/**' | sed -n '1,120p'
echo
echo "Search for OLLA_FILE_OUTPUT and logging output mentions:"
rg -n "OLLA_(FILE_OUTPUT|LOGGING_OUTPUT)|logging\.output|output: \"file\"|file handler|rotat" docs/content/configuration || true
echo
echo "Relevant sections in overview/reference (line ranges):"
for f in docs/content/configuration/overview.md docs/content/configuration/reference.md docs/content/configuration/environment-variables.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
fi
doneRepository: thushan/olla
Length of output: 6565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Reference relevant excerpt:"
sed -n '760,845p' docs/content/configuration/reference.md
echo
echo "Monitoring bootstrap/runtime relevant excerpt:"
sed -n '170,225p' docs/content/configuration/practices/monitoring.md
echo
echo "Python text extractor summary for logging env variable mentions:"
python3 - <<'PY'
from pathlib import Path
for p in sorted(Path('docs/content').rglob('*.md')):
text = p.read_text()
if 'logging.output' in text or 'OLLA_FILE_OUTPUT' in text or 'OLLA_LOGGING_OUTPUT' in text:
lines = text.splitlines()
print(f'--- {p} ---')
for i, line in enumerate(lines, 1):
if 'logging.output' in line or 'OLLA_FILE_OUTPUT' in line or 'OLLA_LOGGING_OUTPUT' in line:
print(f'{i}: {line}')
PYRepository: thushan/olla
Length of output: 6810
Clarify the bootstrap/runtime logging output scope.
OLLA_FILE_OUTPUT is read before YAML config loads and enables rotating bootstrap logger output, while logging.output: "file" controls runtime logging. Scope the current "no environment variable for output" and YAML-only statements to the runtime config-layer logger, and keep the bootstrap file-rotation variables separate. Do the same for docs/content/configuration/overview.md and docs/content/configuration/reference.md.
📍 Affects 2 files
docs/content/configuration/overview.md#L379-L379(this comment)docs/content/configuration/reference.md#L787-L787
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/content/configuration/overview.md` at line 379, Update the logging
configuration documentation in docs/content/configuration/overview.md at lines
379-379 and docs/content/configuration/reference.md at lines 787-787 to
distinguish bootstrap logging from runtime logging: state that OLLA_FILE_OUTPUT
and related file-rotation variables configure bootstrap logger output, while
logging.output: "file" configures runtime logging alongside stdout and has no
environment-variable equivalent, being YAML-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improves the config handling and hardens some of the formatting woes and adds a validator flag to ensure configs are valid via
--validate-config.Summary by CodeRabbit
--validate-configto check configuration and provider profiles without starting services.