Add support for Standalone Nexus Operations#1046
Conversation
Evanthx
left a comment
There was a problem hiding this comment.
Found a few nit-picks! Your call on whether they are worth addressing, they are all quite minor.
| if err := printNexusOperationFailure(cctx, operationID, runID, v.Failure); err != nil { | ||
| cctx.Logger.Error("Nexus operation failed, and printing the output also failed", "error", err) | ||
| } | ||
| return fmt.Errorf("nexus operation failed") |
There was a problem hiding this comment.
Should Nexus be capitalized? Same for line 112 above and 131, 492 below. Didn't do a complete check for places, but looking at the code for "fmt.Errorf.*nexus" I see it both ways!
There was a problem hiding this comment.
in errors it should be lower case based on convention, same as workflow and logs it may be uppercase
| // Matches the SDK's pollActivityTimeout in internal_activity_client.go. | ||
| const pollNexusOperationTimeout = 60 * time.Second | ||
|
|
||
| // pollNexusOperationOutcome polls for a nexus operation result using a |
There was a problem hiding this comment.
Feel free to ignore this comment - would this be better as a method on Handle then, both for reuse and so that if someone made a change to Handle.Get later this method would be visible there to get the equivalent change? ie, maybe Handle would then have a Get and a RawGet method, something like that?
| } | ||
| resultJSON, err := json.Marshal(valuePtr) | ||
| if err != nil { | ||
| return fmt.Errorf("failed marshaling result: %w", err) |
There was a problem hiding this comment.
Line 202 and 210 return the exact same string as 177 and 184. So if we get that error, we won't actually know which line failed, just that it's one of those two. Given that they are doing the same thing to the same object it might not matter much, but it's nice to just go to the offending log line without having to wonder which line it was.
VegetarianOrc
left a comment
There was a problem hiding this comment.
A few minor nits, but looks good to me!
849b8da to
9a27d93
Compare
What changed?
Add support for stand alone nexus operations to the CLI.
Checklist
Stability
-o json/-o jsonl) are treated as breaking changesDesign
temporal <noun> <verb>structure (e.g.temporal workflow start)--search-attribute, bad:--index-field)(Experimental)incommands.yamlHelp text (see style guide at the top of
commands.yaml)--namespace, not-n), one flag per lineYourXxxform (YourWorkflowId,YourNamespace)Behavior
Tests
SharedServerSuite)func TestXxx) where applicableManual tests
Setup
Happy path
Error case
Composition — start an operation, then fetch its result, then list/describe: