Add support for Standalone Nexus Operations#1046
Conversation
Evanthx
left a comment
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?
| if cctx.JSONShorthandPayloads { | ||
| var valuePtr any | ||
| if err = converter.GetDefaultDataConverter().FromPayload(result, &valuePtr); err != nil { | ||
| return fmt.Errorf("failed decoding result: %w", err) |
There was a problem hiding this comment.
Might add a little more context here - ie, Nexus operation completed, but failed decoding result, something like that?
Same for the other errors being returned below.
| } | ||
| 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.
| if err != nil { | ||
| return fmt.Errorf("failed marshaling failure: %w", err) | ||
| } | ||
| _ = cctx.Printer.PrintStructured(struct { |
There was a problem hiding this comment.
printNexusOperationResult gets the error from PrintStructured and returns it (line 208), this ignores any error values from PrintStructured and returns nil. Should they both behave the same way?
| } | ||
|
|
||
| func (s *SharedServerSuite) TestNexusOperationCancel() { | ||
| endpointName := "test-ep-" + uuid.NewString()[:8] |
There was a problem hiding this comment.
TestNexusOperationCancel and TestNexusOperationTerminate both set up endpoints - can that be combined into a helper function using setupNexusEndpointAndWorker?
VegetarianOrc
left a comment
VegetarianOrc
left a comment
There was a problem hiding this comment.
A few minor nits, but looks good to me!
3 participants
What was changed
Add support for Standalone Nexus Operations
Why?
So users can interact with them through the CLI
Checklist
Closes
How was this tested:
Any docs updates needed?