Pass plugin loaders to client and server #4730
Conversation
dadgar
force-pushed
the
f-plugin-loader
branch
from
505c59c
to
7e3c711
Compare
dadgar
force-pushed
the
f-plugin-loader
branch
from
7e3c711
to
66c487a
Compare
There was a problem hiding this comment.
Small nit, looks good though.
Pondered about a way we could only have to add a single new plugin loader field to the config structs with something like below, but ultimately I don't think we gain much from it and the two fields is much more straight forward.
type PluginCatalogWithSingleton interface {
PluginCatalog
Singleton() PluginCatalog
}
``
|
|
||
| pluginConfig = pc | ||
|
|
||
| // TODO We should log the config to warn users about upgrade pathing |
There was a problem hiding this comment.
Should we attempt to merge the two pluginConfigs?
There was a problem hiding this comment.
For now lets keep it simple. Merging behavior at anything more than key merge would be very tricky. How do you merge two arrays for example
| ) | ||
|
|
||
| // Registration is the registration of an internal plugin | ||
| type Registration struct { |
There was a problem hiding this comment.
I would consider adding a func with a Registration receiver that returns the map[string]interface{} after doing the check if ConfigLoader is set.
There was a problem hiding this comment.
Is it possible to create the PluginLoader, use it to initialize the catalog, then wrap it in the singleton wrapper and only expose that wrapper to the client?
I'm unsure why you need both around after initialization, and I'm afraid it will be easy to accidentally use the wrong one since they have the same API.
command/agent/plugins.go
Outdated
|
|
||
| for id, reg := range catalog { | ||
| if reg.Config == nil { | ||
| a.logger.Warn("skipping loading internal plugin because it is missing its configuration", "plugin", id) |
There was a problem hiding this comment.
Is this a programming error? If so it seems like it should be ERROR if not a panic.
| // Capture the list of binaries in the plugins folder | ||
| f, err := os.Open(l.pluginDir) | ||
| if err != nil { | ||
| // There are no plugins to scan | ||
| if os.IsNotExist(err) { |
There was a problem hiding this comment.
Trace (or debug) log message in this case to help users who accidently misconfigured their plugin dir.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR: