plugins: add support for plugin configs #6613
Conversation
Stebalien
force-pushed
the
feat/plugin-config
branch
from
a7dcac5 to
45f5115
Compare
Stebalien
force-pushed
the
feat/plugin-config
branch
from
45f5115 to
6a5867f
Compare
| pluginpath := filepath.Join(repoPath, "plugins") | ||
|
|
||
| plugins, err := loader.NewPluginLoader() | ||
| plugins, err := loader.NewPluginLoader(repoPath) |
There was a problem hiding this comment.
I needed the repo path so I figured I might as well move a bunch of the plugin loading logic into the loader.
| if err != nil { | ||
| return nil, fmt.Errorf("error loading preloaded plugins: %s", err) | ||
| } | ||
|
|
||
| // check if repo is accessible before loading plugins | ||
| ok, err := checkPermissions(repoPath) |
There was a problem hiding this comment.
This was kind of useless so I didn't bother replicating it. Instead of checking permissions, we just do the thing and see if it fails.
| return nil, err | ||
| } | ||
| if ok { | ||
| if err := plugins.LoadDirectory(pluginpath); err != nil { |
There was a problem hiding this comment.
Called in NewPluginLoader
commands/context.go
Outdated
| @@ -57,6 +57,12 @@ func (c *Context) GetNode() (*core.IpfsNode, error) { | |||
| return nil, errors.New("nil ConstructNode function") | |||
| } | |||
| c.node, err = c.ConstructNode() | |||
| if err != nil { | |||
There was a problem hiding this comment.
Ok, this change doesn't have anything to do with this PR. I just noticed that we were parsing the config twice and figured we could just do it once.
Stebalien
force-pushed
the
feat/plugin-config
branch
from
6a5867f to
0d6bc78
Compare
For now, configs specified in `daemon --init-config` and `init CONFIG` are not available. We should fix this eventually but isn't necessary for now (and supporting this will be annoying).
Stebalien
force-pushed
the
feat/plugin-config
branch
from
0d6bc78 to
150b6dd
Compare
|
@djdv CI is passing. This should be ready for review. |
| } | ||
|
|
||
| // NewPluginLoader creates new plugin loader | ||
| func NewPluginLoader() (*PluginLoader, error) { | ||
| loader := &PluginLoader{plugins: make(map[string]plugin.Plugin, len(preloadPlugins))} | ||
| func NewPluginLoader(repo string) (*PluginLoader, error) { |
There was a problem hiding this comment.
The repo a Plugin receives from the Environment should probably be normalized into absolute paths.
Not sure though, there may be a case for accepting relatives.
Abs patch 1/2
| func NewPluginLoader(repo string) (*PluginLoader, error) { | |
| func NewPluginLoader(repo string) (*PluginLoader, error) { | |
| var err error | |
| if repo, err = filepath.Abs(repo); err != nil { | |
| return nil, err | |
| } |
There was a problem hiding this comment.
Given that we don't do this anywhere else, I'd like to leave this as it is for consistency (that way the repo path is the same everywhere). I believe it'll usually be an absolute path anyways.
If we do run into issues, we should be consistent and make this path absolute at a higher layer.
| for _, v := range preloadPlugins { | ||
| if err := loader.Load(v); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| if err := loader.LoadDirectory(filepath.Join(repo, "plugins")); err != nil { |
There was a problem hiding this comment.
Abs patch 2/2
| if err := loader.LoadDirectory(filepath.Join(repo, "plugins")); err != nil { | |
| if err = loader.LoadDirectory(filepath.Join(repo, "plugins")); err != nil { |
There was a problem hiding this comment.
In general, I don't like using = in assignments inside if statements. The point of using an if statement like this is to scope the err variable to the if statement. Using a normal assignment lets it escape the scope and can be really confusing.
There was a problem hiding this comment.
Basically, this should either be left as-is or should be:
err = loader.LoadDirectory(...)
if err != nil {
// ...
}
There was a problem hiding this comment.
Looks good to me after reading.
Also tested it against my own plugin here: djdv@fefafe7#diff-8d243ab8514fa1b77d41df37a81641c9 with no obvious issues. (Doesn't run when disabled, config values work as expected)
Should simplifying plugin logic a little for plugin developers.
For now, configs specified in
daemon --init-configandinit CONFIGare not available. We should fix this eventually but isn't necessary for now (and supporting this will be annoying).