chore(aci): enforce config schema without subclassing #81979
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #81979 +/- ##
=======================================
Coverage 80.35% 80.36%
=======================================
Files 7279 7280 +1
Lines 321322 321394 +72
Branches 20957 20957
=======================================
+ Hits 258207 258284 +77
+ Misses 62701 62696 -5
Partials 414 414 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cathteng
force-pushed
the
cathy/aci/enforce-schema-sans-proxy
branch
from
ee6b7bd to
c49a9e8
Compare
| @@ -121,9 +121,16 @@ def test_state_results_multi_group(self, mock_produce_occurrence_to_kafka): | |||
| ) | |||
|
|
|||
| def test_no_issue_type(self): | |||
| detector = self.create_detector(type="invalid slug") | |||
There was a problem hiding this comment.
with this PR we can no longer create a detector with an invalid slug, so substituting with creating a valid detector and mocking that the GroupType gets deleted
src/sentry/issues/grouptype.py
Outdated
| detector_handler: type[DetectorHandler] | None = None | ||
| detector_validator: type[BaseGroupTypeDetectorValidator] | None = None | ||
| detector_config_schema: dict[str, Any] | None = None |
There was a problem hiding this comment.
It's starting to feel like we just need something like DetectorConfig that encapsulates all of these. Probably something for a separate pr
| # TODO: fill in | ||
| return {} |
There was a problem hiding this comment.
Is the plan to use subclasses here? Won't we run into the same problem we proxy models?
There was a problem hiding this comment.
there isn't a type on workflow so all of them should have the same config schema
There was a problem hiding this comment.
I'm not too clear on what these configs are going to be... If they're consistent for all Workflows this is fine though
| config_schema = group_type.detector_config_schema | ||
| instance.validate_config(config_schema) |
There was a problem hiding this comment.
We should probably also check that detector_config_schema is set too
There was a problem hiding this comment.
this is set to an empty dict by default in GroupType, do we need anything else?
There was a problem hiding this comment.
Oh, I guess that's fine... One problem with defaulting it to an empty dict is that if people forget to add it then they won't get an error, but this works as is
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
We will have many detectors of different
typeswith different config schemas that need to be enforced. Instead of subclassing and using a Django proxy model we can link the config schema toGroupTypeand enforce it via a pre-save signal.Workflowssimilarly have a config schema to enforce but all of them have the same schema. So we use a different pre-save signal function for it and a hardcoded schema.