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.

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

Run bufcheck WithPlugins option #3327

Closed
wants to merge 1 commit into from
Closed

Run bufcheck WithPlugins option #3327

wants to merge 1 commit into from

Conversation

emcfarlane
Copy link
Contributor

@emcfarlane emcfarlane commented Sep 17, 2024

Moves the bufconfig.PluginConfig to be provided with the pluginrpc.Runner as a bufcheck.Plugin. This allows for setting the runner based on the configuration of the plugin. Removes the default runner as this is now provided when invoking a bufcheck check using the new WithPlugins client function option that replaces WithPluginConfigs. RunnerProvider is removed. A helper NewPluginsForRunner is provided to map local plugins to a command.Runner.

Moves the bufconfig.PluginConfig to be provided with the plugin runner
as a Plugin. This allows for setting the pluginrpc.Runner based on the
config.
Copy link
Contributor

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedSep 17, 2024, 2:42 PM

@emcfarlane emcfarlane changed the title Run bufcheck with Plugins Run bufcheck WithPlugins option Sep 17, 2024
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.

@emcfarlane emcfarlane closed this Sep 18, 2024
@emcfarlane emcfarlane deleted the ed/customPlugins branch September 18, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants