feat(preprod): Switch to new URL format in backend (EME-725)#106366
feat(preprod): Switch to new URL format in backend (EME-725)#106366runningcode merged 8 commits intomasterfrom
Conversation
fb34ac1 to
6b4e546
Compare
6b4e546 to
d29acae
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
chromy
left a comment
There was a problem hiding this comment.
looks good % seems like some delta from Ryan's refactoring which maybe ought not to be here
| ) | ||
| .select_related("mobile_app_info") | ||
| .order_by("app_id", "artifact_type", "date_added") | ||
| ) |
There was a problem hiding this comment.
Is this part of this change? Or is it something unrelated to do with the mobile_app_info refactoring?
There was a problem hiding this comment.
This change isn't needed but was a performance optimization suggested by claude which made sense to me.
select_related is the change here which is to prevent n+1 database queries.
With the select_related, Django uses a SQL Join, otherwise it would make a separate database query to fetch the mobile_app_info.
Example:
Before (N+1 queries):
artifacts = PreprodArtifact.objects.filter(...) # 1 query
for artifact in artifacts:
name = artifact.mobile_app_info.app_name # +1 query per artifact
After (1 query):
artifacts = PreprodArtifact.objects.filter(...).select_related("mobile_app_info") # 1 query with JOIN
for artifact in artifacts:
name = artifact.mobile_app_info.app_name # No additional query - already loaded
There was a problem hiding this comment.
Just a follow up here, the only usage of this function (get_sibling_artifacts_for_commit()) is in
src/sentry/preprod/vcs/status_checks/size/tasks.py:139 which then calls format_status_check_messages() which then calls _format_artifact_summary() which then gets mobile_app_info on line 134 for every artifact.
Update backend URL generation to use new format with view type in path
and project as query parameter.
- Update get_preprod_artifact_url() to generate /preprod/{view_type}/{id}?project={slug}
- Update get_preprod_artifact_comparison_url() to generate comparison URLs with new format
- Update GitHub status check templates to use new URL format
- Update tests to verify new URL format
Status check templates were reading from deprecated PreprodArtifact fields (app_name, build_version, build_number) which are no longer populated for artifacts created after migration 0022. This caused app names to display as "--" and versions as "Unknown" in VCS status checks. Changes: - Update get_sibling_artifacts_for_commit() to preload mobile_app_info - Update _format_version_string() to read from mobile_app_info - Update app_name reading to use mobile_app_info.app_name After migration 0022, all mobile app fields are stored in PreprodArtifactMobileAppInfo and should be read from there instead of the deprecated PreprodArtifact fields.
Updated tests to use self.create_preprod_artifact() and self.create_preprod_artifact_size_metrics() factory methods instead of calling Model.objects.create() directly, following test guidelines. The factory methods automatically create PreprodArtifactMobileAppInfo when mobile app fields are provided, ensuring tests work with the new mobile app info model structure. Also updated test assertions to account for platform labels (Android/iOS) being included in qualifier output.
Fixed URL generation to use the correct API for organization.absolute_url() by passing query parameters separately instead of embedding them in the path. This fixes issues with customer domain URL transformations where the regex patterns in customer_domain_path() could mangle URLs with embedded query strings. Changes: - get_preprod_artifact_url: Split path and query parameters - get_preprod_artifact_comparison_url: Split path and query parameters - _format_failure_summary: Pass query as separate parameter This aligns with how other parts of the codebase use absolute_url() (e.g., Project.get_absolute_url, Group.get_absolute_url).
a1417bf to
46c7efb
Compare
…s (EME-725) Revert the change that removed platform prefixes from Watch and Dynamic Feature artifact labels. The format is now back to showing both the platform and artifact type (e.g., "iOS, Watch" instead of just "Watch").
| # Should have two rows - main app shows sizes, watch shows processing | ||
| assert "`com.example.app`" in summary | ||
| assert "-- (Watch)" in summary | ||
| assert "(Android, Watch)" in summary or "(iOS, Watch)" in summary |
There was a problem hiding this comment.
This change is due to self.create_preprod_artifact now setting the artifact_type whereas before it wasn't set.
| # Should have two rows - main app and watch app | ||
| assert "`com.example.app`" in summary # Both main and watch show app_id | ||
| assert "-- (Watch)" in summary # Watch app label | ||
| assert ", Watch)" in summary # Watch app label (with platform prefix) |
There was a problem hiding this comment.
This change is due to self.create_preprod_artifact now setting the artifact_type whereas before it wasn't set.
| if "`com.example.app`" in line and ", Watch)" not in line: | ||
| main_row_idx = i | ||
| elif "-- (Watch)" in line: | ||
| elif ", Watch)" in line: |
There was a problem hiding this comment.
This change is due to self.create_preprod_artifact now setting the artifact_type whereas before it wasn't set.
Summary
Part 2 of 3-part deployment to restructure preprod URLs. This PR updates backend URL generation to use the new format.
Deployment order:
Example URLs Generated
Before:
/organizations/sentry/preprod/my-project/artifact-123/organizations/sentry/preprod/my-project/compare/artifact-123/artifact-456After:
/organizations/sentry/preprod/size/artifact-123?project=my-project/organizations/sentry/preprod/size/compare/artifact-123/artifact-456?project=my-projectWhy This Approach
Since frontend deployed first with redirects:
Dependencies