Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request removes exposed internal protobuf types from the Go SDK by wrapping them with SDK-native types. This supports PR #2913's proto naming convention changes and ensures the SDK provides a stable API that doesn't expose internal implementation details.
Changes:
- Replaced direct usage of internal proto types with new SDK wrapper types (ResourceType, ResourceEventType, WorkflowRunEventType, workflowEvent, workflowRunEvent, StepRunResult)
- Added conversion functions to transform internal proto events to SDK wrapper types
- Added comprehensive test coverage for the new conversion functions
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/client/workflow.go | Updated WorkflowResult to use the new workflowRunEvent wrapper type instead of internal proto type; removed internal proto import |
| pkg/client/listener.go | Introduced new SDK wrapper types and enums; added conversion functions workflowEventToDeprecatedWorkflowEvent and workflowRunEventToDeprecatedWorkflowRunEvent; integrated conversions in event handler paths |
| pkg/client/listener_test.go | Added comprehensive test coverage for both conversion functions including success cases, nil timestamp handling, empty events, and events with results |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func workflowEventToDeprecatedWorkflowEvent(event *dispatchercontracts.WorkflowEvent) (*workflowEvent, error) { | ||
| result := &workflowEvent{ | ||
| WorkflowRunId: event.WorkflowRunId, | ||
| ResourceType: ResourceType(event.ResourceType), | ||
| EventType: ResourceEventType(event.EventType), | ||
| ResourceId: event.ResourceId, | ||
| EventPayload: event.EventPayload, | ||
| Hangup: event.Hangup, | ||
| StepRetries: event.StepRetries, | ||
| RetryCount: event.RetryCount, | ||
| EventIndex: event.EventIndex, | ||
| } | ||
|
|
||
| if event.EventTimestamp != nil { | ||
| t := event.EventTimestamp.AsTime() | ||
| result.EventTimestamp = &t | ||
| } | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
The conversion functions always return nil as the error value. Since these functions currently cannot fail, consider either:
- Removing the error return if no future errors are anticipated:
func workflowEventToDeprecatedWorkflowEvent(event *dispatchercontracts.WorkflowEvent) *workflowEvent - If the error return is for future extensibility, add a comment explaining this
This would simplify the calling code by removing unnecessary error checks at lines 477-479 and 393-395.
| func TestWorkflowEventToDeprecatedWorkflowEvent_Success(t *testing.T) { | ||
| // Verifies successful conversion of WorkflowEvent to deprecated WorkflowEvent | ||
|
|
||
| workflowRunId := "test-workflow-run-123" | ||
| stepId := "test-step-456" | ||
| eventPayload := "test-payload" | ||
|
|
||
| event := &dispatchercontracts.WorkflowEvent{ | ||
| WorkflowRunId: workflowRunId, | ||
| ResourceType: dispatchercontracts.ResourceType_RESOURCE_TYPE_STEP_RUN, | ||
| ResourceId: stepId, | ||
| EventType: dispatchercontracts.ResourceEventType_RESOURCE_EVENT_TYPE_STARTED, | ||
| EventPayload: eventPayload, | ||
| EventTimestamp: timestamppb.New(time.Unix(1234567890, 123456789)), | ||
| } | ||
|
|
||
| deprecated, err := workflowEventToDeprecatedWorkflowEvent(event) | ||
|
|
||
| require.NoError(t, err, "conversion should succeed") | ||
| require.NotNil(t, deprecated, "deprecated event should not be nil") | ||
|
|
||
| // Verify key fields are preserved | ||
| assert.Equal(t, workflowRunId, deprecated.WorkflowRunId) | ||
| assert.Equal(t, stepId, deprecated.ResourceId) | ||
| assert.Equal(t, eventPayload, deprecated.EventPayload) | ||
| } |
There was a problem hiding this comment.
The test verifies field values but doesn't assert that the enum types (ResourceType and EventType) are correctly converted. Consider adding assertions like:
assert.Equal(t, ResourceType_RESOURCE_TYPE_STEP_RUN, deprecated.ResourceType)
assert.Equal(t, ResourceEventType_RESOURCE_EVENT_TYPE_STARTED, deprecated.EventType)
This would ensure the enum casting at lines 563-564 in listener.go works correctly.
| func TestWorkflowRunEventToDeprecatedWorkflowRunEvent_Success(t *testing.T) { | ||
| // Verifies successful conversion of WorkflowRunEvent to deprecated WorkflowRunEvent | ||
|
|
||
| workflowRunId := "test-workflow-run-789" | ||
|
|
||
| event := &dispatchercontracts.WorkflowRunEvent{ | ||
| WorkflowRunId: workflowRunId, | ||
| EventType: dispatchercontracts.WorkflowRunEventType_WORKFLOW_RUN_EVENT_TYPE_FINISHED, | ||
| EventTimestamp: timestamppb.New(time.Unix(9876543210, 987654321)), | ||
| } | ||
|
|
||
| deprecated, err := workflowRunEventToDeprecatedWorkflowRunEvent(event) | ||
|
|
||
| require.NoError(t, err, "conversion should succeed") | ||
| require.NotNil(t, deprecated, "deprecated event should not be nil") | ||
|
|
||
| // Verify key fields are preserved | ||
| assert.Equal(t, workflowRunId, deprecated.WorkflowRunId) | ||
| assert.NotNil(t, deprecated.EventTimestamp) | ||
| } |
There was a problem hiding this comment.
The test verifies basic fields but doesn't assert that the EventType is correctly converted. Consider adding an assertion like:
assert.Equal(t, WorkflowRunEventType_WORKFLOW_RUN_EVENT_TYPE_FINISHED, deprecated.EventType)
This would ensure the enum casting at line 584 in listener.go works correctly.
| // ResourceType represents the type of resource | ||
| type ResourceType int32 | ||
|
|
||
| const ( | ||
| ResourceType_RESOURCE_TYPE_UNKNOWN ResourceType = 0 | ||
| ResourceType_RESOURCE_TYPE_STEP_RUN ResourceType = 1 | ||
| ResourceType_RESOURCE_TYPE_WORKFLOW_RUN ResourceType = 2 | ||
| ) | ||
|
|
||
| // ResourceEventType represents the type of event | ||
| type ResourceEventType int32 | ||
|
|
||
| const ( | ||
| ResourceEventType_RESOURCE_EVENT_TYPE_UNKNOWN ResourceEventType = 0 | ||
| ResourceEventType_RESOURCE_EVENT_TYPE_STARTED ResourceEventType = 1 | ||
| ResourceEventType_RESOURCE_EVENT_TYPE_COMPLETED ResourceEventType = 2 | ||
| ResourceEventType_RESOURCE_EVENT_TYPE_FAILED ResourceEventType = 3 | ||
| ResourceEventType_RESOURCE_EVENT_TYPE_CANCELLED ResourceEventType = 4 | ||
| ResourceEventType_RESOURCE_EVENT_TYPE_TIMED_OUT ResourceEventType = 5 | ||
| ResourceEventType_RESOURCE_EVENT_TYPE_STREAM ResourceEventType = 6 | ||
| ) | ||
|
|
||
| // WorkflowRunEventType represents the type of workflow run event | ||
| type WorkflowRunEventType int32 | ||
|
|
||
| const ( | ||
| WorkflowRunEventType_WORKFLOW_RUN_EVENT_TYPE_FINISHED WorkflowRunEventType = 0 | ||
| ) | ||
|
|
||
| // workflowEvent is the internal representation of a workflow event | ||
| type workflowEvent struct { | ||
| EventTimestamp *time.Time | ||
| StepRetries *int32 | ||
| RetryCount *int32 | ||
| EventIndex *int64 | ||
| WorkflowRunId string | ||
| ResourceId string | ||
| EventPayload string | ||
| ResourceType ResourceType | ||
| EventType ResourceEventType | ||
| Hangup bool | ||
| } | ||
|
|
||
| // StepRunResult represents the result of a step run | ||
| type StepRunResult struct { | ||
| Error *string | ||
| Output *string | ||
| StepRunId string | ||
| StepReadableId string | ||
| JobRunId string | ||
| } | ||
|
|
||
| // workflowRunEvent is the internal representation of a workflow run event | ||
| type workflowRunEvent struct { | ||
| EventTimestamp *time.Time | ||
| WorkflowRunId string | ||
| Results []*StepRunResult | ||
| EventType WorkflowRunEventType | ||
| } |
There was a problem hiding this comment.
The new public types (ResourceType, ResourceEventType, WorkflowRunEventType, workflowEvent, workflowRunEvent, and StepRunResult) lack documentation comments. While some have minimal comments (e.g., "ResourceType represents the type of resource"), they should include more detail about their purpose and how they relate to the SDK's API design.
Consider adding package-level documentation or more detailed comments explaining that these types wrap internal proto definitions to provide a stable public API, especially since this is part of addressing issue #2913.
Description
Removes a few instances of exposed internal protos in the Go SDK, to try and get #2913 over the line.
Type of change