search

OpenVoiceOS / ovos-PHAL

Apache License 2.0
1 stars 4 forks source link

Support `enabled: False` config for plugins with validators #35

Closed NeonDaniel closed 4 months ago

NeonDaniel commented 4 months ago

Refactor plugin load logic to respect plugins disabled in config Adds unit test coverage for PHAL plugin load Relates to https://github.com/NeonGeckoCom/NeonCore/issues/686

JarbasAl commented 4 months ago

I see that disabling a plugin that has a validator method isn't working in upstream PHAL, sending a PR there

this was a per-plugin thing (except for admin plugins), not universal, but this is a good change!

builderjer commented 4 months ago

Does this mean that the plugin config MUST contain "enabled": true? If the "enabled" key is not in the config, won't it return False?

NeonDaniel commented 4 months ago

Does this mean that the plugin config MUST contain "enabled": true? If the "enabled" key is not in the config, won't it return False?

Only if there isn't a validator (matching existing behavior).

If the enabled key is False, then the plugin won't try to validate or load. If enabled is unset, then it follows the existing behavior where it will try to load if EITHER:

JarbasAl commented 4 months ago

it should load if enabled is not set and validator doesnt exist, ie, by default instaling a plugin is all that is needed to load it

only admin plugins have the extra checks

enabled: false should be respected universally and thats what i thought this PR was doing

NeonDaniel commented 4 months ago

it should load if enabled is not set and validator doesnt exist, ie, by default instaling a plugin is all that is needed to load it

only admin plugins have the extra checks

enabled: false should be respected universally and thats what i thought this PR was doing

Without (and with) this PR, the default is enabled=False if there is no validator. This PR just adds support for the enabled config param when there is a validator.

I'm not opposed to changing the default behavior to load plugins without validators though; this would match the behavior for skills where the default is to load anything that is installed. @mikejgray does this change seem good to you since you already approved the changes as they are now?