-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feat(workflow_engine): workflow and action serializers #83146
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #83146 +/- ##
==========================================
- Coverage 87.58% 87.57% -0.01%
==========================================
Files 9438 9450 +12
Lines 536582 537373 +791
Branches 21115 21115
==========================================
+ Hits 469948 470612 +664
- Misses 66275 66402 +127
Partials 359 359 |
return attrs | ||
|
||
def serialize(self, obj: Workflow, attrs: Mapping[str, Any], user, **kwargs) -> dict[str, Any]: | ||
# WHAT TO DO ABOUT CONFIG? |
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 is a good question 🤔 Cathy added enforcement of the schemas in https://github.com/getsentry/sentry/pull/81979/files and I was working on defining a schema for the detector config here that I need to get back to but we haven't made others yet afaik. Wanna bring it up in office hours tomorrow?
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.
haven't quite been able to finish reviewing (not 100% sure about where you'd want to handle the workflow actions, for example. for that you'll probably want to do some research around validators to see the best practices), but wanted to make sure you had this feedback sooner rather than later.
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.
Thanks for addressing the feedback! From a frontend API consumer perspective, this looks good.
Implement the serializer for the
Action
andWorkflow
APIs. I'm thinking that I will make a separate PR to alphabetize these serializers after this merges.