search

OpenVoiceOS / ovos_skill_manager

skill installer for OVOS
Apache License 2.0
5 stars 6 forks source link

Updating skills installed with branch specs #28

Closed NeonDaniel closed 3 years ago

NeonDaniel commented 3 years ago

With the ability to specify a branch in requirements, it seems like auto-updates after the fact should respect those branch specs but currently they utilize the values in skill.json. Looking for opinions on this and what the best method may be..

My current proposal would be to overwrite the skill.json url and branch at install time when a branch is specified.

ChanceNCounter commented 3 years ago

From a first-pass design perspective, updates should do what installation did, but that obviously isn't good enough.

From a devs-and-hackers perspective, I think we should follow MSM's lead and refrain from altering repos that can't fast-forward. I also don't think we should change branches, because updates might have knock-on effects when you change back to the branch you were on.

From a user-support perspective, which imo should win out, destructive updates are the price of altering software with auto-updates enabled. The whole premise behind automatic updates is that end users should get a clean upgrade to the newest and/or best stable version available, if only for security and performance reasons. It should happen smoothly, it should happen according to the user's expectations, and it should happen quietly when nothing has gone wrong.

From that perspective, +1 requirements and go nuclear, or offer a confirmation when the upgrade isn't clean. It's probably worth discussing an upgrade paradigm unto itself.

JarbasAl commented 3 years ago

osm has no concept of update really, an update just means installing again, it checks md5 sums to let you know if something changed, but there is no distinction between update and install, that would be up to the user... auto updates are out of scope for osm completely

in the context of neon the skills service is what handles auto updates, and this is only for default skills

not sure if i understood the issue correctly, does "specify a branch in requirements" mean a skill requiring a skill in skill_requirements.txt?

NeonDaniel commented 3 years ago

I hadn't considered the local changes questions, but that's a good point.. At the moment, I don't think we have any indication of local changes unless we start tracking OSM install time and comparing with last modified time. I believe this means the Neon Core updater implementaiton is already clobbering local changes.

I agree in general that devs just turn off auto-updates if they want to make local changes; FWIW my workflow would be to make changes in git and update the device anyway. From an integrator standpoint, I want to define my skills list and trust OSM to do what I tell it and update as specified to the branch I specified, regardless of local changes.

I think in the future this does become a broader question of what, when, and how to update skills with local changes; my initial thoughts regarding options are:

NeonDaniel commented 3 years ago

osm has no concept of update really, an update just means installing again, it checks md5 sums to let you know if something changed, but there is no distinction between update and install, that would be up to the user... auto updates are out of scope for osm completely

in the context of neon the skills service is what handles auto updates, and this is only for default skills

not sure if i understood the issue correctly, does "specify a branch in requirements" mean a skill requiring a skill in skill_requirements.txt?

Sorry, requirements was the wrong term, I was referring to default_skills or essential_skills

JarbasAl commented 3 years ago

so this issue is about https://github.com/OpenVoiceOS/ovos-core/commit/1d937af88924af4f756da60947dee64b93f657fe

osm itself does not handle auto updates, thats using the event scheduler in repos above

i think that for all osm is concerned it treats all urls the same, and when told to install something, it does. maybe we want to make behavior configurable here?

NeonDaniel commented 3 years ago

This is

should we move this issue to https://github.com/OpenVoiceOS/ovos-core then? or maybe even https://github.com/NeonJarbas/NeonCore/ ?

osm itself does not handle auto updates, thats using the event scheduler in repos above

i think that for all osm is concerned it treats all urls the same, and when told to install something, it does. maybe we want to make behavior configurable here?

  • skill exists -> overwrite
  • skill exists -> abort
  • skill exists -> prompt user (cli / skill / gui)

This is fine by me, really more of a core question than a skill manager one at this point; this really is behaving as programmed, unless we want to put update logic within OSM

ChanceNCounter commented 3 years ago

fwiw, I'd posit that osm should handle auto-updates, and have implementations set its config, rather than leaving it to each implementation to write an update mechanism. It's Python. If somebody really hates it, they can override it, but the first people who will have to write it in separate downstreams are :wave: the people tagged in this issue =P

JarbasAl commented 3 years ago

auto updates are handled by mycroft-core, mycroft does not want to support alternative appstores, it would be a bunch of work to make things compatible

i really am convinced auto updates are out of scope, those could be as simple as a cronjob passing urls to osm

NeonDaniel commented 3 years ago

Currently, auto-update is out of scope of this repo. Closing issue as current update implementation exists elsewhere. A new issue should probably be created if auto-update functionality is desired in OSM directly