mycroft-core icon indicating copy to clipboard operation
mycroft-core copied to clipboard

Discussion MycroftSkill parameter use_settings to be or not to be

Open forslund opened this issue 3 years ago • 4 comments

The use_settings parameter for the MycroftSkill was a bit of a hack when we added it. Basically it was because Padatious ran as a Skill and we didn't want it to show up on the skill page.

Padatious is now a proper service on the same level(ish) as Adapt, and the need for the original hack is not there any more.

I don't think the feature is of much use at the moment since it disables all use of settings (even local), and can thus be removed. The other thing that could be done is to use it as a flag to disable updates from the remote backend.

forslund avatar Mar 06 '21 10:03 forslund

https://github.com/MycroftAI/mycroft-core/pull/2734 already provides a mechanism to globally disable setting updates from backend

i do like the idea of being able to disable settings upload per skill, i really dont want to upload sensitive data by accident (whenever the skill settings 2 way sync finally gets fixed...)

i see a lot of value in disabling it in the skill itself, end users might not want to disable the sync globally, but as a skill dev if i can ensure the default is not uploading stuff, then i would integrate with skill settings which i have been avoiding, eg, in https://github.com/JarbasAl/skill-email-commands the config is done in mycroft.conf for privacy reasons

JarbasAl avatar Mar 06 '21 12:03 JarbasAl

I agree that disabling remote settings per skill would probably be useful, (skills with only gui settings or voice settings). Perhaps rename it to use_remote_settings and update the functionality to allow usage of the local settings.json file while disabling remote updates.

forslund avatar Mar 06 '21 18:03 forslund

an alternate solution is what i do in ovos_utils

https://github.com/OpenVoiceOS/ovos_utils/pull/50

this differentiates between public and private skill settings, would account for a skill wanting to expose only some settings to the web ui, in this case the param could simply be deprecated

JarbasAl avatar Mar 06 '21 19:03 JarbasAl

Yes that's a good point. Another way could be to only sync the keys that are in the settingsmeta making the values private by default. Possibly adding a per variable flag for the settingsmeta to disable uploading (for things like @AIIX's gui settings which also uses the settingsmeta).

You changed my mind to "better remove it" whichever option for allowing private/local varibles is decided upon.

forslund avatar Mar 07 '21 07:03 forslund