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

Add configurable base plugin support in buf.plugin.yaml #3513

Merged
merged 7 commits into from
Dec 6, 2024
Merged

Add configurable base plugin support in buf.plugin.yaml #3513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
57be7b9
update proto
mfridman Dec 5, 2024
86ec011
update implementation
mfridman Dec 5, 2024
f2ad326
code comment
mfridman Dec 5, 2024
a2edfb7
change to "base plugin"
mfridman Dec 5, 2024
a8d1340
cleanup comments and tests
mfridman Dec 5, 2024
e27a190
cleanup and update logic
mfridman Dec 5, 2024
2f9dbb7
clarify comment
mfridman Dec 5, 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
10 changes: 10 additions & 0 deletions private/bufpkg/bufremoteplugin/bufremoteplugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ func PluginRegistryToProtoRegistryConfig(pluginRegistry *bufremotepluginconfig.R
case pluginRegistry.Go != nil:
goConfig := &registryv1alpha1.GoConfig{}
goConfig.MinimumVersion = pluginRegistry.Go.MinVersion
if pluginRegistry.Go.BasePlugin != nil {
goConfig.BasePlugin = pluginRegistry.Go.BasePlugin.IdentityString()
}
if pluginRegistry.Go.Deps != nil {
goConfig.RuntimeLibraries = make([]*registryv1alpha1.GoConfig_RuntimeLibrary, 0, len(pluginRegistry.Go.Deps))
for _, dependency := range pluginRegistry.Go.Deps {
Expand Down Expand Up @@ -249,6 +252,13 @@ func ProtoRegistryConfigToPluginRegistry(config *registryv1alpha1.RegistryConfig
case config.GetGoConfig() != nil:
goConfig := &bufremotepluginconfig.GoRegistryConfig{}
goConfig.MinVersion = config.GetGoConfig().GetMinimumVersion()
if config.GetGoConfig().GetBasePlugin() != "" {
basePluginIdentity, err := bufremotepluginref.PluginIdentityForString(config.GetGoConfig().GetBasePlugin())
if err != nil {
return nil, err
}
goConfig.BasePlugin = basePluginIdentity
}
runtimeLibraries := config.GetGoConfig().GetRuntimeLibraries()
if runtimeLibraries != nil {
goConfig.Deps = make([]*bufremotepluginconfig.GoRegistryDependencyConfig, 0, len(runtimeLibraries))
Expand Down
2 changes: 2 additions & 0 deletions private/bufpkg/bufremoteplugin/bufremotepluginconfig/bufremotepluginconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ type RegistryConfig struct {
type GoRegistryConfig struct {
MinVersion string
Deps []*GoRegistryDependencyConfig
BasePlugin bufremotepluginref.PluginIdentity
}

// GoRegistryDependencyConfig is the go registry dependency configuration.
Expand Down Expand Up @@ -440,6 +441,7 @@ type ExternalGoRegistryConfig struct {
Module string `json:"module,omitempty" yaml:"module,omitempty"`
Version string `json:"version,omitempty" yaml:"version,omitempty"`
} `json:"deps,omitempty" yaml:"deps,omitempty"`
BasePlugin string `json:"base_plugin,omitempty" yaml:"base_plugin,omitempty"`
Copy link
Member Author

@mfridman mfridman Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

An alternative way to configure this might be to expose an is_base: true within deps, e.g.,

deps:
  - plugin: remote/org/go-multi:v0.1.0
    is_base: true

And allowing exactly one plugin within deps to have this be set to true.

However, I don't like this because it leaks a Generated SDK configuration beyond the registry config. (which at the moment is Go-specific).

}

// ExternalNPMRegistryConfig is the external registry configuration for a JavaScript NPM plugin.
Expand Down
6 changes: 6 additions & 0 deletions private/bufpkg/bufremoteplugin/bufremotepluginconfig/bufremotepluginconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ func TestGetConfigForBucket(t *testing.T) {
require.NoError(t, err)
pluginDependency, err := bufremotepluginref.PluginReferenceForString("buf.build/library/go:v1.28.0", 1)
require.NoError(t, err)
basePluginIdentity, err := bufremotepluginref.PluginIdentityForString("buf.build/library/go")
require.NoError(t, err)
require.Equal(
t,
&Config{
Expand All @@ -54,6 +56,7 @@ func TestGetConfigForBucket(t *testing.T) {
Registry: &RegistryConfig{
Go: &GoRegistryConfig{
MinVersion: "1.18",
BasePlugin: basePluginIdentity,
Deps: []*GoRegistryDependencyConfig{
{
Module: "google.golang.org/grpc",
Expand Down Expand Up @@ -82,6 +85,8 @@ func TestParsePluginConfigGoYAML(t *testing.T) {
require.NoError(t, err)
pluginDependency, err := bufremotepluginref.PluginReferenceForString("buf.build/library/go:v1.28.0", 1)
require.NoError(t, err)
basePluginIdentity, err := bufremotepluginref.PluginIdentityForString("buf.build/library/go")
require.NoError(t, err)
require.Equal(
t,
&Config{
Expand All @@ -96,6 +101,7 @@ func TestParsePluginConfigGoYAML(t *testing.T) {
Registry: &RegistryConfig{
Go: &GoRegistryConfig{
MinVersion: "1.18",
BasePlugin: basePluginIdentity,
Deps: []*GoRegistryDependencyConfig{
{
Module: "google.golang.org/grpc",
Expand Down
45 changes: 37 additions & 8 deletions private/bufpkg/bufremoteplugin/bufremotepluginconfig/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package bufremotepluginconfig
import (
"errors"
"fmt"
"slices"
"strings"

"buf.build/go/spdx"
Expand Down Expand Up @@ -60,7 +61,7 @@ func newConfig(externalConfig ExternalConfig, options []ConfigOption) (*Config,
dependencies = append(dependencies, reference)
}
}
registryConfig, err := newRegistryConfig(externalConfig.Registry)
registryConfig, err := newRegistryConfig(externalConfig.Registry, dependencies, opts.overrideRemote)
if err != nil {
return nil, err
}
Expand All @@ -87,7 +88,11 @@ func newConfig(externalConfig ExternalConfig, options []ConfigOption) (*Config,
}, nil
}

func newRegistryConfig(externalRegistryConfig ExternalRegistryConfig) (*RegistryConfig, error) {
func newRegistryConfig(
externalRegistryConfig ExternalRegistryConfig,
pluginDependencies []bufremotepluginref.PluginReference,
overrideRemote string,
) (*RegistryConfig, error) {
var (
isGoEmpty = externalRegistryConfig.Go == nil
isNPMEmpty = externalRegistryConfig.NPM == nil
Expand Down Expand Up @@ -125,7 +130,7 @@ func newRegistryConfig(externalRegistryConfig ExternalRegistryConfig) (*Registry
options := OptionsSliceToPluginOptions(externalRegistryConfig.Opts)
switch {
case !isGoEmpty:
goRegistryConfig, err := newGoRegistryConfig(externalRegistryConfig.Go)
goRegistryConfig, err := newGoRegistryConfig(externalRegistryConfig.Go, pluginDependencies, overrideRemote)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -242,14 +247,18 @@ func newNPMRegistryConfig(externalNPMRegistryConfig *ExternalNPMRegistryConfig)
}, nil
}

func newGoRegistryConfig(externalGoRegistryConfig *ExternalGoRegistryConfig) (*GoRegistryConfig, error) {
func newGoRegistryConfig(
externalGoRegistryConfig *ExternalGoRegistryConfig,
pluginDependencies []bufremotepluginref.PluginReference,
overrideRemote string,
) (*GoRegistryConfig, error) {
if externalGoRegistryConfig == nil {
return nil, nil
}
if externalGoRegistryConfig.MinVersion != "" && !modfile.GoVersionRE.MatchString(externalGoRegistryConfig.MinVersion) {
return nil, fmt.Errorf("the go minimum version %q must be a valid semantic version in the form of <major>.<minor>", externalGoRegistryConfig.MinVersion)
}
var dependencies []*GoRegistryDependencyConfig
var runtimeDependencies []*GoRegistryDependencyConfig
for _, dep := range externalGoRegistryConfig.Deps {
if dep.Module == "" {
return nil, errors.New("go runtime dependency requires a non-empty module name")
Expand All @@ -260,17 +269,37 @@ func newGoRegistryConfig(externalGoRegistryConfig *ExternalGoRegistryConfig) (*G
if !semver.IsValid(dep.Version) {
return nil, fmt.Errorf("go runtime dependency %s:%s does not have a valid semantic version", dep.Module, dep.Version)
}
dependencies = append(
dependencies,
runtimeDependencies = append(
runtimeDependencies,
&GoRegistryDependencyConfig{
Module: dep.Module,
Version: dep.Version,
},
)
}
var basePlugin bufremotepluginref.PluginIdentity
if externalGoRegistryConfig.BasePlugin != "" {
var err error
basePlugin, err = pluginIdentityForStringWithOverrideRemote(externalGoRegistryConfig.BasePlugin, overrideRemote)
if err != nil {
return nil, fmt.Errorf("failed to parse base plugin: %w", err)
}
// Validate the base plugin is included as one of the plugin dependencies when both are
// specified. This ensures there's exactly one base type and it has a known dependency to
// generate imports correctly and build a correct Go mod file.
if len(pluginDependencies) > 0 {
ok := slices.ContainsFunc(pluginDependencies, func(ref bufremotepluginref.PluginReference) bool {
return ref.IdentityString() == basePlugin.IdentityString()
})
if !ok {
return nil, fmt.Errorf("base plugin %q not found in plugin dependencies", externalGoRegistryConfig.BasePlugin)
}
}
Comment on lines +287 to +297
Copy link
Member

Choose a reason for hiding this comment

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

Is the intention that if there's no plugin dependency, assigning base_plugin has no effect? Is there a use-case for assigning it without a matching dependency?

Copy link
Member Author

@mfridman mfridman Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not quite, we still want to support option 1 in the description.

Suppose you have a multi-plugin that slams ALL the generators into a single multi-plugin. In this scenario, there are no plugin deps (all plugins output to the same package), but we still need to generate the correct module imports.

E.g., think of acme/petapis, which depends on acme/paymentapis. Without specifying the base type, we default to protocolbuffers/go, but what we want is something like this:

package petv1

import (
-	v1alpha1 "remote/gen/go/acme/paymentapis/protocolbuffers/go/payment/v1alpha1"
+	v1alpha1 "remote/gen/go/acme/paymentapis/custom-plugins/go/payment/v1alpha1"

Likewise, we need to build a go.mod file that has the correct Go module:

module remote/gen/go/acme/petapis/custom-plugins/go

go 1.21

require (
-	remote/gen/go/acme/paymentapis/protocolbuffers/go v0.2.0-20241204162507-076bca1034ed.1
+	remote/gen/go/acme/paymentapis/custom-plugins/go v0.2.0-20241204162507-076bca1034ed.1

Copy link
Member

Choose a reason for hiding this comment

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

ah, I see, was not thinking about the multi-plugin. That makes sense to me.

Copy link
Member Author

@mfridman mfridman Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if the BSR should support option 1 with no further configuration. If there are no plugin dependencies, use the plugin name itself (instead of protocolbuffers/go) for module dependencies.

Effectively, the same as manually setting just so protocolbuffers/go isn't used.

version: v1
name: remote/org/go-multi # --- SAME
plugin_version: v0.1.0
output_languages:
  - go
registry:
  go:
    base_plugin: remote/org/go-multi # --- SAME
    min_version: "1.21"
    deps:
      - module: google.golang.org/protobuf
        version: v1.35.2
      - module: github.com/planetscale/vtprotobuf
        version: v0.6.0
  # Add the options to invoke each plugin for the generated SDK.
  opts:
    - --go_out=.
    - --go_opt=paths=source_relative
    - --go-json_out=.
    - --go-json_opt=paths=source_relative,emit_defaults=true
    - --go-vtproto_out=.
    - --go-vtproto_opt=paths=source_relative,features=marshal+unmarshal+size

}
return &GoRegistryConfig{
MinVersion: externalGoRegistryConfig.MinVersion,
Deps: dependencies,
Deps: runtimeDependencies,
BasePlugin: basePlugin,
}, nil
}

Expand Down
1 change: 1 addition & 0 deletions private/bufpkg/bufremoteplugin/bufremotepluginconfig/testdata/success/go/buf.plugin.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ deps:
output_languages: [go]
registry:
go:
base_plugin: buf.build/library/go
min_version: 1.18
deps:
- module: google.golang.org/grpc
Expand Down
Loading
Loading