Base go-plugin client/server #4574
Conversation
plugins/base/plugin.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (p *PluginBase) GRPCClient(ctx context.Context, broker *plugin.GRPCBroker, c *grpc.ClientConn) (interface{}, error) { |
There was a problem hiding this comment.
This doesn’t implement the go plugin interface correctly. It wants a context from the context package instead of the older x/net one.
| @@ -1,7 +1,7 @@ | |||
| // Code generated by protoc-gen-go. DO NOT EDIT. | |||
| // source: github.com/hashicorp/nomad/plugins/base/base.proto | |||
| // source: github.com/hashicorp/nomad/plugins/base/proto/base.proto | |||
There was a problem hiding this comment.
I think we need a way too disable go vet on protobuf generated code, this and the other driver plugin proto generated code always result in go vet warnings because it can't correctly resolve usages.
plugins/base/plugin.go
Outdated
| } | ||
| ) | ||
|
|
||
| type PluginBase struct { |
There was a problem hiding this comment.
Comment that this implements the go-plugin GRPCPlugin interface
|
|
||
| // PluginApiVersion returns the version of the Nomad plugin API it is built | ||
| // against. | ||
| PluginApiVersion string |
There was a problem hiding this comment.
Do we want to model these versions as github.com/coreos/go-semver/semver.Version ptrs? Or may be add Getters to the struct that return that type? Just pondering if we should do some sort of validation on these version strings since they'll later be used for load ordering?
There was a problem hiding this comment.
We will use this to parse as semver: https://github.com/hashicorp/go-version
Simpler to keep using this library and expose a primitive type for all languages over protobuf.
There was a problem hiding this comment.
I agree over protobuf. But a helper here in the go interface layer might be nice. Don't want to bikeshed this.
plugins/base/base.go
Outdated
| @@ -0,0 +1,35 @@ | |||
| package shared | |||
There was a problem hiding this comment.
Did you intend to put these in the github.com/hashicorp/nomad/plugins/base package and have the package named shared?
There was a problem hiding this comment.
Nope! Was a refactoring accident.
plugins/base/plugin_test.go
Outdated
| PluginInfoF: knownType, | ||
| } | ||
|
|
||
| conn, server := plugin.TestGRPCConn(t, func(s *grpc.Server) { |
There was a problem hiding this comment.
You might want to use TestPluginGRPCConn and dispense a basePluginClient.
plugins/base/proto/base.proto
Outdated
| @@ -20,8 +20,9 @@ service BasePlugin { | |||
| // PluginType enumerates the type of plugins Nomad supports | |||
| enum PluginType { | |||
| UNKNOWN = 0; | |||
| DRIVER = 1; | |||
| DEVICE = 2; | |||
| BASE = 1; | |||
There was a problem hiding this comment.
Why is BASE considered a plugin type?
There was a problem hiding this comment.
It shouldn't! Was being lazy for testing.
| @@ -312,12 +312,15 @@ | |||
| {"path":"github.com/ulikunitz/xz/internal/hash","checksumSHA1":"vjnTkzNrMs5Xj6so/fq0mQ6dT1c=","revision":"0c6b41e72360850ca4f98dc341fd999726ea007f","revisionTime":"2017-06-05T21:53:11Z"}, | |||
| {"path":"github.com/ulikunitz/xz/internal/xlog","checksumSHA1":"m0pm57ASBK/CTdmC0ppRHO17mBs=","revision":"0c6b41e72360850ca4f98dc341fd999726ea007f","revisionTime":"2017-06-05T21:53:11Z"}, | |||
| {"path":"github.com/ulikunitz/xz/lzma","checksumSHA1":"2vZw6zc8xuNlyVz2QKvdlNSZQ1U=","revision":"0c6b41e72360850ca4f98dc341fd999726ea007f","revisionTime":"2017-06-05T21:53:11Z"}, | |||
| {"path":"github.com/vmihailenco/msgpack","checksumSHA1":"t9A/EE2GhHFPHzK+ksAKgKW9ZC8=","revision":"b5e691b1eb52a28c05e67ab9df303626c095c23b","revisionTime":"2018-06-13T09:15:15Z"}, | |||
| {"path":"github.com/vmihailenco/msgpack/codes","checksumSHA1":"OcTSGT2v7/2saIGq06nDhEZwm8I=","revision":"b5e691b1eb52a28c05e67ab9df303626c095c23b","revisionTime":"2018-06-13T09:15:15Z"}, | |||
There was a problem hiding this comment.
what's the difference between the codec methods in https://github.com/hashicorp/go-msgpack which we already vendor, and this new library? Would be helpful to document or comment on why we need two different vendored libraries to deal with msgpack.
There was a problem hiding this comment.
It is an import of the go-cty/messagepack package. I have a todo to look and see if we can replace the message pack library there with the one we use.
dadgar
force-pushed
the
f-base-go-plugin
branch
from
70e5956 to
1736d93
Compare
|
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 implements a client/server wrapping for a base plugin type.