Add "exposed" operation for system nexus endpoint#736
Add "exposed" operation for system nexus endpoint#736
Conversation
| // (-- api-linter: core::0136::prepositions=disabled | ||
| // aip.dev/not-precedent: "With" is used to indicate combined operation. --) | ||
| rpc SignalWithStartWorkflowExecution (SignalWithStartWorkflowExecutionRequest) returns (SignalWithStartWorkflowExecutionResponse) { | ||
| option (nexus.v1.operation).tags = "exposed"; |
There was a problem hiding this comment.
For reviewer: this tag can be used by our code generation tool to generate Nexus handler interfaces.
lina-temporal
left a comment
lina-temporal
left a comment
There was a problem hiding this comment.
I like the annotation approach here, I think that even with the downside of a few of the fields being irrelevant in Nexus operation context, it's still the best option for maintainability and adoption across our APIs. Think we're still in discussion on the nature of the codegen, but this part LGTM.
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
We have some questions to answer before we can merge this.
nexusannotations/v1/options.proto
Outdated
|
|
||
| import "google/protobuf/descriptor.proto"; | ||
|
|
||
| option go_package = "github.com/bergundy/nexus-proto-annotations/go/nexus/v1"; |
There was a problem hiding this comment.
The package name here would be different depending on the verdict of where we would want to put these annotations. I don't think we should reference anything in my personal repo.
There was a problem hiding this comment.
Also note that the annotations already exist in the referenced GH repo, I had to inline them here because the package name nexus was conflicting and confusing our protoc generation pipeline.
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
Approved, but let's make sure this is at minimum prototyped in the UI and SDK before merging.
| go install github.com/bufbuild/buf/cmd/buf@v1.27.0 | ||
|
|
||
| ##### Sync external proto dependencies ##### | ||
| sync-nexus-annotations: |
There was a problem hiding this comment.
Why don't we have the same make target for google? Seems like we can sync both. But probably don't sync google as part of this PR if there are any changes there, better to do that separately.
There was a problem hiding this comment.
This should go in nexus-rpc-gen eventually but it's okay if it starts out in this repo.
There was a problem hiding this comment.
I'm fine with what you have here because it gets the job done but the task was to add a proper tool that anybody can use, not something that is specific to our API repo.
| continue | ||
| } | ||
| opOpts := proto.GetExtension(opts, nexusannotationsv1.E_Operation).(*nexusannotationsv1.OperationOptions) | ||
| if !slices.Contains(opOpts.GetTags(), "exposed") { |
There was a problem hiding this comment.
See what https://github.com/bergundy/protoc-gen-go-nexus did here, don't hardcode exposed in the tool, it should be passed as include and exclude tag arguments.
| if err := writeFile(gen, "nexus/temporal-json-schema-models-nexusrpc.yaml", nexusDoc); err != nil { | ||
| return err | ||
| } | ||
| return writeFile(gen, "nexus/temporal-proto-models-nexusrpc.yaml", langsDoc) |
There was a problem hiding this comment.
These paths needs to be parameters, not hardcoded.
| // | ||
| // e.g. message "SignalWithStartWorkflowExecutionRequest" | ||
| // → "../openapi/openapiv3.yaml#/components/schemas/SignalWithStartWorkflowExecutionRequest" | ||
| func openAPIRef(msg protoreflect.MessageDescriptor) string { |
There was a problem hiding this comment.
When you write a proper protoc plugin that is reusable, you wouldn't be able to rely on openapi schemas being available in these paths.
There was a problem hiding this comment.
I've duplicated this code into a draft PR in nexus-rpc/nexus-rpc-gen while we sort out that story: nexus-rpc/nexus-rpc-gen#36
3 participants
What changed?
Add
nexusannotations/v1package to define service operations as exposed for Nexus handlers.A sample of a code generator that takes use of this command is here
Why?
Enables the system nexus endpoint work in server repo
Breaking changes
NA
Server PR
NA