Propagated history changes#1833
Conversation
…low status Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…dded missing unit tests. Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1833 +/- ##
=======================================
Coverage 63.46% 63.46%
=======================================
Files 312 312
Lines 9279 9279
Branches 1101 1101
=======================================
Hits 5889 5889
Misses 3130 3130
Partials 260 260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
| /// <param name="result">When this method returns <see langword="true"/>, the last matching workflow entry; otherwise <see langword="null"/>.</param> | ||
| /// <returns><see langword="true"/> if a matching entry was found; otherwise <see langword="false"/>.</returns> | ||
| public bool TryGetLastWorkflowByName(string name, [NotNullWhen(true)] out PropagatedHistoryEntry? result) | ||
| public bool TryGetLastWorkflowEventByName(string name, [NotNullWhen(true)] out PropagatedHistoryEvent? result) |
There was a problem hiding this comment.
Why Try? All the other sdks have GetLastWorkflowEventByName without Try. Is this more of a dotnet consistency thing?
There was a problem hiding this comment.
A null in a response can mean so many things. Taking a recent example from the state store - if you query a value from state and you get a null back, does that mean that the value didn't exist or that it did and it is null? A TryGet answers this definitively.
Especially as these are not asynchronous operations, I'd rather return a clear boolean indicating if getting the value(s) worked or not and include a payload with the corresponding result if it did.
| /// </remarks> | ||
| public sealed record PropagatedHistoryWorkflowResult( | ||
| string Name, | ||
| PropagatedHistoryTaskStatus Status, |
There was a problem hiding this comment.
Same as above - could we rename to something like WorkflowStatus? PropagatedHistoryTaskStatus on a workflow result is a bit confusing too, since task usually refers to activities, but end users don't even know that. Users just care about the status, not the propagated history internals.
There was a problem hiding this comment.
They're the same type as above - just refactored to PropagatedHistoryStatus so it's generically used for either one.
| /// <see cref="PropagatedHistory"/>. Use <see cref="TryGetLastActivityByName"/> and | ||
| /// <see cref="TryGetLastChildWorkflowByName"/> to look up specific items in this entry; |
There was a problem hiding this comment.
Maybe I missed this in our DMs, we do support a top level GetLastWorkflowByName/GetWorkflowsByName in the go-sdk. This returns a WorkflowResult. In dotnet you have TryGetLastWorkflowEventByName/GetEventsByWorkflowName returning PropagatedHistoryEvent.
In go, off of WorkflowResult.GetLastChildWorkflowByName/GetChildWorkflowsByName I return the ChildWorkflowResult. In dotnet child is dropped. Can we re-add that on PropagatedHistoryEvent?
There was a problem hiding this comment.
I'm not super worried about having a bunch of variations of the filters. I expose all the events in a property, so if there's some special filter a developer wishes to apply, they can trivially add it themselves.
Also, yes, .NET has filters on the list of all propagated history events to return a single PropagatedHistoryEvent as you noted. But this also has filters within it for the specific workflows and activities you cite, so I think the bases are probably already covered here.
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
… removed method Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
2 participants
Description
Making a few tweaks + added unit tests to the implementation to support the propagated history implementation.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: