Move skill settings.json to self.file_system.path

[NeonDaniel]

Is your feature request related to a problem? Please describe. Feature request to refactor skill settings path to be consistent with self.file_system. Current behavior creates 2 separate directories for skill-specific files, if a skill uses self.file_system.

Describe the solution you'd like Move settings.json to self.file_system, rather than current self.root_path or BaseDirectory.save_config_path( 'mycroft', 'skills', basename(self.root_dir))

Describe alternatives you've considered Current behavior is not broken, only harder to navigate. Old locations could be referenced as they are now, but that potentially leaves config spread among several locations depending on when a skill was installed.

Additional context Using self.file_system should make skill file paths consistent in the event the base paths are changed (i.e. https://github.com/MycroftAI/mycroft-core/pull/2803). Any changes to self.file_system should handle migration of all skill files together (rather than patching individually as proposed here for settings.json)

[forslund]

Wouldn't this be improved a bit when the big XDG PR gets included, moving most if not all configs and the self.file_system under the BaseDirectory.save_config_path(...) ?

[NeonDaniel]

Wouldn't this be improved a bit when the big XDG PR gets included, moving most if not all configs and the self.file_system under the BaseDirectory.save_config_path(...) ?

This was my thinking; if the internal reference is just self.file_system, that object should also be XDG compliant anyway? I see this as related, but not dependent but I could be missing something..

[krisgesling]

Even with XDG is the question then - should settings.json live in:

• XDG_DATA_HOME - eg $HOME/.local/share/mycroft/skills/my-skill/ or
• XDG_CONFIG_HOME - eg $HOME/.config/mycroft/skills/my-skill/?

One one hand it makes sense to separate them since one is a configuration where as the self.file_system is for general data. On another hand is having two directories per Skill needlessly complicated?

Perhaps it leads to a broader question - should the settings.json be read-writeable by a Skill outside of the self.settings object and pre-defined methods? ^^ this would clearly be a breaking change to what you can do currently

[NeonDaniel]

One one hand it makes sense to separate them since one is a configuration where as the self.file_system is for general data. On another hand is having two directories per Skill needlessly complicated?

I tend to lean towards your second point that it adds complication for a skill to keep data in 3 locations (including the install location). I also see some confusion there since self.file_system could contain other configuration files too.

Regarding modifications to settings.json, that also brings up the question of how to sync settings when a backend can also make changes independent of the local copy. I think that discussion is really deserving of a separate issue/spec document

[forslund]

I do think it's good to keep them separated self.file_system is intended for the skill creator to be able to put anything in (data, configuration, small binaries, etc) and do anything to (rm *), while the settings.json is something handled and controlled internally by Mycroft. If a skill creator wipes the file_system location with an rm * the settings.json should not be affected

@krisgesling Regarding the rw rights of the file: The skill settings was always "managed" and a skill creator shouldn't tamper with the file directly. But blocking direct access to the file in the filesystem (for someone who really wants to) would be tricky to implement.

@NeonDaniel the local vs remote settings is worthy of discussion. Last I heard it was said that an improved version of the old settings sync would be the way to go (local changes pushed to "mycroft-home" to keep synced). But I don't know how the work on that is going, so it's definitely worth discussing.

[NeonDaniel]

If a skill creator wipes the file_system location with an rm * the settings.json should not be affected

This is a good point that settings should generally be protected from accidental deletion.. My main concern is that from an integrator/administrator/power user perspective, this adds one more file path to find things. I see self.file_system as a convenient place to keep everything related to a particular skill that isn't managed by the Skill Manager.

The skill settings was always "managed" and a skill creator shouldn't tamper with the file directly. But blocking direct access to the file in the filesystem (for someone who really wants to) would be tricky to implement.

I think some API methods for allowed interactions with settings would be useful here to avoid skills manually making changes. At minimum, I think a skill should be able to modify an existing config value and to reset defaults from settings-meta.

the local vs remote settings is worthy of discussion

Neon hasn't used Selene or any other backend to manage settings in quite a while, so I'm not all that familiar with the current implementation. Generically, I think some kind of synchronization is the right answer vs. treating one source as overriding the other (though it gets complicated when you consider "local" and "remote" are not necessarily 1:1).

[forslund]

Some api methods for that would be an excellent addition, especially restore default would be great. Not sure what you mean with modify existing config since that is there afaik (except good sync)

[NeonDaniel]

Some api methods for that would be an excellent addition, especially restore default would be great. Not sure what you mean with modify existing config since that is there afaik (except good sync)

I think I misunderstood your comment about a skill creator messing with settings files and read it as saying settings shouldn't be modifiable by a skill (changes locked to backend). In general though, API methods for settings would make the actual location of the file less relevant (i.e. if I can just call methods to reset/clear, I'm less concerned with where those values are cached).

[forslund]

Sorry for not being clear :) As you surmised I just mean that the skills / skill creators shouldn't need to bother with the file on disk, they should use a simple api and not need to think about saving and loading and such.

What would be the complete list of desired api methods?

Current Support:

• set, through self.settings
• get, through self.settings

Desired Support:

• Clear (can technically be done through self.settings = {})
• set_defaults() / reset() (not sure what name is best), restore settings to default settings using the settingsmeta.yaml

Is there any other that would be good to have?

[NeonDaniel]

I think that all makes sense.. FWIW, I would just define a SkillSettings object with dict-like set/get support and added methods for reset_defaults, and maybe sync_backend for manual pull of remote settings (or scheduled/automated?). Could be helpful to retain history (cache load-time/pre-backend sync settings) for rolling back?

All of this is probably out of scope of this issue though..

[forslund]

That is pretty much the old implementation of the settings object. In an attempt to make the settings handling simpler we converted it into a normal dict and a couple of helper functions in the skill class.

But at some point it's definitely worth exploring again.