Add initial Plugin specification #132

mfridman · 2024-09-25T17:37:05Z

This PR adds a new Plugin entity type. It is modelled like the Module entity.

A plugin and a module both have immutable commits and a label history. Which means they can be similarly referenced within configuration files by a label or an explicit commit.

Note, given the similarities between commits and labels, there's an alternative proposal by @doriable in #132 (comment) to avoid duplicating common definitions.

github-actions · 2024-09-25T17:37:24Z

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

Build	Format	Lint	Breaking	Updated (UTC)
`✅ passed`	`✅ passed`	`✅ passed`	`✅ passed`	Oct 25, 2024, 1:17 PM

doriable · 2024-10-11T18:56:54Z

At the top-level, it seems like we are making carbon copies of definitions for resources, commits, and labels here, which makes sense from the perspective that these are shared concepts. But because of that, I also think rather than making copies, we should actually be defining our APIs to reflect the fact that they are shared. My thoughts are something that looks like:

(main %=)🐱~/src/buf/registry-proto @ $ tree .
.
├── buf
│   └── registry
│       ├── resource
│       │   ├── module
│       │   │   └── v1
│       │   │       ├── file.proto
│       │   │       ├── graph.proto
│       │   │       ├── graph_service.proto
│       │   │       ├── module.proto
│       │   │       └── module_service.proto
│       │   ├── plugin
│       │   │   └── v1
│       │   │       ├── code_generation.proto
│       │   │       ├── collection.proto
│       │   │       ├── plugin.proto
│       │   │       └── plugin_service.proto
│       │   └── v1
│       │       ├── commit.proto
│       │       ├── commit_service.proto
│       │       ├── digest.proto
│       │       ├── download_service.proto
│       │       ├── label.proto
│       │       ├── label_service.proto
│       │       ├── resource.proto
│       │       ├── resource_service.proto
│       │       └── upload_service.proto
│       ├── module (same as current)
│       ├── owner (same as current)
│       └── priv (same as current)
├── buf.lock
└── buf.yaml

Or alternatively, a slightly less nested structure:

(main %=)🐱~/src/buf/registry-proto @ $ tree .
.
├── buf
│   └── registry
│       ├── module
│       │   ├── v1 (same as current)
│       │   ├── v1beta1 (same as current)
│       │   └── v2
│       │       ├── file.proto
│       │       ├── graph.proto
│       │       ├── graph_service.proto
│       │       ├── module.proto
│       │       └── module_service.proto
│       ├── owner (same as current)
│       ├── plugin
│       │   └── v1
│       │       ├── code_generation.proto
│       │       ├── collection.proto
│       │       ├── plugin.proto
│       │       └── plugin_service.proto
│       ├── priv (same as current)
│       └── resource
│           └── v1
│               ├── commit.proto
│               ├── commit_service.proto
│               ├── digest.proto
│               ├── download_service.proto
│               ├── label.proto
│               ├── label_service.proto
│               ├── resource.proto
│               ├── resource_service.proto
│               └── upload_service.proto
├── buf.lock
└── buf.yaml

In either case though, I think the API should reflect what the shared entities are. This proposal would also come with the following changes:

This would add P1 as a new digest type to the overall digests, and then I think I would want to use protovalidate rules to enforce B* digests for module and P1 digest for plugins.
Download/upload contents would now be one-ofs of module content, check plugin content, code generation content
Resource would now be one-of the following: Module, Plugin, Label, or Commit

I am open to different names other than "resource", this just felt consistent with what we have right now. But basically, I think the premise of this is that if there were any foundational changes to either labels and/or commits, right now it's implicit that you need to update both modules and plugins, whereas this would make it explicit.

buf/registry/plugin/v1beta1/code_generation_config.proto

buf/registry/plugin/v1beta1/plugin.proto

mfridman · 2024-10-18T19:31:32Z

buf/registry/plugin/v1beta1/upload_service.proto

+    repeated ScopedLabelRef scoped_label_refs = 3;
+  }
+  // The Contents of all references.
+  repeated Content contents = 1 [(buf.validate.field).repeated.min_items = 1];


Plugins might get quite large, so we should consider the batched nature of this API.

Unlike modules, users will likely be pushing a single plugin. Furthermore, I don't think we have plans for the buf plugin push command to support multiple plugin uploads.

Lastly, there's a cost in keeping these in memory. That was one of the motivations for compressing the content payload, because we can keep a smaller footprint. E.g., the ml plugin compressed went from 16MB -> 3MB. We also need to invoke and validate each plugin.

Options:

Keep repeated, but add a validation rule for exactly one

Keep repeated and min_items, but validate on the server

Remove repeated, allow 1 upload at a time

Every other API in registry-proto operates on batches. In the absence of a concrete performance problem, there is no current reason to change the API to be inconsistent with other APIs. Therefore, no change here is required at this time.

bufdev · 2024-10-23T21:06:27Z

buf/registry/plugin/v1beta1/plugin.proto

+  // be overwritten.
+  bool managed = 11;
+  // The collections the Plugin belongs to.
+  repeated Collection collections = 12;


Collection, as it is written, is an entity. It has an ID, and other fields that look like entities. Throughout this API, when one entity references another, it does so by ID, and each entity has a service that allows access to it. We don't embed entity messages in other entity messages.

bufdev · 2024-10-23T21:09:42Z

buf/registry/plugin/v1beta1/plugin.proto

+    (buf.validate.field).string.max_len = 255,
+    (buf.validate.field).ignore = IGNORE_IF_UNPOPULATED
+  ];
+  // Managed indicates whether a Plugin is managed by the BSR.


This field doesn't make sense to me - what do you mean "they will be overwritten?" What do you mean "automatically updated"? These arent entity properties, this is a function of how users use the API.

This field feels incorrect and like it shouldn't be here.

Removing this from the initial proposal. Will revisit this once we need this functionality.

For future, the intention is to have a way to differentiate between plugins that are custom (user-uploaded) or ones that are managed by the registry via a background job.

Need to think of a better name, managed is confusing.

bufdev · 2024-10-23T21:14:24Z

buf/registry/plugin/v1beta1/plugin_service.proto

+      (buf.validate.field).ignore = IGNORE_IF_UNPOPULATED
+    ];
+    // The Plugin type.
+    PluginType type = 6 [


What does it mean to create a plugin of type PLUGIN_TYPE_PROTOC? More generally, what does it mean to create a plugin without any commits? How is (or how is this not) congruent with Modules?

bufdev · 2024-10-23T21:15:07Z

buf/registry/plugin/v1beta1/plugin_service.proto

+  // The collections to list Plugins for.
+  //
+  // If empty, all Plugins are listed regardless of collection.
+  repeated string collection_names = 4;


Why are you referencing collection names when Collection is an entity?

bufdev · 2024-10-23T21:18:43Z

buf/registry/plugin/v1beta1/plugin.proto

+enum PluginType {
+  PLUGIN_TYPE_UNSPECIFIED = 0;
+  // PLUGIN_TYPE_CHECK says that the Plugin is a check plugin.
+  PLUGIN_TYPE_CHECK = 1;


There was a comment about PLUGIN_TYPE_PROTOC that was deleted from the current view, because PLUGIN_TYPE_PROTOC was removed without us concluding the discussion around whether or not it should be included. I was confused when I couldn't find the discussion.

The answer to whether PLUGIN_TYPE_PROTOC should be included depends on a few areas:

Can you map version/revision to label/commit?

What does it mean to request a download of a protoc plugin? What error is returned otherwise?

How do you explain in the docs that you cannot upload a plugin of type protoc?

- Removed managed from Plugin - Add collection_ids to Plugin - Pull in buf.build/bufbuild/bufplugin and PluginInfo - Add source_control_url to Commit

mfridman · 2024-10-24T13:25:10Z

To recap:

Make collection a well-defined entity with a read-only service to GetCollections and ListCollections. This is a many-many relationship, so we we'll have collections_ids on the Plugin.
Remove the Plugin managed field. We don't need it initially. We will revisit it once we have Buf-managed check plugins.
description and source_url belong to Plugin / Module entity, updateable via the UI or API.
- documentation and source_control_url belong to commits, immutable.
- bufbuild/bufplugin updated accordingly
Okay to depend on bufplugin protos, pull them in as a dependency. Add PluginInfo to Commit.
Punt on protoc plugins. Focus on check plugins. We don't need to solve this to ship check plugins.
Potentially some renaming of the bufplugin to just plugin, unlikely to change to naming to "extensions".

bufdev · 2024-10-24T14:46:43Z

buf.yaml

@@ -2,6 +2,7 @@ version: v2
 name: buf.build/bufbuild/registry
 deps:
  - buf.build/bufbuild/protovalidate
+  - buf.build/bufbuild/bufplugin


Extreme nit: I would sort these alphabetically, to match the lock file

bufdev · 2024-10-24T14:47:15Z

buf/registry/plugin/v1beta1/collection_service.proto

+  }
+}
+
+message GetCollectionsRequest {}


What's up with these?

bufdev · 2024-10-24T14:47:35Z

buf/registry/plugin/v1beta1/commit.proto

+    (buf.validate.field).ignore = IGNORE_IF_UNPOPULATED
+  ];
+  // The plugin info of the Commit.
+  buf.plugin.info.v1.PluginInfo info = 8;


plugin_info

bufdev · 2024-10-24T14:48:14Z

buf/registry/plugin/v1beta1/commit.proto

+    (buf.validate.field).string.max_len = 255,
+    (buf.validate.field).ignore = IGNORE_IF_UNPOPULATED
+  ];
+  // The plugin info of the Commit.


Make this a bit more. Something like "plugin information as stored directly within the plugin. This is part of the Bufplugin API, and can be returned from a plugin's GetPluginInfo implementation."

bufdev · 2024-10-24T14:49:04Z

buf/registry/plugin/v1beta1/upload_service.proto

+    ];
+  }
+  // The Contents of all references.
+  repeated Content contents = 1 [(buf.validate.field).repeated.min_items = 1];


From our conversation: OK to reintroduce CompressionType and compression here

For brevity, do we also want to have compression type on the download service as well for content?

mfridman · 2024-10-24T15:31:08Z

buf/registry/plugin/v1beta1/collection_service.proto

+    option idempotency_level = NO_SIDE_EFFECTS;
+  }
+  // Get the Collections for the given Plugins.
+  rpc GetCollectionsPlugins(GetCollectionsPluginsRequest) returns (GetCollectionsPluginsResponse) {


@bufdev not sure how much you hate this, but as @emcfarlane and I flushed this model out it seemed better to remove collection_ids from the Plugin entity and instead have a flattened list via this RPC.

Happy to bring back collection_ids on Plugin, but figured I'd add this for discussion. Say if you have PluginRefs and want to know which collection each of those belongs to you would:

Call this endpoint with plugin refs

Resolve the set of collection_id against GetCollections

bufdev · 2024-10-24T19:36:04Z

I would probably keep in v1beta1 for a little bit unless you're supremely confident this is the final shape, but up to you

mfridman · 2024-10-25T13:21:20Z

I changed it back to v1beta1 in dea5e2a.

I'd feel more confident marking it as v1 once we've flushed out code generation plugins (if we decide to do that).

mfridman and others added 5 commits September 24, 2024 22:13

wip

e2a11ff

wip; pass format lint

aa08ddf

disable push

2577109

Enable push

2a1a374

first pass at tidying up registry config

861779d

mfridman and others added 22 commits September 25, 2024 13:47

wip

badc37d

wip

fa7e531

wip

c5e3a78

wip

0eac937

wip

a82217d

wip

dff06aa

wip

adb18e6

switch to commits and labels

69b571b

wip

0208156

wip

a3472e1

wip

af0c906

Update commit service and Digest P1

df121ef

add collection entity instead of enum

6887aab

wip

c0a6fdc

fix lint

7741a6b

commit

365a2e4

commit

b34ff09

fix collection id

681ee14

add plugin image uploads

bab5cf2

update plugin create request

a7468f6

update upload_service.proto

0badaec

s/module/plugin and comments

b1f3222

emcfarlane reviewed Oct 16, 2024

View reviewed changes

buf/registry/plugin/v1beta1/code_generation_config.proto Outdated Show resolved Hide resolved

buf/registry/plugin/v1beta1/plugin.proto Show resolved Hide resolved

mfridman added 2 commits October 17, 2024 14:16

update comment

a801d90

add scoped label refs to upload

6813f20

mfridman commented Oct 18, 2024

View reviewed changes

bufdev reviewed Oct 23, 2024

View reviewed changes

bufdev mentioned this pull request Oct 23, 2024

Remove Doc type and update PluginInfo bufbuild/bufplugin#7

Merged

mfridman added 4 commits October 23, 2024 22:00

updates following review

4282fe1

- Removed managed from Plugin - Add collection_ids to Plugin - Pull in buf.build/bufbuild/bufplugin and PluginInfo - Add source_control_url to Commit

restore protovalidate for now

398d8e0

add partial collection_service.proto

304e859

add source_control_url to upload

c47e631

bufdev reviewed Oct 24, 2024

View reviewed changes

add collection service definitions; misc cleanup

2e7c067

mfridman commented Oct 24, 2024

View reviewed changes

mfridman added 6 commits October 24, 2024 12:04

fix comment

624adc0

use The for consistency

b3ac2f4

Add a limit to upload

f6f7205

add GetPluginCollectionAssociations

3dee9d4

rename from v1beta1 to v1

1782bc5

add compression.proto and use in both upload and download

45eb887

bufdev approved these changes Oct 24, 2024

View reviewed changes

rename from v1 to v1beta1

dea5e2a

mfridman mentioned this pull request Oct 25, 2024

Add initial specification for plugin support #126

Closed

11 tasks

mfridman merged commit 81aff04 into main Oct 25, 2024
5 checks passed

mfridman deleted the mf/plugins branch October 25, 2024 14:01

emcfarlane mentioned this pull request Nov 25, 2024

Add ResourceService for v1beta1 Plugins #135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial Plugin specification #132

Add initial Plugin specification #132

mfridman commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading

doriable commented Oct 11, 2024

mfridman Oct 18, 2024 •

edited

Loading

bufdev Oct 23, 2024

bufdev Oct 23, 2024

bufdev Oct 23, 2024

mfridman Oct 23, 2024 •

edited

Loading

bufdev Oct 23, 2024

bufdev Oct 23, 2024

bufdev Oct 23, 2024

mfridman commented Oct 24, 2024

bufdev Oct 24, 2024

bufdev Oct 24, 2024

bufdev Oct 24, 2024

bufdev Oct 24, 2024

bufdev Oct 24, 2024

mfridman Oct 24, 2024

mfridman Oct 24, 2024 •

edited

Loading

bufdev commented Oct 24, 2024

mfridman commented Oct 25, 2024 •

edited

Loading

Add initial Plugin specification #132

Add initial Plugin specification #132

Conversation

mfridman commented Sep 25, 2024 • edited Loading

github-actions bot commented Sep 25, 2024 • edited Loading

doriable commented Oct 11, 2024

mfridman Oct 18, 2024 • 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

mfridman Oct 23, 2024 • 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

mfridman commented Oct 24, 2024

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

mfridman Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

bufdev commented Oct 24, 2024

mfridman commented Oct 25, 2024 • edited Loading

mfridman commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading

mfridman Oct 18, 2024 •

edited

Loading

mfridman Oct 23, 2024 •

edited

Loading

mfridman Oct 24, 2024 •

edited

Loading

mfridman commented Oct 25, 2024 •

edited

Loading