-
-
Notifications
You must be signed in to change notification settings - Fork 41
fix(config.py): fix crashes if settings.yml is not up to date #887
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
Conversation
Reviewer's GuideAuto-merges missing config keys from settings.yml.example to prevent crashes and replaces manual ALLOW_SYSADMINS_EVAL fallback with a direct merged config lookup. Sequence Diagram for Configuration Loading and MergingsequenceDiagram
participant App as Application
participant CPY as "config.py module"
participant UserConfig as "settings.yml (user)"
participant DefaultConfig as "settings.yml.example (default)"
participant LoggerSvc as "Logger"
App->>CPY: Starts (imports config.py)
CPY->>UserConfig: Reads data (config_file.read_text())
UserConfig-->>CPY: Returns user_data
CPY->>DefaultConfig: Reads data (config_file_example.read_text())
DefaultConfig-->>CPY: Returns default_data
CPY->>CPY: Calls merge_defaults(user_data, default_data)
activate CPY
loop For each key, default_value in default_data
alt key not in user_data
Note over CPY: user_data[key] = default_value
CPY->>LoggerSvc: logger.warning(f"Added missing config key: {key}")
else key is in user_data AND user_data[key] is dict AND default_value is dict
CPY->>CPY: merge_defaults(user_data[key], default_value) // Recursive call
end
end
deactivate CPY
Note over CPY: Module-level 'config' dictionary is now merged
App->>CPY: Access Config.ALLOW_SYSADMINS_EVAL
CPY-->>App: Returns value from merged 'config' dictionary
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Pull Request Overview
This PR ensures the application does not crash when settings.yml is missing newly introduced keys by loading defaults from settings.yml.example and auto-adding any missing keys. It also simplifies the ALLOW_SYSADMINS_EVAL lookup by relying on the patched config.
- Load
settings.yml.exampleand compare keys - Log warnings and patch missing keys in memory
- Remove manual defaulting logic for
ALLOW_SYSADMINS_EVAL
Comments suppressed due to low confidence (3)
tux/utils/config.py:39
- [nitpick] Rename
config_exampletoexample_configordefault_configto more clearly indicate its role as the default settings reference.
config_example = yaml.safe_load(config_file_example.read_text())
tux/utils/config.py:41
- [nitpick] Add a comment explaining that this patch only updates the in-memory config and does not write changes back to
settings.yml, or implement persistence if needed.
missing_keys = set(config_example.keys()) - set(config.keys())
tux/utils/config.py:41
- Add unit tests to verify that missing keys from the example config are correctly added to the loaded config at runtime.
missing_keys = set(config_example.keys()) - set(config.keys())
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.
Hey @electron271 - I've reviewed your changes - here's some feedback:
- Consider doing a recursive (deep) merge of
config_exampleintoconfigso that missing nested keys are also handled, not just top-level ones. - Wrap the loading of
settings.yml.examplein a try/except block to avoid introducing a new crash if the example file is missing or malformed. - If you’re automatically adding missing keys, you may want to persist the updated config back to
settings.ymlor clearly document that the fix is in-memory only.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Deploying tux with
|
| Latest commit: |
8fdf5d7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d13cb682.tux-afh.pages.dev |
| Branch Preview URL: | https://config-autogen.tux-afh.pages.dev |
|
@sourcery-ai review |
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.
Hey @electron271 - I've reviewed your changes - here's some feedback:
- Consider lowering the log level (e.g. to debug or info) or batching warnings for added keys to avoid flooding your logs on startup.
- Since merge_defaults mutates the config dict in place and reuses the example values directly, consider deep-copying defaults or moving the merge logic into a dedicated loader function to avoid unintended side effects.
- Wrap the YAML loading in try/except blocks to provide clearer errors if either settings.yml or the example is missing or malformed instead of relying solely on safe_load to raise exceptions.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary by Sourcery
Load and merge default settings from settings.yml.example into the user config to prevent crashes when settings.yml is missing keys and simplify retrieval of ALLOW_SYSADMINS_EVAL.
Bug Fixes:
Enhancements: