Nested plugin binaries are output to their respective directories #1007
Conversation
wbrowne
requested review from
marefr,
andresmgot and
xnyo
and removed request for
a team
| } | ||
|
|
||
| // execNameCache is a cache for the executable name, so we don't have to read the plugin.json file multiple times. | ||
| var executableNameCache sync.Map |
There was a problem hiding this comment.
This is mimicking what we are already doing today with var exname string above
There was a problem hiding this comment.
I know you wanted to mimick what was there already but is this cache relevant? I'd say the time to read the plugin json file is negligible vs the time to compile 😅
There was a problem hiding this comment.
I guess for building for all distros for example you benefit from not reading it 6+ times. I haven't performance tested it or anything though 😅
There was a problem hiding this comment.
I did some light testing and it's negligible it seems. I'll keep it for now for consistency and it does not harm I guess
| var ( | ||
| defaultOutputBinaryPath = "dist" | ||
| defaultPluginJSONPath = "src" | ||
| defaultNestedDataSourcePath = "datasource" |
There was a problem hiding this comment.
I have opted to not add a config option for this because I think if people want to do something out of the standard we are offering (docs to come in plugin-tools), then they can do so with mage. For example:
func Foo() {
configCallback := func(cfg build.Config) (build.Config, error) {
cfg.PluginJSONPath = "src/myCustomPath"
return cfg, nil
}
build.SetBeforeBuildCallback(configCallback)
build.BuildAll()
}
| @@ -40,6 +46,10 @@ func SetBeforeBuildCallback(cb BeforeBuildCallback) error { | |||
|
|
|||
| var exname string | |||
|
|
|||
| // Deprecated: Use getExecutableNameForPlugin instead. | |||
There was a problem hiding this comment.
Nit. This is not exported so maybe can be deleted?
There was a problem hiding this comment.
I wanted to - but this func is being used in a couple of places (for debugging it looks like) and I'm reluctant to change in the off chance someone is relying on this behaviour in the debugging setup!
| } | ||
|
|
||
| // execNameCache is a cache for the executable name, so we don't have to read the plugin.json file multiple times. | ||
| var executableNameCache sync.Map |
There was a problem hiding this comment.
I know you wanted to mimick what was there already but is this cache relevant? I'd say the time to read the plugin json file is negligible vs the time to compile 😅
What this PR does / why we need it:
Currently it is expected that any nested datasource binary is placed in the root of the plugin dist. This forces the
executablepath of the nested plugin to contain relative path information so that the binary can be found when loaded by Grafana.This is problematic as it makes it impossible to run these nested plugins on Windows as the path information is UNIX style. Considering the binary belongs to the nested plugin - it should live in the nested plugin's directory so that it is clear what the binary is associated with.
This PR makes it possible to do so while also keeping existing plugins backwards compatible. Therefore plugins with path information in the nested plugin executable
../$binaryNamewill continue to work.Special notes for your reviewer: