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

feat(dnd-worldmap-removal) Added null checks for columns and fields. #53475

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

Abdkhan14
Copy link
Contributor

@Abdkhan14 Abdkhan14 commented Jul 24, 2023

  • Migration already approved here: PR but it had a runtime error.
  • Added null checks for columns and fields in DashboardWidgetQuery.
  • Should fix run time error from previous migration attempt:
Screenshot 2023-07-24 at 6 17 48 PM

@Abdkhan14 Abdkhan14 requested a review from markstory July 24, 2023 22:20
@Abdkhan14 Abdkhan14 requested a review from a team as a code owner July 24, 2023 22:20
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 24, 2023
@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0520_migrate_worldmap_widgets.py ()

--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #53475 (19dfa52) into master (64fba06) will decrease coverage by 1.32%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #53475      +/-   ##
==========================================
- Coverage   79.55%   78.24%   -1.32%     
==========================================
  Files        4942     4917      -25     
  Lines      209068   208787     -281     
  Branches    35603    35562      -41     
==========================================
- Hits       166331   163355    -2976     
- Misses      37658    40222    +2564     
- Partials     5079     5210     +131     

see 259 files with indirect coverage changes

@Abdkhan14 Abdkhan14 requested a review from a team July 25, 2023 01:26
Comment on lines 11 to 13
widgetQueries = DashboardWidgetQuery.objects.select_related("widget").filter(
widget__display_type=5
)
Copy link
Member

Choose a reason for hiding this comment

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

You're around the 10k row count. You could hit a migration timeout when updating rows. If you wanted to be totally safe an is_dangerous = True migration would safer albeit much slower.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'd rather be safe. How much longer would it take @markstory?

@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0521_migrate_world_map_widgets.py ()

--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--

@Abdkhan14 Abdkhan14 merged commit 29b55e0 into master Jul 25, 2023
53 of 55 checks passed
@Abdkhan14 Abdkhan14 deleted the abdk/dnd-world-map-widgets branch July 25, 2023 14:10
chloeho7 pushed a commit that referenced this pull request Jul 25, 2023
…53475)

- Migration already approved here:
[PR](#53109) but it had a
runtime error.
- Added null checks for columns and fields in DashboardWidgetQuery.
- Should fix run time error from previous migration attempt: 
<img width="488" alt="Screenshot 2023-07-24 at 6 17 48 PM"
src="https://github.com/getsentry/sentry/assets/60121741/49fe3214-77c3-4bdb-b02f-355eb567f1d3">

- Link to failure:
https://deploy.getsentry.net/go/tab/build/detail/deploy-getsentry-backend-us/63/migrations/1/migrations
- Tested it out locally with NULL columns.

Co-authored-by: Abdullah Khan <abdullahkhan@PG9Y57YDXQ.local>
@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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