Add high level workflow describe #1924
Conversation
client/client.go
Outdated
| // - serviceerror.Internal | ||
| // - serviceerror.Unavailable | ||
| // - serviceerror.NotFound | ||
| DescribeWorkflow(ctx context.Context, workflowID, runID string) (*DescribeWorkflowExecution, error) |
There was a problem hiding this comment.
Returned a pointer here for consistency with some other APIs
| @@ -901,6 +917,27 @@ type QueryWorkflowWithOptionsResponse struct { | |||
| QueryRejected *querypb.QueryRejected | |||
| } | |||
|
|
|||
| type WorkflowExecutionMetadata struct { | |||
There was a problem hiding this comment.
I picked fields to expsoe here based of what dotnet exposed https://github.com/temporalio/sdk-dotnet/blob/main/src/Temporalio/Client/WorkflowExecutionDescription.cs#L10
There was a problem hiding this comment.
Works for me, though not the biggest fan of the term "metadata", but I know it's what Java uses, so up to you. I figure one day we'll deprecate all the bad forms of "list" and have a new "list" call that returns iter.Seq[WorkflowExecutionMetadata], but not sure if that affects your name choice.
There was a problem hiding this comment.
If you have a different name I am open to suggestions, when we add list I was going to add another type that would also embedded WorkflowExecutionMetadata
There was a problem hiding this comment.
Yeah, since WorkflowExecution is already in use elsewhere, this is fine. Not sure at list time we need another type since list contents should be a subset of describe, but can discuss at that time.
client/client.go
Outdated
| // - serviceerror.Internal | ||
| // - serviceerror.Unavailable | ||
| // - serviceerror.NotFound | ||
| DescribeWorkflow(ctx context.Context, workflowID, runID string) (*DescribeWorkflowExecution, error) |
There was a problem hiding this comment.
Do we want to accept DescribeWorkflowOptions for future proofing?
There was a problem hiding this comment.
I was thinking we would add DescribeWorkflowWithOptions if we ever add more options here
| @@ -1133,6 +1136,16 @@ type ( | |||
| // - serviceerror.NotFound | |||
| DescribeWorkflowExecution(ctx context.Context, workflowID, runID string) (*workflowservice.DescribeWorkflowExecutionResponse, error) | |||
There was a problem hiding this comment.
Do we want to discourage/deprecate this in favor of either using DescribeWorkflow or WorkflowService().DescribeWorkflowExecution?
There was a problem hiding this comment.
I wasn't going to as part of this PR, I think if we want to deprecate these calls that just directly call the WorkflowService() we should deprecate them all.
There was a problem hiding this comment.
👍 Would support deprecating any high-level ones that don't really provide value over direct gRPC call, but definitely for this one we should deprecate the other one (can be separate PR)
| @@ -901,6 +917,27 @@ type QueryWorkflowWithOptionsResponse struct { | |||
| QueryRejected *querypb.QueryRejected | |||
| } | |||
|
|
|||
| type WorkflowExecutionMetadata struct { | |||
There was a problem hiding this comment.
Works for me, though not the biggest fan of the term "metadata", but I know it's what Java uses, so up to you. I figure one day we'll deprecate all the bad forms of "list" and have a new "list" call that returns iter.Seq[WorkflowExecutionMetadata], but not sure if that affects your name choice.
| // GetStaticSummary returns the summary set on workflow start. | ||
| // | ||
| // NOTE: Experimental | ||
| func (w *WorkflowExecutionDescription) GetStaticSummary() (string, error) { |
There was a problem hiding this comment.
Arguably could be worth memoizing e.g. behind a sync.Once, but no strong opinion
client/client.go
Outdated
| @@ -269,6 +269,9 @@ type ( | |||
| // QueryWorkflowWithOptionsResponse defines the response to QueryWorkflowWithOptions. | |||
| QueryWorkflowWithOptionsResponse = internal.QueryWorkflowWithOptionsResponse | |||
|
|
|||
| // DescribeWorkflowExecution defines the response to DescribeWorkflow. | |||
| DescribeWorkflowExecution = internal.WorkflowExecutionDescription | |||
There was a problem hiding this comment.
Though you don't technically have to, I think you should also export the embedded field type
There was a problem hiding this comment.
What is the use case for the exported embedded field type?
There was a problem hiding this comment.
For tests, people often appreciate the ability to easily instantiate the objects our clients can return. In this case, you have to instantiate and then set one field at a time as separate statements because you can't instantiate this embedded field explicitly. I think it's a reasonable rule that "every struct that is exported should also have its exported fields' types exported" (unless there's a good reason to have a non-exported interface, which is possible but rare)
Add high level workflow describe
close #1667