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

fix(aci milestone 3): update the resolve datacondition when we update triggers #84202

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

mifu67
Copy link
Contributor

@mifu67 mifu67 commented Jan 28, 2025

If we update the a legacy alert rule's triggers, then we may have to update the resolution threshold on the resolve detector trigger (if no resolution threshold was specified on the alert rule). Create this update helper and add it to the serializer.

To facilitate this, I also updated the way we automatically get the resolution threshold. Now, instead of checking for the number of data conditions, we explicitly check for the critical and (optionally) warning data conditions, which we know have been previously dual written.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 28, 2025
@mifu67 mifu67 marked this pull request as ready for review February 5, 2025 21:23
@mifu67 mifu67 requested review from a team as code owners February 5, 2025 21:23
@mifu67 mifu67 requested a review from ceorourke February 5, 2025 21:23
if critical_data_condition is None:
logger.error(
"no data conditions exist for detector data condition group",
"no critical or warning data conditions exist for detector data condition group",
extra={"detector_data_condition_group": detector_triggers},
)
return -1
Copy link
Member

Choose a reason for hiding this comment

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

I think I missed this in previous reviews, but why are we returning -1 instead of like none?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an arbitrary integer value that is impossible as a valid threshold

Copy link

codecov bot commented Feb 5, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
23428 1 23427 289
View the top 1 failed tests by shortest run time
tests.sentry.incidents.endpoints.test_organization_alert_rule_details.AlertRuleDetailsPutEndpointTest::test_delete_trigger_dual_update_resolve
Stack Traces | 4.51s run time
#x1B[1m#x1B[.../incidents/endpoints/test_organization_alert_rule_details.py#x1B[0m:838: in test_delete_trigger_dual_update_resolve
    resp = self.get_success_response(
#x1B[1m#x1B[.../sentry/testutils/cases.py#x1B[0m:695: in get_success_response
    assert_status_code(response, status.HTTP_200_OK)
#x1B[1m#x1B[.../sentry/testutils/asserts.py#x1B[0m:40: in assert_status_code
    assert minimum <= response.status_code < maximum, (
#x1B[1m#x1B[31mE   AssertionError: (400, b'{"detail":{"code":"invalid-request","message":"Invalid request","extra":{}}}')#x1B[0m
#x1B[1m#x1B[31mE   assert 400 < 201#x1B[0m
#x1B[1m#x1B[31mE    +  where 400 = <Response status_code=400, "application/json">.status_code#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

Comment on lines +852 to +856
self.create_member(
user=self.user, organization=self.organization, role="owner", teams=[self.team]
)

self.login_as(self.user)
Copy link
Member

Choose a reason for hiding this comment

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

can you put this into setup to avoid repeating it in each test?

Copy link
Member

@ceorourke ceorourke left a comment

Choose a reason for hiding this comment

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

discussed offline that we'll implement the changes in a separate pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants