Add user-facing buf.plugin.yaml #1210

amckinney · 2022-06-16T17:22:54Z

This adds a simple version of the buf.plugin.yaml. Some fields (e.g. deps, better opts, etc) are not included for now, but they will be added later on.

version: v1
name: buf.build/grpc/go
opts:
  - paths=source_relative
runtime:
  go:
    min_version: v1.18
    deps:
      - "google.golang.org/grpc:v1.32.0"

This takes some inspiration from the bufpluginconfig package that already exists, but adapts some of the keys and internal implementation so that we separate the ExternalConfig from the internal Config representation. There are a couple things to consider with this one:

Structured options

Not all plugin options are expressed as key, value pairs. For example, protoc-gen-java has an option that are simple strings, just like we see in command line flags (e.g. --dry-run).

For now, we handle these cases by mapping the key (the option name) to an empty string value. This is susceptible to break down if there's ever a case that we need to explicitly specify the empty string as a value though. I imagine that we might change this later on (options are just free-form strings in the buf.gen.yaml), but this gives us something to work on top of for the time being - comments are left in-line.

Runtime dependencies

Runtime dependencies aren’t easily expressed in the same format across the board. Some output formats produce dependency names with ‘:’ (e.g. Java for Gradle/Maven) and ‘@‘ (e.g. JS and TS). It's also unclear how archive runtime dependencies can be reliably used in a generic fashion - the dependency format will be directly influenced by the target that's being produced.

For now, we can always treat the final ‘:’ element as the version, and trust that the user expressed their dependency in the format that they need. Then, it’s up to the registry (e.g. Go proxy, NPM registry, etc) to recognize this runtime dependency and act accordingly. All of this should be validated in the planned server-side unit test / verification (if possible).

bufdev · 2022-06-16T17:30:09Z

private/bufpkg/bufplugin/bufpluginconfig/bufpluginconfig.go

+
+// ArchiveRuntimeConfig is the runtime configuration for a plugin that can be downloaded
+// as an archive instead of a language-specific registry.
+type ArchiveRuntimeConfig struct {


I don't think this should exist, what's a use case of it?

This is one of the concerns I have in summary. I'm fine with removing this for now, there's plenty of problems brewing in this space. It's something we ought to spend time on and think about in the meantime though. I'll remove it here, but let's discuss more offline.

bufdev · 2022-06-16T17:30:52Z

private/bufpkg/bufplugin/bufpluginconfig/bufpluginconfig.go

+	//
+	//  java_opt=annotate_code
+	//
+	// In those cases, the option value in this map will be set to


Questionable, but I understand it

Agreed on it being questionable - left a note in the summary that mentions we might change this behavior later. Feels fine to start like this for now, and we can iterate on it as we go.

bufdev · 2022-06-16T17:31:09Z

private/bufpkg/bufplugin/bufpluginconfig/bufpluginconfig.go

+// within the ReadBucket of this configuration file.
+//
+// Returns empty string and no error if no configuration file exists.
+func ExistingConfigFilePath(ctx context.Context, readBucket storage.ReadBucket) (string, error) {


Do we need this?

For now, no - I'll remove it and we can add it back later if we ever do.

bufdev · 2022-06-16T17:31:25Z

private/bufpkg/bufplugin/bufpluginconfig/bufpluginconfig.go

+type ExternalConfig struct {
+	Version string                `json:"version,omitempty" yaml:"version,omitempty"`
+	Name    string                `json:"name,omitempty" yaml:"name,omitempty"`
+	Opts    []string              `json:"opts,omitempty" yaml:"opts,omitempty"`


Maybe just options? I don't know

I guess opts is consistent with deps, min_version

Yea, I'd like it to be consistent with the buf.gen.yaml, too.

bufdev · 2022-06-16T17:35:52Z

private/bufpkg/bufplugin/bufpluginconfig/config.go

+		isGoEmpty      = externalRuntimeConfig.Go.IsEmpty()
+		isNPMEmpty     = externalRuntimeConfig.NPM.IsEmpty()
+	)
+	if isArchiveEmpty && isGoEmpty && isNPMEmpty {


Id probably do this as:

if (!isArchiveEmpty && !isGoEmpty) || (!isArchiveEmpty && !isNPMEmpty) || (!isGoEmpty && !isNPMEmpty) { return error } // nil vs error, idk if we should distinguish, right? return &RuntimeConfig { NPM: newNPM, Go: newGo, Archive: newArchive, }

Or, if you want to make this scale to 10 more runtimes:

emptyCount := 0 for _, isEmpty := range []bool { externalRuntimeConfig.Archive.IsEmpty(), externalRuntimeConfig.Go.IsEmpty(), ... } { if isEmpty { emptyCount++ } } if emptyCount > 1 { return error }

Or, add a pkg/boolutil with Count(bool...) int

Noted, I'll find a better way to do it 👍

I would do it with UnmarshalYAML and UnmarshalJSON

bufdev · 2022-06-16T17:38:53Z

private/bufpkg/bufplugin/bufpluginconfig/get.go

+	"go.uber.org/multierr"
+)
+
+func getConfigForBucket(ctx context.Context, readBucket storage.ReadBucket) (_ *Config, retErr error) {


Will getConfigForBucket work the same as buf.yaml, i.e. are we expecting to pass a bucket where buf.plugin.yaml should be at the root? I guess so yea

srikrsna-buf · 2022-06-16T17:37:16Z

private/bufpkg/bufplugin/bufpluginconfig/config.go

+	// Note that this ensures the user's configuration specifies
+	// a 'v' prefix in the version (e.g. v1.18) even though the
+	// minimum version is rendered without it in the go.mod.
+	if externalGoRuntimeConfig.MinVersion != "" && !semver.IsValid(externalGoRuntimeConfig.MinVersion) {


I think we should loose the v and use golang.org/x/mod/modfile.GoVersionRE.MatchString() to check the go version.

I was hoping that all versions would conform to the same style, as in "all of the runtime dependencies require a v prefix, so this should too for consistency". But I felt uneasy about this as I was going about it - I'll adjust this to use the function you just shared and we can go from there.

srikrsna-buf · 2022-06-16T17:44:56Z

private/bufpkg/bufplugin/bufpluginconfig/config.go

+	}, nil
+}
+
+func validateRuntimeDeps(dependencies []string) error {


Instead of defining a format, since we already have language specific config, I think we should have language specific dependency definitions. It gives us,

Idiomatic names (modules for go, packages for npm etc...)

Language specific constraints (simple semver for go, semver with constraints for npm)

Won't have to worry about archive registy as they can include anything that is at least human readable (package/version)

In general I agree, but I don't like that it puts more cognitive overhead on the user to understand how to specify dependencies for the specific runtime in question - it'd be great if we had standard way of specifying deps for the user-facing configuration (what this attempts to propose). This was something I discussed with @bufdev.

With that said, I admit that it feels like this is susceptible to break down at some point - we just need to be mindful of the ergonomics here.

I'm afraid that this increases cognitive overhead for NPM users considerably, because they wouldn't know how to enter acceptable values.

NPM uses version ranges for dependencies, not semver versions. To express compatibility with major version 1 of a package, the range would be given as ^1.2.3. This is very widely used in the ecosystem, but it would be invalid according to this function.

Next, I'd try 1.x, and perhaps just 1, but they would fail as well, because package semver deviates from the spec:

This package follows Semantic Versioning 2.0.0 (see semver.org) with two exceptions. First, it requires the "v" prefix. Second, it recognizes vMAJOR and vMAJOR.MINOR (with no prerelease or build suffixes) as shorthands for vMAJOR.0.0 and vMAJOR.MINOR.0.

srikrsna-buf · 2022-06-16T17:47:18Z

private/bufpkg/bufplugin/bufpluginconfig/bufpluginconfig.go

+type Config struct {
+	// Name is the name of the plugin (e.g. 'buf.build/protocolbuffers/go').
+	Name bufpluginref.PluginIdentity
+	// Options is the set of options available to the plugin.


What does it mean by options available to the plugin. Isn't this the default options that we pass to the plugin?

Yea I can rephrase this comment a bit - they are the default options passed into the plugin.

srikrsna-buf · 2022-06-16T17:53:14Z

private/bufpkg/bufplugin/bufpluginconfig/config.go

+		isGoEmpty      = externalRuntimeConfig.Go.IsEmpty()
+		isNPMEmpty     = externalRuntimeConfig.NPM.IsEmpty()
+	)
+	if isArchiveEmpty && isGoEmpty && isNPMEmpty {


I would do it with UnmarshalYAML and UnmarshalJSON

amckinney · 2022-06-23T16:51:38Z

Landing this one as-is - we have a couple branches stacked on top of this, so this will make it easier to continue iterating. I'll follow-up with all of the remaining concerns in a separate PR soon after this is merged.

Add buf.plugin.yaml primitives

56dda7f

bufdev reviewed Jun 16, 2022

View reviewed changes

srikrsna-buf reviewed Jun 16, 2022

View reviewed changes

amckinney mentioned this pull request Jun 17, 2022

Add 'buf beta plugin push' #1214

Merged

pkwarren approved these changes Jun 22, 2022

View reviewed changes

amckinney merged commit 5f57c01 into main Jun 23, 2022

amckinney deleted the amckinney/tcn-168-plugin-config branch June 23, 2022 16:51

amckinney mentioned this pull request Jun 23, 2022

Refactor and simplify buf.plugin.yaml configuration #1226

Merged

Monirul1 pushed a commit to Monirul1/buf that referenced this pull request Apr 30, 2023

Add buf.plugin.yaml primitives (bufbuild#1210)

8c2471f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user-facing buf.plugin.yaml #1210

Add user-facing buf.plugin.yaml #1210

amckinney commented Jun 16, 2022

bufdev Jun 16, 2022

amckinney Jun 16, 2022

bufdev Jun 16, 2022

amckinney Jun 16, 2022

bufdev Jun 16, 2022

amckinney Jun 16, 2022

bufdev Jun 16, 2022

bufdev Jun 16, 2022

amckinney Jun 16, 2022

bufdev Jun 16, 2022

bufdev Jun 16, 2022

bufdev Jun 16, 2022 •

edited

Loading

amckinney Jun 16, 2022

srikrsna-buf Jun 16, 2022

bufdev Jun 16, 2022

srikrsna-buf Jun 16, 2022

amckinney Jun 16, 2022

srikrsna-buf Jun 16, 2022

amckinney Jun 16, 2022

timostamm Jun 17, 2022

srikrsna-buf Jun 16, 2022

amckinney Jun 16, 2022

srikrsna-buf Jun 16, 2022

amckinney commented Jun 23, 2022

Add user-facing buf.plugin.yaml #1210

Add user-facing buf.plugin.yaml #1210

Conversation

amckinney commented Jun 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bufdev Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amckinney commented Jun 23, 2022

bufdev Jun 16, 2022 •

edited

Loading