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

Jump to bottom

Run bufcheck WithPlugins option #3327

Closed
wants to merge 1 commit into from
Closed

Run bufcheck WithPlugins option #3327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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)
if err != nil {
return nil, err
}
Expand Down
4 changes: 1 addition & 3 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 @@ -38,9 +38,7 @@ import (
imagev1 "github.com/bufbuild/buf/private/gen/proto/go/buf/alpha/image/v1"
"github.com/bufbuild/buf/private/pkg/app/appcmd"
"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 +1348,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)
require.NoError(t, err)
allRules, err := client.AllRules(context.Background(), check.RuleTypeBreaking, version)
require.NoError(t, err)
Expand Down
9 changes: 6 additions & 3 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,12 +209,16 @@ 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.ClientWithStderr(container.Stderr()))
if err != nil {
return err
}
plugins, err := bufcheck.NewPluginsForRunner(command.NewRunner(), imageWithConfig.PluginConfigs()...)
if err != nil {
return err
}
breakingOptions := []bufcheck.BreakingOption{
bufcheck.WithPluginConfigs(imageWithConfig.PluginConfigs()...),
bufcheck.WithPlugins(plugins...),
}
if flags.ExcludeImports {
breakingOptions = append(breakingOptions, bufcheck.BreakingWithExcludeImports())
Expand Down
15 changes: 11 additions & 4 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.ClientWithStderr(container.Stderr()))
if err != nil {
return err
}
Expand Down Expand Up @@ -228,8 +227,12 @@ func lsRun(
default:
return fmt.Errorf("unknown check.RuleType: %v", ruleType)
}
plugins, err := bufcheck.NewPluginsForRunner(command.NewRunner(), bufYAMLFile.PluginConfigs()...)
if err != nil {
return err
}
configuredRuleOptions := []bufcheck.ConfiguredRulesOption{
bufcheck.WithPluginConfigs(bufYAMLFile.PluginConfigs()...),
bufcheck.WithPlugins(plugins...),
}
if bufcli.IsPluginEnabled(container) {
configuredRuleOptions = append(configuredRuleOptions, bufcheck.WithPluginsEnabled())
Expand All @@ -244,8 +247,12 @@ func lsRun(
return err
}
} else {
plugins, err := bufcheck.NewPluginsForRunner(command.NewRunner(), bufYAMLFile.PluginConfigs()...)
if err != nil {
return err
}
allRulesOptions := []bufcheck.AllRulesOption{
bufcheck.WithPluginConfigs(bufYAMLFile.PluginConfigs()...),
bufcheck.WithPlugins(plugins...),
}
if bufcli.IsPluginEnabled(container) {
allRulesOptions = append(allRulesOptions, bufcheck.WithPluginsEnabled())
Expand Down
9 changes: 6 additions & 3 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,12 +134,16 @@ 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.ClientWithStderr(container.Stderr()))
if err != nil {
return err
}
plugins, err := bufcheck.NewPluginsForRunner(command.NewRunner(), imageWithConfig.PluginConfigs()...)
if err != nil {
return err
}
lintOptions := []bufcheck.LintOption{
bufcheck.WithPluginConfigs(imageWithConfig.PluginConfigs()...),
bufcheck.WithPlugins(plugins...),
}
if bufcli.IsPluginEnabled(container) {
lintOptions = append(lintOptions, bufcheck.WithPluginsEnabled())
Expand Down
4 changes: 1 addition & 3 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 @@ -26,8 +26,6 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufconfig"
"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 +174,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.ClientWithStderr(container.Stderr()))
if err != nil {
return err
}
Expand Down
4 changes: 1 addition & 3 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 @@ -28,9 +28,7 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufanalysis"
"github.com/bufbuild/buf/private/bufpkg/bufcheck"
"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 +124,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.ClientWithStderr(pluginEnv.Stderr))
if err != nil {
return err
}
Expand Down
4 changes: 1 addition & 3 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 @@ -27,9 +27,7 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufanalysis"
"github.com/bufbuild/buf/private/bufpkg/bufcheck"
"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 +99,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.ClientWithStderr(pluginEnv.Stderr))
if err != nil {
return err
}
Expand Down
7 changes: 4 additions & 3 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,15 +1347,17 @@ 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()))
plugins, err := bufcheck.NewPluginsForRunner(command.NewRunner(), workspace.PluginConfigs()...)
require.NoError(t, err)
client, err := bufcheck.NewClient(zap.NewNop(), tracing.NopTracer)
require.NoError(t, err)
err = client.Breaking(
ctx,
breakingConfig,
image,
previousImage,
bufcheck.BreakingWithExcludeImports(),
bufcheck.WithPluginConfigs(workspace.PluginConfigs()...),
bufcheck.WithPlugins(plugins...),
bufcheck.WithPluginsEnabled(),
)
if len(expectedFileAnnotations) == 0 {
Expand Down
47 changes: 33 additions & 14 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 @@ -145,12 +147,12 @@ type ClientFunctionOption interface {
AllCategoriesOption
}

// WithPluginConfigs returns a new ClientFunctionOption that says to also use the given plugins.
// WithPlugin returns a new ClientFunctionOption that says to also use the given plugins.
//
// The default is to only use the builtin Rules and Categories.
func WithPluginConfigs(pluginConfigs ...bufconfig.PluginConfig) ClientFunctionOption {
return &pluginConfigsOption{
pluginConfigs: pluginConfigs,
func WithPlugins(plugins ...Plugin) ClientFunctionOption {
return &pluginsOption{
plugins: plugins,
}
}

Expand All @@ -162,27 +164,44 @@ func WithPluginsEnabled() ClientFunctionOption {
return pluginsEnabledOption{}
}

// RunnerProvider provides pluginrpc.Runners for program names and args.
type RunnerProvider interface {
NewRunner(programName string, programArgs ...string) pluginrpc.Runner
// Plugin is a configured lint or breaking plugin that wraps a pluginrpc.Runner.
type Plugin interface {
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand this interface - it doesn't make sense to me why a Plugin is a Runner. Should I be able to execute arbitrary arguments on a Plugin? Almost for sure no.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A plugin is run/invoked so I think having a method Run makes sense. Although maybe the interface name shouldn't be Runner as that does imply handling multiple.

Copy link
Member

Choose a reason for hiding this comment

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

I probably should have been more direct, apologies - I don't think the interface signature makes sense here. I'm not super convinced by this change as a whole, wondering if your RunnerProvider idea might be less invasive.

Config() bufconfig.PluginConfig
pluginrpc.Runner
}

// RunnerProviderFunc is a function that implements RunnerProvider.
type RunnerProviderFunc func(programName string, programArgs ...string) pluginrpc.Runner
// NewPlugin returns a new Plugin.
func NewPlugin(
pluginConfig bufconfig.PluginConfig,
runner pluginrpc.Runner,
) Plugin {
return newPlugin(pluginConfig, runner)
}

// NewRunner implements RunnerProvider.
func (r RunnerProviderFunc) NewRunner(programName string, programArgs ...string) pluginrpc.Runner {
return r(programName, programArgs...)
// NewPluginsForRunner returns new Plugins for the command.Runner and configs.
func NewPluginsForRunner(delegate command.Runner, pluginConfigs ...bufconfig.PluginConfig) ([]Plugin, error) {
return slicesext.MapError(
pluginConfigs,
func(pluginConfig bufconfig.PluginConfig) (Plugin, 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 NewPlugin(
pluginConfig,
pluginrpcutil.NewRunner(delegate, path[0], path[1:]...),
), nil
},
)
}

// NewClient returns a new Client.
func NewClient(
logger *zap.Logger,
tracer tracing.Tracer,
runnerProvider RunnerProvider,
options ...ClientOption,
) (Client, error) {
return newClient(logger, tracer, runnerProvider, options...)
return newClient(logger, tracer, options...)
}

// ClientOption is an option for a new Client.
Expand Down
Loading