Add initial Nexus support#816
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae21583287
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| for _, feature := range nexusFeatures { | ||
| name := "features-nexus-" + feature.TaskQueue |
There was a problem hiding this comment.
createNexusEndpoints builds endpoint names directly from feature.TaskQueue ("features-nexus-" + feature.TaskQueue), but task queues are derived from feature dirs like nexus/sync_success, so the resulting endpoint name contains / (and other non-identifier characters). Temporal Nexus endpoint names are validated as identifiers (e.g., EndpointSpec.Name pattern), so CreateNexusEndpoint can fail for these generated names, causing all features/nexus/* runs to fail before the workflow starts. Sanitize/encode the task queue into an allowed endpoint-name charset before creating the endpoint.
Useful? React with 👍 / 👎.
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
Worth calling out that this doesn't work in cloud while (I think?) the rest of the features do. It's possible to make it work but endpoint creation is much slower in cloud and you'll need admin credentials to the control plane.
I'd start by documenting that this is OSS only.
Feel free to merge after addressing any meaningful comments.
| r.log.Warn("Failed deleting nexus endpoint", "Endpoint", ep.Name, "Error", err) | ||
| } | ||
| } | ||
| cl.Close() |
There was a problem hiding this comment.
Want to also log if close fails?
There was a problem hiding this comment.
Close can't fail, it doesn't return an error. If the Go SDK does detect an issue it will log though
| var created []createdEndpoint | ||
| cleanup := func() { | ||
| for _, ep := range created { | ||
| delCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) |
There was a problem hiding this comment.
Should this function get a context passed in so it respects any outer cancelations?
There was a problem hiding this comment.
The caller has not context to pass in so don't think it is worth it at this point. The caller would just pass in context.Background()
| } | ||
|
|
||
| for _, feature := range nexusFeatures { | ||
| name := "features-nexus-" + feature.TaskQueue |
There was a problem hiding this comment.
Note that endpoint names have stricter validation than task queues (because we think we may want to use them as DNS entries at some point).
|
|
||
| A workflow invokes a synchronous Nexus operation and receives its result. | ||
|
|
||
| # Detailed spec |
There was a problem hiding this comment.
Worth checking that the operation transitions to completed state, skipping started in this case.
|
|
||
| const ServiceName = "test-service" | ||
|
|
||
| var SayHelloOperation = nexus.NewSyncOperation( |
There was a problem hiding this comment.
Minor suggestion: Since we don't care about the actual operation's logic, do you think it would be net clearer to name this NexusSyncOperation instead? i.e. describe the kind of thing it is, since "SayHello" isn't that important in this context?
| // FromArgs converts the given arguments to features to run. | ||
| func (r *Run) FromArgs(args []string) error { | ||
| for _, arg := range args { | ||
| colonIndex := strings.Index(arg, ":") |
There was a problem hiding this comment.
Nit: For readability, what do you think about moving this parsing logic into a separate function? Something like:
feature, err := parseRunFeature(arg)
if err != nil {
return fmt.Errorf("parsing argument %q: %w", arg, err)
}
r.Features = append(r.Features, feature)3 participants
What was changed
Add initial Nexus support. Just a basic test in Go and Java will follow up with more languages and test
Why?
Catch any regressions with Nexus support early, track cross SDK consistency. Same reason we add any feature here.
Checklist
Closes
How was this tested: