Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

Always install default skills before initial load #2644

Merged
merged 2 commits into from
Aug 11, 2020

Conversation

forslund
Copy link
Collaborator

Description

Following the discussion in #2639, this adds a check for the default skills before considered being initialized, if a default skill is missing the a skill update will occur before the mycroft.skills.initialized message is sent to trigger Padatious training. The check seem to be quite fast due to msm caching and will not cause the skills startup time to be delayed unless a skill is really missing.

This checks if all default skills are installed before continuing with
initial load.

For devices with no skills installed on first start this should ensure
that skills are installed before sending the mycroft.skills.initialized
to start precise training and generating the mycroft.ready message

How to test

Remove a default skill and check that the skill is installed during startup.

Contributor license agreement signed?

CLA [ Yes ]

This checks if all default skills are installed before continuing with
initial load.

For devices with no skills installed on first start this should ensure
that skills are installed before sending the mycroft.skills.initialized
to start precise training and generating the mycroft.ready message
@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Jul 29, 2020
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@j1nx
Copy link

j1nx commented Jul 29, 2020

Currently in the middle of rebuilds, but hopefully tomorrow I can and will test this in combination with the pairing skill and a fresh firstboot.

True if all default skills are installed, else False.
"""
defaults = self.msm.list_all_defaults()[self.msm.platform]
return all([skill.is_local for skill in defaults])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to account for people blacklisting default Skills?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, blacklisting skills still installs them, it just doesn't load them afaik. It's an update we can do but we should probably change the update method as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed an update to exclude blacklisted skills from the default check. I've also looked at excluding blacklisted skills from the install/update procedure which is fairly easy to add, if we think that should be added as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working great.

Yeah I think that makes sense too. If someone has blacklisted a Skill we don't need to install it. Though if there is some security update, perhaps we do want to update?

I see that update and install are handled separately so we could do updates but not installs? Really though, this is a nice to have so I'm going to merge this PR as is and we can do the above in the future.

Don't try do an install on startup if a blacklisted skill is uninstalled
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling krisgesling merged commit 69a1c1d into MycroftAI:dev Aug 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants