fix: silence plugin warnings and folder creation when plugins disabled #4297
Conversation
…potify/Last.fm plugins
Navidrome was emitting a warning when the optional Spotify or
Last.fm agents weren’t enabled, filling the journal with entries like:
level=warning msg="Plugin not found" capability=MetadataAgent name=spotify
Fixed by completely disable the plugin system when Plugins.Enabled = false.
Signed-off-by: Deluan <deluan@navidrome.org>
Signed-off-by: Deluan <deluan@navidrome.org>
Signed-off-by: Deluan <deluan@navidrome.org>
There was a problem hiding this comment.
Summary of Changes
Hello @deluan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses critical issues related to plugin management when plugins are disabled. The primary goal is to prevent unnecessary plugin-related warnings and errors, particularly concerning directory creation, and to improve overall system efficiency by completely bypassing plugin operations when they are not in use. This is achieved through conditional logic and a significant refactoring of the plugin manager to use an interface-based approach with a no-op implementation for disabled states.
Highlights
- Silenced Plugin Warnings: Eliminated repetitive 'Plugin not found' warnings in logs for inactive Spotify/Last.fm agents when plugins are disabled, leading to cleaner log output.
- Conditional Plugin Folder Creation: Modified the application startup logic to ensure the plugin folder is only created when plugins are explicitly enabled in the configuration. This prevents permission errors in restrictive environments where the directory might not be writable or needed.
- Improved Plugin System Efficiency: Introduced a
noopManagerimplementation for the plugin system. WhenPlugins.Enabled = false, the system now uses this no-op manager, completely bypassing plugin loading and related operations, which improves efficiency and reduces unnecessary resource usage. - Plugin Manager Refactoring: Refactored the
plugins.Managerconcrete type into aplugins.Managerinterface and aplugins.managerImplconcrete implementation. This improves modularity, testability, and allows for thenoopManagerto be seamlessly integrated.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issues of plugin-related warnings and unnecessary folder creation when plugins are disabled. The introduction of the Manager interface and noopManager is a good design choice, improving efficiency and maintainability by completely bypassing plugin loading operations when they are not enabled. The refactoring from concrete *Manager types to *managerImpl and the Manager interface is well-executed across the codebase. Error handling in getPlugin has also been improved by returning errors instead of logging warnings, which is a positive change for correctness.
There was a problem hiding this comment.
Pull Request Overview
This PR ensures plugin operations are skipped when plugins are disabled by introducing a Manager interface with a concrete managerImpl and a noopManager, and wrapping plugin folder creation in a feature flag.
- Refactors plugin manager into an interface (
Manager) and implementation (managerImpl), adding anoopManagerwhen plugins are disabled. - Updates all plugin-related methods, adapters, and tests to use
managerImplreceivers. - Wraps plugin directory creation in
Load()so the folder is only created ifPlugins.Enabledis true.
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/runtime_test.go | Updated test to use managerImpl instead of the old *Manager. |
| plugins/runtime.go | Changed plugin runtime methods to receivers on managerImpl. |
| plugins/plugin_lifecycle_manager_test.go | Updated test description and type to managerImpl. |
| plugins/manifest_permissions_test.go | Switched test variable type from *Manager to *managerImpl. |
| plugins/manager_test.go | Updated tests to declare mgr *managerImpl. |
| plugins/manager.go | Introduced Manager interface, managerImpl, and noopManager. |
| plugins/host_websocket_test.go | Switched manager *Manager to *managerImpl in test. |
| plugins/host_websocket.go | Changed websocketService to hold *managerImpl. |
| plugins/host_scheduler_test.go | Updated test to use *managerImpl. |
| plugins/host_scheduler.go | Changed schedulerService to hold *managerImpl. |
| plugins/adapter_websocket_callback.go | Adapter now expects *managerImpl instead of *Manager. |
| plugins/adapter_scrobbler.go | Adapter now expects *managerImpl instead of *Manager. |
| plugins/adapter_scheduler_callback.go | Adapter now expects *managerImpl instead of *Manager. |
| plugins/adapter_media_agent_test.go | Switched test variable type from *Manager to *managerImpl. |
| plugins/adapter_media_agent.go | Adapter now expects *managerImpl instead of *Manager. |
| conf/configuration.go | Wrapped plugin folder creation in if Server.Plugins.Enabled. |
| cmd/wire_injectors.go | Updated wire.Bind to reflect the new plugins.Manager interface. |
| cmd/wire_gen.go | Mirrored injector changes for plugins.Manager bindings. |
Comments suppressed due to low confidence (2)
plugins/manager.go:116
- There's no test covering the case when
Plugins.Enabled = false, which returns anoopManager. Consider adding a unit test to verify that disabling plugins yields a no-op manager with methods that do nothing.
func GetManager(ds model.DataStore, metrics metrics.Metrics) Manager {
plugins/plugin_lifecycle_manager_test.go:21
- [nitpick] Test descriptions should focus on behavior rather than the concrete implementation name. Consider renaming this to
Plugin Lifecycle Managerand using theManagerinterface type in tests to avoid coupling tomanagerImpl.
Describe("Plugin Lifecycle managerImpl", func() {
Description
This PR fixes issues with plugin-related warnings and directory creation that were impacting users when plugins are disabled. The changes ensure that plugin operations are only performed when plugins are actually enabled.
Key Changes:
Plugins.Enabled = falseRelated Issues
Fixes #4292 - This PR addresses the "permission denied" error when creating the plugins directory in environments where plugins are disabled.
Type of Change
Checklist
Please review and check all that apply:
How to Test
Plugins.Enabled = falsein configuration/data/pluginsdirectory is not created when plugins are disabledScreenshots / Demos (if applicable)
Before: Logs filled with warnings like:
After: Clean logs with no plugin-related warnings when plugins are disabled and no builtin agents are configured.
Additional Notes
This fix is particularly important for Docker deployments and environments with restrictive file permissions, where the automatic creation of plugin directories can cause startup failures.