-
Notifications
You must be signed in to change notification settings - Fork 216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for query parameter when listing schedules #1556
Conversation
Guard for eventual consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (though Eventually
has a bit of a poor impl on the testify
side, but probably fine here). Will leave open a bit to confirm @Quinn-With-Two-Ns has nothing to add.
Also, can you confirm you have privileges to "merge" this? If not, we can merge for you.
@@ -199,6 +199,7 @@ func (sc *scheduleClient) List(ctx context.Context, options ScheduleListOptions) | |||
Namespace: sc.workflowClient.namespace, | |||
MaximumPageSize: int32(options.PageSize), | |||
NextPageToken: nextToken, | |||
Query: options.Query, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any issue on the server side when passing both a Query
string and a NextPageToken
? I don't think we do this anywhere else in the Go SDK and want to make sure the server does the correct thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ListWorkflowExecutions? The SDK doesn't call it but it exposes it to be used that way. IOW the token does not encapsulate the query, you still need the query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the SDK doesn't wrap ListWorkflowExecutions
so I wasn't sure on the contract.
What was changed
Add support for query parameter when listing schedules
Checklist
Closes Support "query" when listing schedules #1539
How was this tested:
Added integration test cases