Fix remote settings overwrite at startup #2707
Conversation
devops-mycroft
added
the
CLA: Yes
|
Voight Kampff Integration Test Succeeded (Results) |
|
this looks good, its a partial solution but ensures the settings updated event means exactly that 2 way sync is still needed however issue clarification:
Out of scope for this PR either way, we could separate local settings from remote settings somehow, but any approach like that is just hacky, the proper fix is selene side |
|
I definitely agree that the two-way sync is what's needed. But in addition to that I think this is needed as well as core shouldn't send settings updated messages to all skills at startup even if they've been synced. |
krisgesling
added
Status: To be reviewed
|
I've confirmed that it works as described:
The code looks straightforward, I can't see what could possibly be wrong with it, and it runs as described. Only thing I can't say is whether it'll break what's underneath that allows it to work. Where's the main change itself? I don't see anything to do with the overwriting loop. |
|
The main change is L353, the settings already only request skills to reload settings if there is a difference. At above mentioned line the copy of the settings used for comparison when identifying changes is restored so the initial download after startup also has a reference. The behavior you describe doesn't sound quite correct, gonna see if I can replicate that. A change on home while the device is turned off should trigger an update, the idea is that if there is no change on home since last fetch but a local change has been made, the local change should not be overwritten as soon as Mycroft is restarted. |
forslund
force-pushed
the
feature/remote-startup-on-change
branch
from
8c8a210
to
ecfd2b2
Compare
|
I haven't been able to replicate your behavior using my test skill, going to try the dismissal skill as well. I did find and remove an unused member variable assignment. |
Mycroft always mangeled any local settings changes at startup. This caches the last settings from home on disk and only updates skills _if_ there is a change. This means that as if a member is changed locally (manually edited or changed by the skill itself) it will be kept until another change is made to the skill on Home.
forslund
force-pushed
the
feature/remote-startup-on-change
branch
from
ecfd2b2
to
bd19d16
Compare
|
Voight Kampff Integration Test Succeeded (Results) |
|
To be honest, I thought the behavior I saw was what you intended. It made sense to me. |
|
Voight Kampff Integration Test Failed (Results). |
|
Hey Chance, thanks for checking it out. I wasn't clear from your first message of whether you tried changing the setting locally on the device? I was thinking along the lines of
Might be even easier to see with something like the News Skill where you can switch between many default stations. Or something with a text input. |
|
Thanks, Gez! I was able to confirm the behavior as described, this works and I'm not sure what happened the first time around. It may simply be that I didn't wait long enough before reporting back, and didn't notice it overwriting on sync. |
|
Voight Kampff Integration Test Succeeded (Results) |
|
Excellent, thanks Chance - if you are happy to submit an approving code review I'll merge it in :) |
Description
This is a suggestion to improve the usability of the local settings, an implementation of an idea mentioned in #2698
Currently Mycroft always mangels any local settings changes at startup. This PR adds a cache of the last settings from home on disk and only updates skills if there is an actual change.
This means that as if a member is changed locally (manually edited or changed by the skill itself) it will be kept until another change is made to the skill on Home.
In my opinion this is a more intuitive way of handling the changes, but it's definitely up for discussion / rejection.
How to test
Ensure that skill updates from home works as normally. Edit a settings.json while Mycroft is turned off and make sure that the change doesn't get overwritten when mycroft starts again.
Contributor license agreement signed?
CLA [ Yes ]