-
Notifications
You must be signed in to change notification settings - Fork 111
core: libs: commonwealth: settings: managers: Deal with invalid settings file #3529
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
core: libs: commonwealth: settings: managers: Deal with invalid settings file #3529
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideBroaden exception handling in PydanticManager and PyksonManager to catch all errors when loading settings files so that any invalid file is skipped and the next candidate is tried. Sequence diagram for settings file loading with broader exception handlingsequenceDiagram
participant Manager
participant SettingsFile
participant Logger
Manager->>SettingsFile: load_from_file(settings_type, valid_file)
alt Success
Manager->>Logger: debug("Using valid_file as settings source")
Manager-->>Manager: return
else Exception
Manager->>Logger: debug("Invalid settings, going to try another file:", exception)
Manager->>SettingsFile: load_from_file(settings_type, next_candidate)
end
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.
Hey there - I've reviewed your changes - here's some feedback:
- Avoid catching all Exceptions here—narrow the except clause to the specific errors you actually expect (e.g. SettingsFromTheFuture) so you don’t accidentally swallow unrelated failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Avoid catching all Exceptions here—narrow the except clause to the specific errors you actually expect (e.g. SettingsFromTheFuture) so you don’t accidentally swallow unrelated failures.
## Individual Comments
### Comment 1
<location> `core/libs/commonwealth/src/commonwealth/settings/managers/pydantic_manager.py:127` </location>
<code_context>
logger.debug(f"Using {valid_file} as settings source")
return
- except SettingsFromTheFuture as exception:
+ except Exception as exception:
logger.debug("Invalid settings, going to try another file:", exception)
</code_context>
<issue_to_address>
Catching Exception may mask unrelated errors.
Catching Exception instead of SettingsFromTheFuture may hide unrelated errors. Consider handling only relevant exceptions, and add logging or re-raise as needed to prevent silent failures.
</issue_to_address>
### Comment 2
<location> `core/libs/commonwealth/src/commonwealth/settings/managers/pykson_manager.py:123` </location>
<code_context>
logger.debug(f"Using {valid_file} as settings source")
return
- except SettingsFromTheFuture as exception:
+ except Exception as exception:
logger.debug("Invalid settings, going to try another file:", exception)
</code_context>
<issue_to_address>
Broad exception handling could obscure root causes.
Consider catching only specific exceptions related to settings, or ensure unexpected exceptions are logged or re-raised to aid debugging.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
logger.debug(f"Using {valid_file} as settings source") | ||
return | ||
except SettingsFromTheFuture as exception: | ||
except Exception as exception: |
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.
issue (bug_risk): Catching Exception may mask unrelated errors.
Catching Exception instead of SettingsFromTheFuture may hide unrelated errors. Consider handling only relevant exceptions, and add logging or re-raise as needed to prevent silent failures.
logger.debug(f"Using {valid_file} as settings source") | ||
return | ||
except SettingsFromTheFuture as exception: | ||
except Exception as exception: |
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.
issue (bug_risk): Broad exception handling could obscure root causes.
Consider catching only specific exceptions related to settings, or ensure unexpected exceptions are logged or re-raised to aid debugging.
491e380
to
b6b3da1
Compare
core/libs/commonwealth/src/commonwealth/settings/managers/pykson_manager.py
Outdated
Show resolved
Hide resolved
core/libs/commonwealth/src/commonwealth/settings/managers/pydantic_manager.py
Outdated
Show resolved
Hide resolved
b6b3da1
to
d74cdc5
Compare
@joaomariolago check now, updates with tests |
e4ae4c7
to
acd4cf0
Compare
…ngs file Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
…valid files Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
acd4cf0
to
f84a344
Compare
To test:
Create a invalid settings-1, run a version that has settings-2, settings-1 is invalid and default settings-2 is created.
Tested with cable-guy