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

Support enabled: False config for plugins with validators #35

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

NeonDaniel
Copy link
Member

Refactor plugin load logic to respect plugins disabled in config
Adds unit test coverage for PHAL plugin load
Relates to NeonGeckoCom/NeonCore#686

Adds unit test coverate for PHAL plugin load
@NeonDaniel NeonDaniel requested a review from a team June 28, 2024 01:24
@JarbasAl
Copy link
Member

JarbasAl commented Jun 28, 2024

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
Copy link
Member

builderjer commented Jun 28, 2024

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
Copy link
Member Author

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:

  • there is a validator method that returns true
  • enabled config is set to True

@JarbasAl
Copy link
Member

JarbasAl commented Jun 28, 2024

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
Copy link
Member Author

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?

@NeonDaniel NeonDaniel marked this pull request as draft June 28, 2024 23:06
@NeonDaniel NeonDaniel requested a review from mikejgray June 28, 2024 23:25
@NeonDaniel NeonDaniel marked this pull request as ready for review June 28, 2024 23:25
@NeonDaniel NeonDaniel merged commit 28a2ca3 into dev Jun 28, 2024
6 checks passed
@NeonDaniel NeonDaniel deleted the FIX_ConfigDisablePlugin branch June 28, 2024 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants