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 grpc-docs support to grpc api widget #11746

Closed

Conversation

kissmikijr
Copy link
Contributor

@kissmikijr kissmikijr commented May 31, 2022

Hey, I just made a Pull Request!

Adds the possibility to render a generated protoc-gen-doc descriptors inside the gRPC API widget. I added this functionality into the gRPC widget because the APIs that this represents are still type: grpc, but just rendered in a nice way, something like what openapi does.

This uses the grpc-docs package to render the generated files.

With an entity setup:

apiVersion: backstage.io/v1alpha1
kind: API
metadata:
  annotations:
   gendocu-com/grpc-docs: protoc-gen-doc
  name: hello-world
  description: Hello World example for gRPC
spec:
  type: grpc
  lifecycle: deprecated
  owner: team-c
  definition:
    $text: https://github.com/myOrg/myRepo/my-descriptor.json

An example gendocu json rendered in the apiwidget:
image

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@github-actions
Copy link
Contributor

github-actions bot commented May 31, 2022

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-api-docs plugins/api-docs minor v0.8.6-next.1

@kissmikijr
Copy link
Contributor Author

I can see that the verify fails because of the usage of a private registry for one of the deps. I opened an issue in the package that I'd like to use for this work, to publish it to npm.

Copy link

@plaflamme plaflamme left a comment

Choose a reason for hiding this comment

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

Thanks, this is great!

@@ -15,6 +15,7 @@ Right now, the following API formats are supported:
- [OpenAPI](https://swagger.io/specification/) 2 & 3
- [AsyncAPI](https://www.asyncapi.com/docs/specifications/latest/)
- [GraphQL](https://graphql.org/learn/schema/)
- [gRPC](https://grpc.io/) - Using [GenDocu](https://github.com/pseudomuto/protoc-gen-doc) if configured, otherwise plain text.

Choose a reason for hiding this comment

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

GenDocu and protoc-gen-doc are separate things, developed independently. Perhaps this should mention both, e.g.:

@@ -218,6 +219,27 @@ security:
- [read_pets, write_pets]
```

### gRPC rendering protoc-gen-doc descriptors

To configure the gRPC api widget to render your generated [protoc-gen-doc](https://github.com/pseudomuto/protoc-gen-doc) files add the following annotation to your api entities: `'backstage.io/definition-generated-by': 'protoc-gen-doc'`

Choose a reason for hiding this comment

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

In theory, there could be other plugins similar to GenDocu using the same protoc-gen-doc output. Perhaps the annotation should be gendocu-com/grpc-docs? Also, that plugin may very well decide to use a different input in the future and not rely on protoc-gen-docs at all...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, valid points, I am not familiar with this whole ecosystem, and tbh thought they are the same :D But in this case it makes much more sense to have the annotation specify the plugin that consumes theprotoc-gen-doc output

@kissmikijr
Copy link
Contributor Author

@plaflamme Now that I think I start to understand how this works, and connects together a little bit more, maybe it would be better to just use only the protoc-gen-doc plugin. I think we should be able to pretty easily render the markdown output of that plugin.
With this route, we wouldn't introduce 2 new dependencies, only one, but that package seems like it is being used in the grpc world.

Wdyt?

@plaflamme
Copy link

@kissmikijr Although I think it can also be useful for some to show the markdown output of that plugin, I believe that the grpc-docs plugin provides a better user experience. We currently use the strategy of displaying markdown for our protocols and it's great, particularly for the more complicated ones.

I would recommend trying this on non-trivial protocols like the ones provided by Google here to get a sense of what "actual" use-cases might look like.

@kissmikijr kissmikijr changed the title Add protoc-gen-doc support to grpc api widget Add grpc-docs support to grpc api widget Jun 2, 2022
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Thank you! 👍

Couple of initial thoughts

@@ -28,7 +28,7 @@ export const ApiDefinitionCard: () => JSX.Element;
export type ApiDefinitionWidget = {
type: string;
title: string;
component: (definition: string) => React_2.ReactElement;
component: (entity: ApiEntity) => React_2.ReactElement;
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if we should deprecate the definition param and then grab the entity with useEntity for these instead? That way we avoid the breaking change for now and we eventually reduce the API surface.

Comment on lines 33 to 34
props.entity.metadata.annotations?.['gendocu-com/grpc-docs'] ===
'protoc-gen-doc'
Copy link
Member

Choose a reason for hiding this comment

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

This should perhaps be a different API type instead right? The definition is completely different as far as I understand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are correct, however in this case the API entity would represent a service's API which is technically a grpc API, but got transformed to be able to render a lot more clear and in the API.
The type of an entity spec is rendered on the API catalog page in the table and if we move this to a different type it would mean that the new type will be rendered in the table.

I think you are right and trying to force it to render the correct type in the table probably not a good idea :D I'll move this to a different type!

"graphql": "^16.0.0",
"graphql-ws": "^5.4.1",
"grpc-docs": "^1.1.2",
Copy link
Member

Choose a reason for hiding this comment

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

Not hyped about the look of this dep tbh, don't really want to add it to everyones trees at this point. What do you think about perhaps making this a module instead that provides an easy way to add this rendering method to the widget API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, and is reasonable ask. I'll move it to a module. What do you mean by an easy way of adding this rendering method to the widget API ? Wouldn't it be a added like this: https://github.com/backstage/backstage/tree/master/plugins/api-docs#custom-api-renderings ?

Copy link
Member

Choose a reason for hiding this comment

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

Was gonna say you could provide a factory too, but realized that would create a dependency on the plugin package, so perhaps not. Would hope that we could simply export a component from the separate module that matches ApiDefinitionWidget and that the module doesn't need a dependency on the plugin.

Proper way would otherwise be a *-react package, but ain't nobody got time for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll create a *-react package, shouldn't take that much longer I hope

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.

None yet

3 participants