Update bufcheck.RunnerProvider to use the plugin config #3328
Conversation
Changes bufcheck.RunnerProvider to take the bufconfig.PluginConfig as the argurment for creating the runner. This allows for setting the runner based on configuration other than the plugin path. A helper has been provided to create local runners for a command.Runner.
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| } | ||
|
|
||
| // NewRunnerProvider returns a new RunnerProvider for the command.Runner. | ||
| func NewRunnerProvider(delegate command.Runner) RunnerProvider { |
There was a problem hiding this comment.
To avoid import of bufconfig in a pkg dir this has been moved to bufcheck.
| @@ -331,21 +331,17 @@ func (c *client) getMultiClient( | |||
| ) | |||
| } | |||
| for _, pluginConfig := range pluginConfigs { | |||
| if pluginConfig.Type() != bufconfig.PluginConfigTypeLocal { | |||
| return nil, syserror.New("we only handle local plugins for now with lint and breaking") | |||
There was a problem hiding this comment.
Error message is moved to the runner provider.
| annotations := slicesext.Map( | ||
| delegateResponse.Annotations(), | ||
| func(checkAnnotation check.Annotation) *annotation { | ||
| return newAnnotation(checkAnnotation, delegate.PluginName) | ||
| }, | ||
| ) | ||
| lock.Lock() |
There was a problem hiding this comment.
Unrelated change, looks like we can move the lock to wrap just the append.
| type RunnerProvider interface { | ||
| NewRunner(programName string, programArgs ...string) pluginrpc.Runner | ||
| NewRunner(pluginConfig bufconfig.PluginConfig) (pluginrpc.Runner, error) |
There was a problem hiding this comment.
I believe this is the primary motivation of this PR, otherwise, this interface is tightly coupled to executing local binaries command exec style. Specifically here:
buf/private/bufpkg/bufcheck/client.go
Lines 344 to 348 in 6df73b9
By taking a PluginConfig, we can have constructors for various configs like NewLocalPluginConfig and NewRemotePluginConfigNewLocalWASMPluginConfig (bad name, but something to differentiate).
This lifts the pluginConfig.Type() != bufconfig.PluginConfigTypeLocal check to the NewRunnerProvider constructor. The provider now has more information about what the RunnerProvider should be.
I think it'd be awesome if in the end state, we had a common runner provider interface that could support:
- executing local binaries
- executing compiled WASM modules (this would be super handy for testing, although you could argue option 1 is good enough)
- executing WASM modules downloaded from the BSR locally
Update
bufcheck.RunnerProviderto take thebufconfig.PluginConfigas the argument for creating the runner. This allows for setting the runner not based on the the local path config. A helper has been provided to create local runners for a command.Runner.