-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(aci): use singular preferred detector in workflow processing round 2 #105865
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
Conversation
kcons
left a comment
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.
All optional notes or random asides, but I'd like to be sure we're not logging an exception every time we choose issue stream as primary.
| event_data: WorkflowEventData, detector: Detector | None = None | ||
| def get_detectors_for_event_data( | ||
| event_data: WorkflowEventData, | ||
| detector: Detector | None = None, |
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.
idle thought: this should probably be known_preferred for clarity. But feel free to ignore this thought.
| mock_logger.info.assert_any_call( | ||
| "workflow_engine.process_workflows.evaluation.workflows.not_triggered", | ||
| extra={ | ||
| "event_id": self.event.event_id, |
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.
I really wish there were a less awkward way to assert a small subset of possible keys so that any change to the logged extras doesn't break this test.
Well, abstractions for that do exist, they're just not idiomatic, so maybe I'll hack on that at some point.
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.
I'm familiar with using ANY but that's it
#105674 round 2 and tests the behavior that broke
This is safe to merge now that we don't have any non error, non metric issues going through workflow engine + the issue stream detector is currently disabled.