Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/skill gid #2104

Merged
merged 10 commits into from May 22, 2019
Merged

Feature/skill gid #2104

merged 10 commits into from May 22, 2019

Conversation

forslund
Copy link
Collaborator

Description

Replace skillsmeta hash with skill_gid

How to test

Test with Tartarus and make sure it's working nicely and not crashing the server.

Contributor license agreement signed?

CLA [ Yes ]

@forslund forslund added CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) tagged for review and potential merge Status: Work in progress PR being actively worked on, not yet ready for review. labels Apr 22, 2019
@forslund forslund force-pushed the feature/skill-gid branch 2 times, most recently from 77cfaea to cd634b9 Compare April 24, 2019 06:38
@forslund forslund added Merge after next release For large changes that look good, but we want to keep in Dev a little longer and removed tagged for review and potential merge labels Apr 24, 2019
This also removes the notion of an owner skill and all skills may update settings on the server.
use the userSkill endpoint instead of skill endpoint to always be able
to get settings no matter who the owner is.
- restore storing / loading uuid
- use uuid to delete before pushing settingsmeta
skillMetadata could be accessed in settings without it when checking if remote update was needed.
@pep8speaks
Copy link

pep8speaks commented May 20, 2019

Hello @forslund! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-05-21 15:48:49 UTC

The calls are not implemented and functionality will likely change.
Before sending the skills manifest to the backend attach device uuid as needed.
@forslund forslund merged commit 1300773 into dev May 22, 2019
@penrods penrods deleted the feature/skill-gid branch May 22, 2019 04:36
@cclauss
Copy link
Contributor

cclauss commented Aug 9, 2019

flake8 testing of https://github.com/MycroftAI/mycroft-core on Python 3.7.1

$ flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics

./mycroft/api/__init__.py:399:64: F821 undefined name 'skill_gid'
            LOG.debug("Deleting remote metadata for {}".format(skill_gid))
                                                               ^
./mycroft/api/__init__.py:403:39: F821 undefined name 'skill_gid'
                         "/{}".format(skill_gid))
                                      ^
2     F821 undefined name 'skill_gid'
2

@forslund
Copy link
Collaborator Author

forslund commented Aug 9, 2019

Thanks for reporting. I know I fixed that but must have forgotten to commit.

That's what I get for not writing a unittest.

Will fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Merge after next release For large changes that look good, but we want to keep in Dev a little longer Status: Work in progress PR being actively worked on, not yet ready for review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants