-
Notifications
You must be signed in to change notification settings - Fork 299
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
Refactor AlertGroup.slack_message
#2957
Conversation
# Conflicts: # CHANGELOG.md
operations = [ | ||
common.migrations.remove_field.RemoveFieldState( | ||
model_name='AlertGroup', | ||
name='slack_message', | ||
), | ||
] |
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.
Remove AlertGroup.slack_message
so the only FK left is SlackMessage.alert_group
if e.response["error"] == "message_not_found": | ||
logger.info(f"message_not_found for alert_group {alert_group.pk}, trying to post new message") | ||
result = self._slack_client.api_call( | ||
"chat.postMessage", channel=slack_message.channel_id, attachments=attachments, blocks=blocks | ||
) | ||
slack_message_updated = SlackMessage( | ||
slack_id=result["ts"], | ||
organization=slack_message.organization, | ||
_slack_team_identity=slack_message.slack_team_identity, | ||
channel_id=slack_message.channel_id, | ||
alert_group=alert_group, | ||
) | ||
slack_message_updated.save() | ||
alert_group.slack_message = slack_message_updated | ||
alert_group.save(update_fields=["slack_message"]) | ||
logger.info(f"Message has been posted for alert_group {alert_group.pk}") |
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.
Removing this logic completely because it's hard to maintain with the new FK structure.
I think it makes more sense to just ignore deleted AG Slack messages instead of trying to post a new message (why post a message one more time if the user already decided they don't want to see it and deleted it?)
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.
LGTM.
I see you are removing the field following the guideline 👍 , so I guess there will be a PR later removing the column from the DB, right?
@matiasb yes, the DB field will be dropped in a future release. |
What this PR does
Refactors the foreign key relationship between
AlertGroup
andSlackMessage
models (see https://github.com/grafana/oncall-private/issues/1867 for more info).Which issue(s) this PR fixes
https://github.com/grafana/oncall-private/issues/1867
Checklist
pr:no public docs
PR label added if not required)CHANGELOG.md
updated (orpr:no changelog
PR label added if not required)