-
Notifications
You must be signed in to change notification settings - Fork 20
Update the logging config actor configuration format #1204
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
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 updates the logging configuration to be compatible with the UI‐generated configuration by requiring the logger name to be specified explicitly, and by separating the root logger configuration from individual logger configurations.
- Introduces a new RootLoggerConfig type for root logger settings
- Updates tests and configuration parsing to use explicit logger names
- Adjusts logging configuration actor to use the new naming scheme
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/config/test_manager.py | Adds explicit logger name in configuration tests |
| tests/config/test_logging_actor.py | Updates logger configuration tests to match the new format |
| src/frequenz/sdk/config/_logging_actor.py | Introduces RootLoggerConfig and updates logging configuration logic |
| src/frequenz/sdk/config/init.py | Updates exports to expose the new RootLoggerConfig |
| mkdocs.yml | Updates documentation formatting options |
| RELEASE_NOTES.md | Documents the logging config changes |
Comments suppressed due to low confidence (1)
src/frequenz/sdk/config/_logging_actor.py:28
- Consider decorating RootLoggerConfig with @DataClass (or an equivalent construct) for consistency with LoggerConfig and to ensure automatic generation of methods such as init.
class RootLoggerConfig:
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.
Optional release notes fix, looks good otherwise.
RELEASE_NOTES.md
Outdated
| level = "DEBUG" | ||
| ``` | ||
|
|
||
| - Now: |
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.
Not aligned with the previous - Before: I guess?
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.
Fixed
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.
Nice!
And it is harder to forget about quote, now :)
This configuration was at the wrong level. Signed-off-by: Leandro Lucarella <[email protected]>
The logging configuration was changed to make it compatible with the configuration generated by the UI, which doesn't quote sub-key properly. To do this, the logger name needs to be specified explicitly now (the configuration key was used as the name before). Signed-off-by: Leandro Lucarella <[email protected]>
|
Fixed release notes indentation issue and rebased to fix conflicts. |
| level = "DEBUG" | ||
| ``` | ||
|
|
||
| - Now: |
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.
Still somehow GitHub gets confused about this...
The logging configuration was changed to make it compatible with the configuration generated by the UI, which doesn't quote sub-key properly.
To do this, the logger name needs to be specified explicitly now (the configuration key was used as the name before).