Skip to content
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

Public API: Acknowledge & Resolve actions #3108

Merged
merged 10 commits into from
Oct 5, 2023
Merged

Conversation

vstpme
Copy link
Member

@vstpme vstpme commented Oct 3, 2023

What this PR does

Makes it possible to acknowledge/unacknowledge and resolve/unresolve alert groups via public API, and makes sure these actions are reflected properly in the alert group timeline.

Demo

curl --request POST \
     --header "Authorization: TOKEN" \
     http://localhost:8080/api/v1/alert_groups/IQMHLV8INB24N/resolve
Screenshot 2023-10-04 at 16 05 27

Which issue(s) this PR fixes

#3051

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

operations = [
migrations.AddField(
model_name='alertgrouplogrecord',
name='action_source',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saving action_source to DB so it's possible to differentiate between actions performed via public API vs. other "action sources"

Comment on lines +264 to +265
if self.action_source == ActionSource.API:
author_name = "API"
Copy link
Member Author

@vstpme vstpme Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it so API actions are shown as "... by API" in the alert group timeline

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be sure, we would still have the API token owner set as user in the record, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct

@@ -479,7 +487,7 @@ def rendered_log_line_action(self, for_slack=False, html=False, substitute_autho
f"because it is already attached or resolved."
)
elif self.type == AlertGroupLogRecord.TYPE_RESOLVED:
result += f"alert group resolved {f'by {author_name}'if author_name else ''}"
result += f"resolved {f'by {author_name}'if author_name else ''}"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"alert group resolved by ..." -> just "resolved by ..." (all other actions omit the "alert group" part, so making this a bit more consistent)

@@ -112,3 +114,74 @@ def destroy(self, request, *args, **kwargs):
wipe.apply_async((instance.pk, request.user.pk))

return Response(status=status.HTTP_204_NO_CONTENT)

@action(methods=["post"], detail=True)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These API actions mostly duplicate the internal API logic. I think these alert group state changes should be eventually refactored to be a proper FSM that can be reused in both APIs, but I want to leave this outside of the scope of this PR/issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@vstpme vstpme marked this pull request as ready for review October 4, 2023 15:10
@vstpme vstpme requested a review from a team as a code owner October 4, 2023 15:10
@vstpme vstpme requested a review from a team October 4, 2023 15:10
Copy link
Contributor

@matiasb matiasb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -112,3 +114,74 @@ def destroy(self, request, *args, **kwargs):
wipe.apply_async((instance.pk, request.user.pk))

return Response(status=status.HTTP_204_NO_CONTENT)

@action(methods=["post"], detail=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Comment on lines +264 to +265
if self.action_source == ActionSource.API:
author_name = "API"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be sure, we would still have the API token owner set as user in the record, right?

@vstpme vstpme merged commit a727450 into dev Oct 5, 2023
@vstpme vstpme deleted the vadimkerr/ack-resolve-api branch October 5, 2023 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants