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

refactor/skill_settings_change #92

Merged
merged 5 commits into from
Mar 19, 2024
Merged

refactor/skill_settings_change #92

merged 5 commits into from
Mar 19, 2024

Conversation

JarbasAl
Copy link
Member

@JarbasAl JarbasAl commented May 26, 2023

companion to OpenVoiceOS/ovos-core#325

once that PR is merged we can do a version check to ensure compatibility with older core versions, it should be merged first

@JarbasAl JarbasAl added the enhancement New feature or request label May 26, 2023
@JarbasAl JarbasAl requested review from j1nx, NeonDaniel and a team May 26, 2023 17:15
@JarbasAl JarbasAl marked this pull request as draft May 26, 2023 17:17
Copy link
Contributor

@mikejgray mikejgray left a comment

Choose a reason for hiding this comment

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

This looks fine in terms of the code, but I'm not sure how to even test it works as expected. Not familiar enough with this portion of the codebase yet

ovos_workshop/skills/base.py Outdated Show resolved Hide resolved
@JarbasAl JarbasAl force-pushed the refactor/settings_watchdog branch from 5da7cbb to f729835 Compare March 19, 2024 17:00
@JarbasAl JarbasAl marked this pull request as ready for review March 19, 2024 17:00
@JarbasAl JarbasAl requested review from mikejgray, goldyfruit and a team March 19, 2024 17:00
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 58.33333% with 5 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (dev@80deb75). Click here to learn what that means.

Files Patch % Lines
ovos_workshop/skills/ovos.py 58.33% 5 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##             dev      #92   +/-   ##
======================================
  Coverage       ?   54.07%           
======================================
  Files          ?       36           
  Lines          ?     3752           
  Branches       ?        0           
======================================
  Hits           ?     2029           
  Misses         ?     1723           
  Partials       ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JarbasAl
Copy link
Member Author

JarbasAl commented Mar 19, 2024

@goldyfruit does over docker share the XDG directory for skill settings between skill volumes and core container?

not sure what the default for the docker flag should be

ovos-core is monitoring the folder for changes in settings.json , and emits a bus event when detected, depending if it has access to the volume or not i need to account for that here, either skills need to monitor their own file separately or can just wait for core events (less threads running)

@goldyfruit
Copy link
Member

Yes, ovos_core and ovos_skill_* containers share ~/.config/mycroft/.

@goldyfruit
Copy link
Member

@JarbasAl JarbasAl merged commit 51612ee into dev Mar 19, 2024
9 checks passed
@JarbasAl JarbasAl deleted the refactor/settings_watchdog branch March 19, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants