skill settings sync flag

[JarbasAl]

adds a flag to .conf that allows disabling skill settings sync

some users just dont want/like it
why do i dislike skill settings?

• the websettings overwrite my local changes (breaks a few of my most important skills) related Fix remote settings overwrite at startup #2707 and Feature/Skill-Settings-In-GUI Revised API #2698
• sync to all devices, messes up stuff like my Voip skill since i cant set up different accounts per device
• privacy reasons, once the 2way sync is reimplemented stuff like creds (changed on device) might get uploaded to you, in this case creds shouldnt even be in settingsmeta, but we dont control what others dev do and forking should not be a requirement for privacy

while this stuff isnt fixed/implemented i essentially stopped using websettings at all. not an issue for me, but i would like other people to be able to use my skills.....

this was proposed before and further discussed in #2633

default flag is set to True for compatibility reasons, but IMHO it should be false by default since this is a privacy feature, i made the code default to False but the .conf defaults to True. My reasoning here is that disabled is the sane default and enabling it is up to the enclosure and not core

[devops-mycroft]

Voight Kampff Integration Test Failed (Results)

[devops-mycroft]

Voight Kampff Integration Test Failed (Results)

[krisgesling]

Hey Jarbas,

This seems like a good change to provide the option of not syncing settings if users want to handle everything locally.

However I think the three dot points you raised probably need three separate responses.

1. Overwriting local changes. It seems like the "proper solution" is 2-way sync, and in the meantime would PR Fix remote settings overwrite at startup #2707 handle most cases?

2. Different settings for different devices. This seems like a big enough issue on it's own, so I've started a thread on the forums to dig into that more specifically as I think it's a pretty necessary feature. With only this change anyone wanting to use something like the VOIP Skill would need to manually manage all their Skill settings from then on. We could add another list of Skills to sync or not to the config, but this seems like extra effort for a work around rather than fixing the real issue.

3. If some bit of data shouldn't get synced then IMO it really shouldn't be in the Skills settings. I'd rather not have sensitive data in an object that may be sent off device. Should we instead be encouraging devs to use a separate config eg something stored in the self.file_system directory?
   The other piece here is that we need to improve our oauth registration process so that Skills can authenticate with 3rd party services without passing credentials around. Currently setting up oauth workflows is done manually which limits the ability for devs to make use of it.

I'm in two minds about the defaults too. Conceptually I like what you have, and any devices should have a default mycroft.conf to pull from. However I'm also wary of something that could cause a big spike in support requests saying that "skill settings aren't syncing". IMO defaulting both to True seems like a safer option and still allows any project or individual to disable the sync if they want to.

[forslund]

Number 2 in that list was planned when Selene was first launched and the redesign of the skill settings handling started. @chrisveilleux may have some old spec for what was intended or if we just discussed it in general terms.

Number 3 was prepared in the oauth service, basically requires a web interface and a minor fix in the db setup (broke in the move to selene) to get working.

There was also a prototype for a more simple credentials gateway for simpler api keys that @MatthewScholefield created if I recall correctly (like the one in the cocktails skill) which is something that should be implemented as well imo.

[MatthewScholefield]

There was also a prototype for a more simple credentials gateway for simpler api keys that @MatthewScholefield created if I recall correctly (like the one in the cocktails skill) which is something that should be implemented as well imo.

Not sure if this is what you're referring to but I had a prototype of a system for allowing skill developers to submit their apikeys for other users to use securely by using the backend as a proxy. It worked by wrapping urllib and requests to reroute specific domains to the backend and the backend would replace a fake api key with the real api key and perform the proxied request.

Client code here and example usage here

[JarbasAl]

@krisgesling this PR solves 2 things:

• user freedom of choice / privacy enhancement
• improve automated tests by ensuring settings are not overwritten

the other 3 points are different issues like you pointed out, but i have been complaining about this since i released the voip skill and there is no fix, it is a selene thing so i cant fix it myself either, what this does is allow us to workaround the issue, but it fixes None of those

1 - 2 way sync is the proper solution, and was there before, this was removed in the migration to selene and i don't think it was on purpose, i answered in that PR why it does not fix the issue, but it makes it slightly more manageable
2 - nice thread, you thought of some things that didnt cross my mind, i think you need sane defaults but all those use cases are valid and should be allowed, a sane option is the one that doesnt break stuff or leak data, it currently breaks quite some stuff, voip is an obvious one, but even the datetime skill has problems because if i want my mark1 to show time then it totally messes up my idle screen in the mark2, but let's keep this discussion in that forum post
3 - some data shouldnt be synced, but you dont control what other skills devs do, if i install a email skill and change the settings locally i dont expect them to be sent to you, but now i need to manually inspect every skill and fork it so they arent accidentally uploaded? this is one of the things you could check before accepting skills in the marketplace in case you come up with a alternative solution

i dont think the team will prioritize any of this, so this PR for me is "urgent" since at least i can workaround the current limitations, but this PR has value by itself and those 3 issues dont really have anything to do with it, they just provide context and are their own issues

[chrisveilleux]

I can understand your frustration with these issues not being addressed. Our development bandwidth is minimal right now and there are certain priority projects we need to complete.

1. Selene was intended to have all the funtionality of the previous backend, Tartarus. So if two-way sync was available before it should also be available now. We have a ticket to address this.

2. Selene was designed to sync settings across all devices initially. This was how it worked before Selene was built. Support for settings control on a more granular basis is on the Selene roadmap but some thought needs to be put into it. There is actually some scaffolding in place to support device groups but that fell out of scope.

3. Also on our roadmap is a way to obfuscate any personal data contained within settings stored in Selene. Privacy and security are always primary concerns but are not the only concerns.

I like the idea of allowing users to turn settings synchronization off if they want to manage all settings on device. I disagree that the default should be "false". As more devices make their ways into the hands of consumers, there will be very few who will want to (or even be able to) mange their settings on device. Maybe we could add the ability to set this option to the inital Picroft bootup? I do like the idea of exposing more of the configs for core and enclosures in the Selene UI for people who want more control, but may not be comfortable opening an SSH session into their device.

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[krisgesling]

Looks good, I'll squash the commits.

Also Travis will be disregarded. It won't let me retrigger the build and it's soon to be obsolete.

Comments addressed

[krisgesling]

I was wrong about Travis, need to work out why the default config isn't been used in the unit tests - unless this is intended behaviour?

[forslund]

I wonder if https://github.com/MycroftAI/mycroft-core/blob/dev/test/unittests/mycroft.conf is causing issues? or if there is some stray config mock not being cleared correctly somewhere.

Edit: No it's likely the base_mock it only contain the skills section

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[krisgesling]

Is it worth us adding the entire default config? Tests can then override values as needed but the tests would ensure that the default installation would work as described.

[forslund]

Yes, but I want to do that in a separate PR.

Currently the config is mocked in many different ways all in the various tests and I would like to pull them to together into a more coherent structure.

[forslund]

Did go ahead and make the mock_config generate a complete configuration, will revisit after this one is merged to use it more across the unittests.

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[devops-mycroft]

Voight Kampff Integration Test Failed (Results)

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)