-
Notifications
You must be signed in to change notification settings - Fork 9
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
change the config merge/load order #27
Conversation
see MycroftAI/mycroft-core#2861 not sure how this bug made it over here as it was extensively discussed an unwanted to make remote > system |
Codecov Report
@@ Coverage Diff @@
## dev #27 +/- ##
=====================================
Coverage ? 0.00%
=====================================
Files ? 9
Lines ? 547
Branches ? 0
=====================================
Hits ? 0
Misses ? 547
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
additionally, the former test masked the problem |
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.
Looks good to me and tested to work under a recent Neon image. Unit test failures are related to a setuptools
vulnerability and out of scope of this PR
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.
Looks right, I remember the original discussion, I concur it’s needed and Daniel has manually tested.
This changes the load/merge order to:
USER > SYSTEM > REMOTE > DEFAULT