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

ref(releases): Move code to a new function #81208

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

armenzg
Copy link
Member

@armenzg armenzg commented Nov 22, 2024

No description provided.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 22, 2024
or status_details.get("inNextRelease")
or status_details.get("inUpcomingRelease")
or status_details.get("inRelease")
):
Copy link
Member Author

Choose a reason for hiding this comment

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

The four if statements prevent the usage for multiple projects:

# TODO(jess): We may want to support this for multi project, but punting on it for now
if len(projects) > 1:
return Response(
{"detail": "Cannot set resolved in next release for multiple projects."},
status=400,
)

Copy link
Member Author

Choose a reason for hiding this comment

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

The split view is better for reviewing this PR:
image

commit = None
release = None

if status == "resolvedInNextRelease" or status_details.get("inNextRelease"):
Copy link
Member Author

Choose a reason for hiding this comment

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

If I copy/paste the original code the only differences are these:

diff --git a/src/sentry/api/helpers/group_index/update.py b/src/sentry/api/helpers/group_index/update.py
index 4d872033722..8de70a96876 100644
--- a/src/sentry/api/helpers/group_index/update.py
+++ b/src/sentry/api/helpers/group_index/update.py
@@ -548,6 +548,12 @@ def resolve_in_release_helper(
     release = None
 
     if status == "resolvedInNextRelease" or status_details.get("inNextRelease"):
+        # TODO(jess): We may want to support this for multi project, but punting on it for now
+        if len(projects) > 1:
+            return Response(
+                {"detail": "Cannot set resolved in next release for multiple projects."},
+                status=400,
+            )
         # may not be a release yet
         release = status_details.get("inNextRelease") or get_release_to_resolve_by(projects[0])
 
@@ -567,6 +573,11 @@ def resolve_in_release_helper(
         res_type_str = "in_next_release"
         res_status = GroupResolution.Status.pending
     elif status_details.get("inUpcomingRelease"):
+        if len(projects) > 1:
+            return Response(
+                {"detail": "Cannot set resolved in upcoming release for multiple projects."},
+                status=400,
+            )
         release = status_details.get("inUpcomingRelease") or most_recent_release(projects[0])
         activity_type = ActivityType.SET_RESOLVED_IN_RELEASE.value
         activity_data = {"version": ""}
@@ -581,6 +592,13 @@ def resolve_in_release_helper(
         res_type_str = "in_upcoming_release"
         res_status = GroupResolution.Status.pending
     elif status_details.get("inRelease"):
+        # TODO(jess): We could update validation to check if release
+        # applies to multiple projects, but I think we agreed to punt
+        # on this for now
+        if len(projects) > 1:
+            return Response(
+                {"detail": "Cannot set resolved in release for multiple projects."}, status=400
+            )
         release = status_details["inRelease"]
         activity_type = ActivityType.SET_RESOLVED_IN_RELEASE.value
         activity_data = {
@@ -598,6 +616,12 @@ def resolve_in_release_helper(
         res_type_str = "in_release"
         res_status = GroupResolution.Status.resolved
     elif status_details.get("inCommit"):
+        # TODO(jess): Same here, this is probably something we could do, but
+        # punting for now.
+        if len(projects) > 1:
+            return Response(
+                {"detail": "Cannot set resolved in commit for multiple projects."}, status=400
+            )
         commit = status_details["inCommit"]
         activity_type = ActivityType.SET_RESOLVED_IN_COMMIT.value
         activity_data = {"commit": commit.id}

res_type = GroupResolution.Type.in_release
res_status = GroupResolution.Status.resolved
except IndexError:
release = None
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 lines come from this block:

# if we've specified a commit, let's see if its already been released
# this will allow us to associate the resolution to a release as if we
# were simply using 'inRelease' above
# Note: this is different than the way commit resolution works on deploy
# creation, as a given deploy is connected to an explicit release, and
# in this case we're simply choosing the most recent release which contains
# the commit.
if commit and not release:
# TODO(jess): If we support multiple projects for release / commit resolution,
# we need to update this to find the release for each project (we shouldn't assume
# it's the same)
try:
release = most_recent_release_matching_commit(projects, commit)
res_type = GroupResolution.Type.in_release
res_status = GroupResolution.Status.resolved
except IndexError:
release = None

Copy link

codecov bot commented Nov 22, 2024

❌ 33 Tests Failed:

Tests completed Failed Passed Skipped
23159 33 23126 214
View the top 3 failed tests by shortest run time
tests.sentry.api.helpers.test_group_index.UpdateGroupsTest::test_resolving_unresolved_group
Stack Traces | 1.98s run time
#x1B[1m#x1B[.../api/helpers/test_group_index.py#x1B[0m:133: in test_resolving_unresolved_group
    update_groups(
#x1B[1m#x1B[.../helpers/group_index/update.py#x1B[0m:255: in update_groups
    result, res_type = handle_resolve_in_release(
#x1B[1m#x1B[.../helpers/group_index/update.py#x1B[0m:330: in handle_resolve_in_release
    ) = resolve_in_release_helper(status, status_details, projects, user)
#x1B[1m#x1B[.../helpers/group_index/update.py#x1B[0m:640: in resolve_in_release_helper
    res_type,
#x1B[1m#x1B[31mE   UnboundLocalError: cannot access local variable 'res_type' where it is not associated with a value#x1B[0m
tests.sentry.issues.endpoints.test_group_details.GroupUpdateTest::test_resolve
Stack Traces | 2.15s run time
#x1B[1m#x1B[.../issues/endpoints/test_group_details.py#x1B[0m:317: in test_resolve
    assert response.status_code == 200, response.content
#x1B[1m#x1B[31mE   AssertionError: b'{"detail":"Internal Error","errorId":null}'#x1B[0m
#x1B[1m#x1B[31mE   assert 500 == 200#x1B[0m
#x1B[1m#x1B[31mE    +  where 500 = <Response status_code=500, "application/json">.status_code#x1B[0m
tests.sentry.integrations.discord.webhooks.test_message_component.DiscordMessageComponentInteractionTest::test_resolve_non_dialog
Stack Traces | 2.29s run time
#x1B[1m#x1B[.../discord/webhooks/test_message_component.py#x1B[0m:203: in test_resolve_non_dialog
    assert response.status_code == 200
#x1B[1m#x1B[31mE   assert 500 == 200#x1B[0m
#x1B[1m#x1B[31mE    +  where 500 = <Response status_code=500>.status_code#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
Got feedback? Let us know on Github

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.

1 participant