Add reset reapply to batch reset command #700
Conversation
|
|
|
Hi, thanks for sending this over! Sorry we haven't gotten to it quite yet, the team's been rather busy, but one of us will try to look at it soon. Just wanted to make sure you knew we've seen this and it hasn't gotten lost. :) |
There was a problem hiding this comment.
Nice, thanks a lot @jamipouchi! I made a couple of minor comments but, do you have time to add the test coverage for this?
| } | ||
| reapplyExcludes, reapplyType, err := getResetReapplyAndExcludeTypes(c.ReapplyExclude.Values, c.ReapplyType.Value) | ||
| if err != nil { | ||
| return fmt.Errorf("getting reset reapply and exclude types failed: %w", err) |
There was a problem hiding this comment.
I think the prefix here ("getting reset reapply and...") isn't quite right for a user-facing error message, since it's making reference to an operation that's essentially something CLI internals are doing. I'm thinking just returning err will give the desired error message.
There was a problem hiding this comment.
Yup, agree.
Did it cuz it was similar to the error above getting reset event... error. I have also changed that one.
I am going on vacation tomorrow until next wednesday, and then I'll pick up work. |
Hi @dandavison, just added the tests for the batch reset using reapply exclude. Also added first/last workflow task, and one making sure that only wfs matching the query were reset. |
There was a problem hiding this comment.
Thanks a lot for this work @jamipouchi, including the careful tests adding coverage that was missing before. My manual testing didn't find any problems.
Fix a flaky test from #700 Prior to this commit, failures of go test -v -count 1 -run '^TestSharedServerSuite/TestWorkflow_ResetBatch_ExcludeSignal$' were fairly frequent.
What was changed
Previously reset reapply options were ignored for batch reset, and only used for single workflow reset command.
This pr also adds reset reapply options to batch reset command.
Why?
I have found myself needing to batch-reset workflows without reapplying signals, but unable to do so with the temporal cli. It is possible via sdk code and via tctl, which makes it look as someone forgot to add it.
Checklist
Closes
How was this tested:
I have tested this with my usecase and worked great.
I have not added any tests. Probably some batch-reset tests are needed, but no logic has been added ( I have reused the method that was used for single reset, so there's not much change it's wrong )