-
Notifications
You must be signed in to change notification settings - Fork 94
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
glbl_cfg: reload without mutating the original #6249
Conversation
5e0aa62
to
93bf977
Compare
* Use the new `reload` kwarg rather than calling the `.load()` method. * Fixes cylc#6244 * The `.load()` method mutates the existing config, due to the use of logging within (and outside of) this routine and the use of `glbl_cfg` in the logging, this created a race condition. * The new `reload` kwarg creates a new config instance, then sets this as the default.
"""Return the platforms defined in the provided config instance.""" | ||
return {p for p in cfg_obj.get(['platforms']).keys()} | ||
|
||
def expect_platforms_during_reload(platforms): |
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.
👍🏼 I enjoy learning about stuff when I review and this is rather clever.
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.
- Happy with code
- Tried locally
- Test fails against 8.3.x (and not just because it has an unrecognized keyword arg)
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.
LGTM!
(Two approvals, but @MetRonnie has an unresolved comment)
Sibling: cylc/cylc-rose#336
reload
kwarg rather than calling the.load()
method..load()
method mutates the existing config, due to the use of logging within (and outside of) this routine and the use ofglbl_cfg
in the logging, this created a race condition.reload
kwarg creates a new config instance, then sets this as the default.To assert that the test captures the issue, try applying this diff to test the old reload method:
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).?.?.x
branch.