feat(aci): issue alert action migration util for slack #82505
Conversation
github-actions
bot
added
the
Scope: Backend
| """ | ||
|
|
||
| # # If we have a specific blob type, we need to sanitize the action data to the blob type | ||
| if action_type == Action.Type.SLACK: |
There was a problem hiding this comment.
i tried out a mapping for this from action_type to the dataclass, but i liked having more explicit typing here since otherwise i think i would have to put a generic type.
There was a problem hiding this comment.
open to better ideas
| Builds a SlackDataBlob from the action data. | ||
| Only includes the keys that are not None. | ||
| """ | ||
| return SlackDataBlob( |
There was a problem hiding this comment.
narrowing the type that can exist in the data blob, should have something similar for most integrations, but might be a bit more difficult (& have diminishing gains) for sentry apps and such
| } | ||
|
|
||
|
|
||
| def build_notification_actions_from_rule_data_actions( |
There was a problem hiding this comment.
main method that will be used
| notification_action = Action( | ||
| type=action_type, | ||
| data=( | ||
| # If the target type is specific, sanitize the action data |
There was a problem hiding this comment.
need to clean this portion up
| k: v | ||
| for k, v in action.items() | ||
| if k | ||
| not in [ |
There was a problem hiding this comment.
convoluted, but basically popping keys we save elsewhere or don't want to save
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## ceorourke/dual-create-action-datacondition #82505 +/- ##
==============================================================================
- Coverage 80.43% 80.43% -0.01%
==============================================================================
Files 7314 7315 +1
Lines 322644 322661 +17
Branches 21039 21039
==============================================================================
+ Hits 259516 259529 +13
- Misses 62724 62728 +4
Partials 404 404 |
| - uuid: maps to action id | ||
| - id: maps to action type | ||
| """ | ||
| EXCLUDED_ACTION_DATA_KEYS = ["uuid", "id"] |
There was a problem hiding this comment.
currently not saving the uuid since we are going to replace with action.id, but i am also considering saving it in the data blob incase something bad happens and we need it to recover? 🤔
There was a problem hiding this comment.
i assume we are populating all the fields for the action in case a workflow is used for the error detector and/or metric detectors?
my main concern is that we should be really careful about assuming things exist in dictionaries and have a fallback plan if things don't exist
| # For email, the target type is user | ||
| # For sentry app, the target type is sentry app | ||
| # FWIW, we don't use target type for issue alerts | ||
| target_type = ACTION_TYPE_2_TARGET_TYPE_RULE_REGISTRY.get(action_type) |
There was a problem hiding this comment.
we store this because the same workflow can be attached to metric alerts? what happens if the action_type doesn't exist in ACTION_TYPE_2_TARGET_TYPE_RULE_REGISTRY? maybe you could enforce that everything exists in a test?
There was a problem hiding this comment.
we are planning to prevent the user from attaching issuer alert actions to metric alert actions (and vice versa) until we work on notification platform and making out notification handlers alert type agnostic.
until then, we can assume these actions we migrate will only be used for the errors detector we are creating.
technically, because of that, we don't even need to maintain this column and since its nullable, i remove it.
| # Get the target_identifier if it exists | ||
| target_identifier_key = ACTION_TYPE_2_TARGET_IDENTIFIER_KEY.get(action_type) | ||
| if target_identifier_key is not None: | ||
| target_identifier = action.get(target_identifier_key) |
There was a problem hiding this comment.
what happens if this doesn't exist?
There was a problem hiding this comment.
this field doesn't have to exist. it won't exist for any ticketing integrations.
for context, we are storing the info on these fields so its easier to query for the frontend.
| # Otherwise, use the action data as is | ||
| sanitize_to_action(action, action_type) | ||
| if target_type == ActionTarget.SPECIFIC | ||
| else action |
There was a problem hiding this comment.
we put the entire old blob in here?
| notification_action.save() | ||
|
|
||
| notification_actions.append(notification_action) |
There was a problem hiding this comment.
instead of saving immediately, you could create a list of unsaved objects and then call bulk_create https://docs.djangoproject.com/en/5.1/ref/models/querysets/#bulk-create, but this won't call the save() method
this pr starts the implementation of migration util for issue alert migration to aci. i realized the pr would become a bit annoying to revidew, so wanted to get this out the main portion for some 👀 .
added some comments to the code for context.