-
Notifications
You must be signed in to change notification settings - Fork 33
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
Expose LastFailure and LastResult from WorkflowInfo #136
Conversation
/// <param name="LastFailure">Failure if this workflow run is a continuation of a failure.</param> | ||
/// <param name="LastResult">Successful result if this workflow is a continuation of a success.</param> |
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.
"workflow run was continued-as-new from a previous run which ended in a..."
Is a bit more clear I think
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.
This also available for successive cron and schedule workflow runs (technically it's exposed in the start API, we just don't expose in SDKs)
var lastFailure = start.ContinuedFailure == null ? | ||
null : failureConverter.ToException(start.ContinuedFailure, PayloadConverter); | ||
var lastResult = start.LastCompletionResult?.Payloads_.Select(v => new RawValue(v)).ToArray(); |
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.
Are codecs automatically applied to this? I think you used some reflection magic somewhere to ensure we apply codecs to anything over the lang-core bridge.
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.
Yes, codecs are applied outside of this and there's a test to confirm no payloads slip by (even if they were just added to proto)
@@ -32,6 +35,8 @@ public record WorkflowInfo( | |||
string? CronSchedule, | |||
TimeSpan? ExecutionTimeout, | |||
IReadOnlyDictionary<string, Api.Common.V1.Payload>? Headers, | |||
Exception? LastFailure, | |||
IReadOnlyCollection<IRawValue>? LastResult, |
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.
nit: there's likely never be more than one result, none of our SDKs support returning more than one result.
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.
Can you also please test last result?
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.
A collection of one is ok. Another benefit is that this will be empty but not null when there's no result but there was a last run.
Can you also please test last result?
Yes, I figured it may be hard to do in a timely manner with schedules, but maybe continue as new works. I will look to see how TS tested this value.
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.
Added test
ee98ac8
to
d5e412b
Compare
What was changed
Expose
LastFailure
andLastResult
fromWorkflowInfo
Checklist