Add plugin id to curated plugin proto #3522
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| @@ -385,6 +385,9 @@ message CuratedPlugin { | |||
| string doc = 22; | |||
| // The collections the Plugin belongs to. | |||
| repeated PluginCollection collections = 23; | |||
| // The Plugin ID uniquely identifies the Plugin entity across all versions. This differs from the | |||
There was a problem hiding this comment.
This needs more docs - I want to understand the design decision from reading the docs here. What is the "Plugin" entity? What does this mean "across all versions"? Why is this on a CuratedPlugin, why does CuratedPlugin have two concepts of ID? Why is this called plugin_id, and should it be?
Future maintainers should be able to understand the above from the docs here
There was a problem hiding this comment.
Curated plugins are immutable references, similar to commits. However, recent work required us to reference a plugin as a whole instead of an individual version (which is what the current id is for).
Internally, we created a top-level plugin entity with a one-to-many relationship to curated plugins. Similar to how we have one plugin to many commits (for Check plugins), we have one plugin to many versions (for protoc plugins).
Until we can port these curated plugins over to the registry-proto definitions, we needed to expose the plugin id, which is being referenced in other (internal) protos. If/when these get added to registry-proto, the plugin_id here will be the same as Plugin.id.
If we don't want to expose the plugin_id within v1alpha1, we can use owner/name. I think I prefer this.
Another option is to add a message Plugin in v1alpha1. However, I think we want to minimize the changes here?
There was a problem hiding this comment.
After typing the above, I'm inclined to close this PR because the concept of Plugin isn't present outside the registry-proto definitions and it may cause more confusion.
There was a problem hiding this comment.
Regardless of choice, my feedback would basically be "take the above and put it into the API docs" :-) Assuming you keep the PR.
There was a problem hiding this comment.
Going to close this PR. This is leaking an entity that doesn't exist within this set of protos.
At some point, we should revisit protoc (and/or future Generate) plugins in registry-proto, but it's also not blocking any work.
We need parts of our existing v1alpha1 API to expose the new Plugin entity we've recently added to the BSR.