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
move release commits endpoint to organization path #4863
Conversation
Security concerns found
Generated by 🚫 danger |
e944276
to
99319eb
Compare
99319eb
to
24cd030
Compare
24cd030
to
123d533
Compare
CHANGES
Outdated
@@ -16,6 +16,7 @@ Schema Changes | |||
~~~~~~~~~~~~~~ | |||
|
|||
- Added ``Deploy`` model | |||
- Added OrganizationReleaseCommitsEndpoint |
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.
This is an API change.
request=request, | ||
queryset=queryset, | ||
order_by='order', | ||
on_results=lambda x: serialize([rc.commit for rc in x], request.user), |
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.
If we're only serializing the Commit objects instead of the ReleaseCommit, why don't we just query for Commits instead? Seems a bit awkward going through ReleaseCommit just to get that.
You could just do:
Commit.objects.filter(releasecommit__release=release)
And you'll directly have the objects you want. Then the order_by
becomes releasecommit__order
.
This gives us SQL like:
SELECT "sentry_commit"."id", "sentry_commit"."organization_id", "sentry_commit"."repository_id", "sentry_commit"."key", "sentry_commit"."date_added", "sentry_commit"."author_id", "sentry_commit"."message"
FROM "sentry_commit"
INNER JOIN "sentry_releasecommit" ON (
"sentry_commit"."id" = "sentry_releasecommit"."commit_id"
)
WHERE "sentry_releasecommit"."release_id" = 1
ORDER BY "sentry_releasecommit"."order" ASC
Unless I'm missing something here with what you're doing.
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.
i just did it this way because that's how we were doing it in the project version of this endpoint (see below file). but i can change it in both places
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.
ok, so that doesn't work because of https://github.com/getsentry/sentry/blob/master/src/sentry/api/paginator.py#L120
like Commit.objects.filter(releasecommit__release=release).order_by('releasecommit__order')
works, but the paginator tries to do commit.releasecommit__order
which doesn't.
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.
welp 😢
moves https://docs.sentry.io/api/releases/get-release-commits/
depends on #4775. tests will fail until that can be merged/rebased in because i need
OrganizationReleasesBaseEndpoint
from that pr.cc @benvinegar @ckj