Skip to content

ref(hybrid-cloud): Require using param when creating transactions or using unguarded_write decorators #53160

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

Merged

Conversation

GabeVillalobos
Copy link
Member

@GabeVillalobos GabeVillalobos commented Jul 19, 2023

With the inclusion of our recent changes to transaction validations in PR #52943 and the inclusion of 'using' in all of our transactions/helpers thanks to PRs #53027 and #53004, we should be able to safely remove the ability to specify a transaction without a using keyword arg.

Because this is now mandatory, it should also be safe to remove autorouting, which ended up being incorrect in many testing contexts anyway.

This is a resubmission of PR #53080 which was reverted due to broken load-mocks and a single broken getsentry migration test.

@GabeVillalobos GabeVillalobos requested review from a team as code owners July 19, 2023 16:27
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 19, 2023
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #53160 (571a8e4) into master (39d1af2) will decrease coverage by 3.20%.
The diff coverage is 90.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #53160      +/-   ##
==========================================
- Coverage   79.50%   76.30%   -3.20%     
==========================================
  Files        4936     4909      -27     
  Lines      207856   207517     -339     
  Branches    35466    35425      -41     
==========================================
- Hits       165253   158352    -6901     
- Misses      37573    44024    +6451     
- Partials     5030     5141     +111     
Impacted Files Coverage Δ
...atic/app/views/releases/list/releaseCard/index.tsx 79.77% <ø> (-0.23%) ⬇️
static/app/views/releases/utils/index.tsx 78.57% <ø> (ø)
...entry/silo/patches/silo_aware_transaction_patch.py 81.25% <77.77%> (-2.09%) ⬇️
src/sentry/models/organization.py 85.12% <100.00%> (-2.80%) ⬇️
src/sentry/services/hybrid_cloud/auth/impl.py 91.71% <100.00%> (-0.64%) ⬇️
src/sentry/silo/safety.py 88.00% <100.00%> (ø)
src/sentry/testutils/factories.py 91.36% <100.00%> (-1.54%) ⬇️
src/sentry/testutils/hybrid_cloud.py 78.49% <100.00%> (-10.03%) ⬇️
...c/sentry/web/frontend/unsubscribe_notifications.py 82.05% <100.00%> (ø)

... and 481 files with indirect coverage changes

@GabeVillalobos GabeVillalobos force-pushed the remove-db-auto-selection-from-transaction-patch branch from 8ae91c7 to 571a8e4 Compare July 19, 2023 17:22
Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Looks good once the dependencies are cleared up.

@GabeVillalobos GabeVillalobos merged commit 9b816ea into master Jul 19, 2023
@GabeVillalobos GabeVillalobos deleted the remove-db-auto-selection-from-transaction-patch branch July 19, 2023 17:53
@github-actions github-actions bot locked and limited conversation to collaborators Aug 4, 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