Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update bufcheck.RunnerProvider to use the plugin config #3328

Merged
merged 3 commits into from
Sep 18, 2024
Merged

Update bufcheck.RunnerProvider to use the plugin config #3328

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f214164
Update bufcheck.RunnerProvider to use the plugin config
emcfarlane Sep 18, 2024
a230e9d
Update error message
emcfarlane Sep 18, 2024
faf3294
Merge branch 'main' into ed/bufcheckRunnerProviderConfig
bufdev Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions private/buf/bufmigrate/migrator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/normalpath"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/slicesext"
"github.com/bufbuild/buf/private/pkg/storage"
"github.com/bufbuild/buf/private/pkg/storage/storagemem"
Expand Down Expand Up @@ -713,7 +712,7 @@ func equivalentCheckConfigInV2(
) (bufconfig.CheckConfig, error) {
// No need for custom lint/breaking plugins since there's no plugins to migrate from <=v1.
// TODO: If we ever need v3, then we will have to deal with this.
client, err := bufcheck.NewClient(logger, tracer, pluginrpcutil.NewRunnerProvider(runner))
client, err := bufcheck.NewClient(logger, tracer, bufcheck.NewRunnerProvider(runner))
if err != nil {
return nil, err
}
Expand Down
3 changes: 1 addition & 2 deletions private/buf/cmd/buf/buf_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ import (
"github.com/bufbuild/buf/private/pkg/app/appcmd/appcmdtesting"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/osext"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/slicesext"
"github.com/bufbuild/buf/private/pkg/storage/storageos"
"github.com/bufbuild/buf/private/pkg/storage/storagetesting"
Expand Down Expand Up @@ -1350,7 +1349,7 @@ func TestCheckLsBreakingRulesFromConfigExceptDeprecated(t *testing.T) {
t.Run(version.String(), func(t *testing.T) {
t.Parallel()
// Do not need any custom lint/breaking plugins here.
client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()))
client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer, bufcheck.NewRunnerProvider(command.NewRunner()))
require.NoError(t, err)
allRules, err := client.AllRules(context.Background(), check.RuleTypeBreaking, version)
require.NoError(t, err)
Expand Down
3 changes: 1 addition & 2 deletions private/buf/cmd/buf/command/breaking/breaking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"github.com/bufbuild/buf/private/pkg/app/appcmd"
"github.com/bufbuild/buf/private/pkg/app/appext"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/slicesext"
"github.com/bufbuild/buf/private/pkg/stringutil"
"github.com/bufbuild/buf/private/pkg/tracing"
Expand Down Expand Up @@ -210,7 +209,7 @@ func run(
tracer := tracing.NewTracer(container.Tracer())
var allFileAnnotations []bufanalysis.FileAnnotation
for i, imageWithConfig := range imageWithConfigs {
client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr()))
client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr()))
if err != nil {
return err
}
Expand Down
3 changes: 1 addition & 2 deletions private/buf/cmd/buf/command/config/internal/internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"github.com/bufbuild/buf/private/pkg/app/appext"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/normalpath"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/slicesext"
"github.com/bufbuild/buf/private/pkg/stringutil"
"github.com/bufbuild/buf/private/pkg/syserror"
Expand Down Expand Up @@ -186,7 +185,7 @@ func lsRun(
}
}
tracer := tracing.NewTracer(container.Tracer())
client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr()))
client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr()))
if err != nil {
return err
}
Expand Down
3 changes: 1 addition & 2 deletions private/buf/cmd/buf/command/lint/lint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/bufbuild/buf/private/pkg/app/appcmd"
"github.com/bufbuild/buf/private/pkg/app/appext"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/stringutil"
"github.com/bufbuild/buf/private/pkg/tracing"
"github.com/spf13/pflag"
Expand Down Expand Up @@ -135,7 +134,7 @@ func run(
tracer := tracing.NewTracer(container.Tracer())
var allFileAnnotations []bufanalysis.FileAnnotation
for _, imageWithConfig := range imageWithConfigs {
client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr()))
client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr()))
if err != nil {
return err
}
Expand Down
3 changes: 1 addition & 2 deletions private/buf/cmd/buf/command/mod/internal/internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"github.com/bufbuild/buf/private/pkg/app/appcmd"
"github.com/bufbuild/buf/private/pkg/app/appext"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/slicesext"
"github.com/bufbuild/buf/private/pkg/stringutil"
"github.com/bufbuild/buf/private/pkg/syserror"
Expand Down Expand Up @@ -176,7 +175,7 @@ func lsRun(
}
// BufYAMLFiles <=v1 never had plugins.
tracer := tracing.NewTracer(container.Tracer())
client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr()))
client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(container.Stderr()))
if err != nil {
return err
}
Expand Down
3 changes: 1 addition & 2 deletions private/buf/cmd/protoc-gen-buf-breaking/breaking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufimage"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/encoding"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/protodescriptor"
"github.com/bufbuild/buf/private/pkg/protoencoding"
"github.com/bufbuild/buf/private/pkg/tracing"
Expand Down Expand Up @@ -126,7 +125,7 @@ func handle(
}
// The protoc plugins do not support custom lint/breaking change plugins for now.
tracer := tracing.NewTracer(container.Tracer())
client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(pluginEnv.Stderr))
client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(pluginEnv.Stderr))
if err != nil {
return err
}
Expand Down
3 changes: 1 addition & 2 deletions private/buf/cmd/protoc-gen-buf-lint/lint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufimage"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/encoding"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/protodescriptor"
"github.com/bufbuild/buf/private/pkg/protoencoding"
"github.com/bufbuild/buf/private/pkg/tracing"
Expand Down Expand Up @@ -101,7 +100,7 @@ func handle(
}
// The protoc plugins do not support custom lint/breaking change plugins for now.
tracer := tracing.NewTracer(container.Tracer())
client, err := bufcheck.NewClient(container.Logger(), tracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(pluginEnv.Stderr))
client, err := bufcheck.NewClient(container.Logger(), tracer, bufcheck.NewRunnerProvider(command.NewRunner()), bufcheck.ClientWithStderr(pluginEnv.Stderr))
if err != nil {
return err
}
Expand Down
3 changes: 1 addition & 2 deletions private/bufpkg/bufcheck/breaking_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufimage"
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/storage/storageos"
"github.com/bufbuild/buf/private/pkg/tracing"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -1348,7 +1347,7 @@ func testBreaking(
require.NoError(t, err)
breakingConfig := workspace.GetBreakingConfigForOpaqueID(opaqueID)
require.NotNil(t, breakingConfig)
client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()))
client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer, bufcheck.NewRunnerProvider(command.NewRunner()))
require.NoError(t, err)
err = client.Breaking(
ctx,
Expand Down
30 changes: 25 additions & 5 deletions private/bufpkg/bufcheck/bufcheck.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"buf.build/go/bufplugin/check"
"github.com/bufbuild/buf/private/bufpkg/bufconfig"
"github.com/bufbuild/buf/private/bufpkg/bufimage"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/slicesext"
"github.com/bufbuild/buf/private/pkg/syserror"
"github.com/bufbuild/buf/private/pkg/tracing"
Expand Down Expand Up @@ -162,17 +164,35 @@ func WithPluginsEnabled() ClientFunctionOption {
return pluginsEnabledOption{}
}

// RunnerProvider provides pluginrpc.Runners for program names and args.
// RunnerProvider provides pluginrpc.Runners for a given plugin config.
type RunnerProvider interface {
NewRunner(programName string, programArgs ...string) pluginrpc.Runner
NewRunner(pluginConfig bufconfig.PluginConfig) (pluginrpc.Runner, error)
Copy link
Member

@mfridman mfridman Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

c.runnerProvider.NewRunner(
// We know that Path is of at least length 1.
pluginPath[0],
pluginPath[1:]...,
),

By taking a PluginConfig, we can have constructors for various configs like NewLocalPluginConfig and NewRemotePluginConfig NewLocalWASMPluginConfig (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:

  1. executing local binaries
  2. executing compiled WASM modules (this would be super handy for testing, although you could argue option 1 is good enough)
  3. executing WASM modules downloaded from the BSR locally

}

// RunnerProviderFunc is a function that implements RunnerProvider.
type RunnerProviderFunc func(programName string, programArgs ...string) pluginrpc.Runner
type RunnerProviderFunc func(pluginConfig bufconfig.PluginConfig) (pluginrpc.Runner, error)

// NewRunner implements RunnerProvider.
func (r RunnerProviderFunc) NewRunner(programName string, programArgs ...string) pluginrpc.Runner {
return r(programName, programArgs...)
func (r RunnerProviderFunc) NewRunner(pluginConfig bufconfig.PluginConfig) (pluginrpc.Runner, error) {
return r(pluginConfig)
}

// NewRunnerProvider returns a new RunnerProvider for the command.Runner.
func NewRunnerProvider(delegate command.Runner) RunnerProvider {
return RunnerProviderFunc(
func(pluginConfig bufconfig.PluginConfig) (pluginrpc.Runner, error) {
if pluginConfig.Type() != bufconfig.PluginConfigTypeLocal {
return nil, syserror.New("we only handle local plugins for now with lint and breaking")
}
path := pluginConfig.Path()
return pluginrpcutil.NewRunner(
delegate,
// We know that Path is of at least length 1.
path[0],
path[1:]...,
), nil
},
)
}

// NewClient returns a new Client.
Expand Down
14 changes: 5 additions & 9 deletions private/bufpkg/bufcheck/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error message is moved to the runner provider.

}
options, err := check.NewOptions(pluginConfig.Options())
if err != nil {
return nil, fmt.Errorf("could not parse options for plugin %q: %w", pluginConfig.Name(), err)
}
pluginPath := pluginConfig.Path()
runner, err := c.runnerProvider.NewRunner(pluginConfig)
if err != nil {
return nil, fmt.Errorf("could not create runner for plugin %q: %w", pluginConfig.Name(), err)
}
checkClient := check.NewClient(
pluginrpc.NewClient(
c.runnerProvider.NewRunner(
// We know that Path is of at least length 1.
pluginPath[0],
pluginPath[1:]...,
),
runner,
pluginrpc.ClientWithStderr(c.stderr),
// We have to set binary as some things cannot be encoded as JSON.
// Example: google.protobuf.Timestamps with positive seconds and negative nanos.
Expand Down
3 changes: 1 addition & 2 deletions private/bufpkg/bufcheck/lint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufimage"
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/storage/storageos"
"github.com/bufbuild/buf/private/pkg/tracing"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -1294,7 +1293,7 @@ func testLintWithOptions(

lintConfig := workspace.GetLintConfigForOpaqueID(opaqueID)
require.NotNil(t, lintConfig)
client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer, pluginrpcutil.NewRunnerProvider(command.NewRunner()))
client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer, bufcheck.NewRunnerProvider(command.NewRunner()))
require.NoError(t, err)
err = client.Lint(
ctx,
Expand Down
2 changes: 1 addition & 1 deletion private/bufpkg/bufcheck/multi_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,13 @@ func (c *multiClient) Check(ctx context.Context, request check.Request) ([]*anno
}
return fmt.Errorf("plugin %q failed: %w", delegate.PluginName, err)
}
lock.Lock()
annotations := slicesext.Map(
delegateResponse.Annotations(),
func(checkAnnotation check.Annotation) *annotation {
return newAnnotation(checkAnnotation, delegate.PluginName)
},
)
lock.Lock()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change, looks like we can move the lock to wrap just the append.

allAnnotations = append(allAnnotations, annotations...)
lock.Unlock()
return nil
Expand Down
5 changes: 2 additions & 3 deletions private/bufpkg/bufcheck/multi_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"buf.build/go/bufplugin/check/checkutil"
"github.com/bufbuild/buf/private/bufpkg/bufconfig"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/pluginrpcutil"
"github.com/bufbuild/buf/private/pkg/slicesext"
"github.com/bufbuild/buf/private/pkg/stringutil"
"github.com/bufbuild/buf/private/pkg/tracing"
Expand Down Expand Up @@ -185,7 +184,7 @@ func TestMultiClientCannotHaveOverlappingRulesWithBuiltIn(t *testing.T) {
client, err := newClient(
zaptest.NewLogger(t),
tracing.NopTracer,
pluginrpcutil.NewRunnerProvider(command.NewRunner()),
NewRunnerProvider(command.NewRunner()),
)
require.NoError(t, err)
duplicateBuiltInRulePluginConfig, err := bufconfig.NewLocalPluginConfig(
Expand Down Expand Up @@ -279,7 +278,7 @@ func TestMultiClientCannotHaveOverlappingCategoriesWithBuiltIn(t *testing.T) {
client, err := newClient(
zaptest.NewLogger(t),
tracing.NopTracer,
pluginrpcutil.NewRunnerProvider(command.NewRunner()),
NewRunnerProvider(command.NewRunner()),
)
require.NoError(t, err)
duplicateBuiltInRulePluginConfig, err := bufconfig.NewLocalPluginConfig(
Expand Down
26 changes: 0 additions & 26 deletions private/pkg/pluginrpcutil/pluginrpcutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,29 +23,3 @@ import (
func NewRunner(delegate command.Runner, programName string, programArgs ...string) pluginrpc.Runner {
return newRunner(delegate, programName, programArgs...)
}

// RunnerProvider provides pluginrpc.Runners for program names and args.
type RunnerProvider interface {
NewRunner(programName string, programArgs ...string) pluginrpc.Runner
}

// RunnerProviderFunc is a function that implements RunnerProvider.
type RunnerProviderFunc func(programName string, programArgs ...string) pluginrpc.Runner

// NewRunner implements RunnerProvider.
func (r RunnerProviderFunc) NewRunner(programName string, programArgs ...string) pluginrpc.Runner {
return r(programName, programArgs...)
}

// NewRunnerProvider returns a new RunnerProvider for the command.Runner.
func NewRunnerProvider(delegate command.Runner) RunnerProvider {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid import of bufconfig in a pkg dir this has been moved to bufcheck.

return RunnerProviderFunc(
func(programName string, programArgs ...string) pluginrpc.Runner {
return NewRunner(
delegate,
programName,
programArgs...,
)
},
)
}