Fix should-be-ignored workflow options update #966
Conversation
| @@ -839,6 +839,12 @@ impl WorkflowMachines { | |||
| if event.is_final_wf_execution_event() { | |||
| self.have_seen_terminal_event = true; | |||
| } | |||
| // Currently nothing of interest to the SDK in this event | |||
| if event.event_type() == EventType::WorkflowExecutionOptionsUpdated | |||
There was a problem hiding this comment.
Why only this event? Should we keep a list of events we do process and be willing to ignore all others when this flag is set? I am worried if another event gets added and we regen protos this same thing will happen if we don't come and add to this conditional.
There was a problem hiding this comment.
Well I kinda want to err on the side of being explicit. The reason it works the way it does now is if there is some event that Core knows about, we should explicitly be OK with ignoring it...
However what this problem revealed is we don't have a great way to learn about new events that fit into this category, and decide to ignore them. I could potentially add some kind of test that ensures all known events are explicitly categorized somehow
There was a problem hiding this comment.
Can you clarify what is meant by "event that Core knows about"? Is it any event that happens to appear in proto (requiring us to add it to a match clause I see in sdk-core-protos). I'd argue the presence of an event in protos and in that match clause isn't really the same as Core knowing about it meaningfully.
I could potentially add some kind of test that ensures all known events are explicitly categorized somehow
👍 Or somehow change that one big match clause that we must update on each event to also include whether it is currently ignored atm (may require restructuring since it is just for getting event type right now). At least that way each new event has to be clear whether it's handled or not.
No description provided.