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

Add support for running remote check plugins #3524

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

emcfarlane
Copy link
Contributor

@emcfarlane emcfarlane commented Dec 10, 2024

This PR adds support for running remote check plugins in buf lint and buf breaking commands. These plugins are specified as references (<remote/owner/plugin>) in the buf.yaml file and must be pinned to specific versions in the buf.lock file.

To pin plugin versions, users can run buf plugin update command, which updates the buf.lock file with the necessary metadata to resolve the plugin version when run. If a plugin reference in buf.yaml is not pinned in the buf.lock an error is returned prompting the user to pin the plugin version by running buf plugin update. This allows for running checks offline and avoids network access when all plugin data is cached.

Arguments can be provided to both local and remote plugins by specifying any space separated commands after the path or reference (e.g. buf.build/myorg/myplugin --debug). These args are passed as command line arguments on every run.

This PR adds support for invoking of remote Wasm plugins locally. Remote
plugins Refs are resolved using the buf.lock file in the input's v2
workspace.
Copy link
Contributor

github-actions bot commented Dec 10, 2024

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedDec 13, 2024, 6:27 PM

Comment on lines 133 to 138
// GetCheckRunnerProvider gets a CheckRunnerProvider for the given input.
//
// The returned RunnerProvider will be able to run lint and breaking checks
// using the PluginConfigs declared in the input buf.yaml. The input
// provided will resolve to a Workspace, and the remote PluginConfigs
// Refs will be resolved to the PluginKeys from the buf.lock file.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not need to resolve the input to a workspace for GetCheckRunnerProvider.

The first use-case, for buf {lint, breaking}, we are already building the target set of []ImageWithConfigs, we should return the requisite CheckRunnerProvider with it, since it already builds the workspace. It also allows us to not duplicate the logic for the various input refs.

For the LSP use-case, we are calling it in FindModule, which provides the workspace.

For the ls-{lint, breaking}-rules use-case, we have avoided building the workspace thus far, but we may need to call GetWorkspace and refactor the logic accordingly. We already have access to the controller.

I think instead of a generic GetCheckRunnerProvider at the top-level of the controller, we should instead have GetTargetImageWithConfigs now also return the CheckRunnerProvider and then add a function that is something like GetCheckRunnerProviderForWorkspace that gets the check runner provider for a given workspace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored GetCheckRunnerProvider to return the image with configs and a bufcheck.RunnerProvider. For cases where a config override is present we now resolve plugins with the BSR. For other cases, where the default config dir path is used the workspace is always attempted to be loaded. The workspace now provides the PluginKeys, which are used to create a static set for the PluginKeyProvider. Going through testing of the various inputs.

private/buf/bufworkspace/workspace.go Outdated Show resolved Hide resolved
private/buf/bufworkspace/workspace_provider.go Outdated Show resolved Hide resolved
@@ -321,6 +322,7 @@ func testNewWorkspaceProvider(t *testing.T, testModuleDatas ...bufmoduletesting.
bsrProvider,
bsrProvider,
bsrProvider,
bufplugin.NopPluginKeyProvider,
Copy link
Member

Choose a reason for hiding this comment

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

I think we could address this in a follow-up PR, or it might give us a little more confidence here, but we may want to implement something similar to the bufmoduletesting.OmniProvider stubs.

private/bufpkg/bufplugin/plugin_key_provider.go Outdated Show resolved Hide resolved
@@ -371,6 +372,24 @@ func (f *file) FindModule(ctx context.Context) {
return
}

// Get the check runner provider for this file. The client is scoped to
// the inputs buf.lock file, so we need to get the check runner provider
// for the workspace that contains this file.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The LSP is currently supported but in testing it can result in a large number of queued checks causing the LSP state to lag behind the editor. It looks like each file change causes a lint command to be executed. As each lint is slower then the number of changes in the file many false annotations are provided before the current file state is reached. This is exaggerated with Wasm plugins as they are slower then native.

@emcfarlane emcfarlane marked this pull request as draft December 13, 2024 20:18
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