Skip to content

ref(hc): Delete get_region_by_id#49435

Merged
RyanSkonnord merged 7 commits into
masterfrom
remove-get-region-by-snowflake-id
May 22, 2023
Merged

ref(hc): Delete get_region_by_id#49435
RyanSkonnord merged 7 commits into
masterfrom
remove-get-region-by-snowflake-id

Conversation

@RyanSkonnord

Copy link
Copy Markdown
Contributor

Rename Region.id field and expand docstring to explain that all lookups should by done by name instead.

Rename Region.id field and expand docstring to explain that all lookups
should by done by name instead.
@RyanSkonnord RyanSkonnord requested a review from leeandher May 18, 2023 20:58
@RyanSkonnord RyanSkonnord requested a review from a team as a code owner May 18, 2023 20:58
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 18, 2023
@RyanSkonnord

Copy link
Copy Markdown
Contributor Author

Just something that I noticed in passing, and saw an opportunity for forestall an antipattern.

@leeandher If you've been spinning up live silos in your dev environment and passing the config as JSON, the JSON might need to be updated for the id -> snowflake_id change. Other than that, I don't think the name change affects anything.

@leeandher leeandher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense, I think some of the outbox code also points at regions by name as well, so making this standard is 👍

@RyanSkonnord

Copy link
Copy Markdown
Contributor Author

https://github.com/getsentry/getsentry/pull/10616 should be merged before this one (and should resolve "getsentry / backend" CI failures).

@codecov

codecov Bot commented May 19, 2023

Copy link
Copy Markdown

Codecov Report

Merging #49435 (f081a73) into master (dbb1fe1) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #49435      +/-   ##
==========================================
+ Coverage   80.94%   80.95%   +0.01%     
==========================================
  Files        4820     4820              
  Lines      202238   202199      -39     
  Branches    11364    11364              
==========================================
- Hits       163695   163690       -5     
+ Misses      38289    38255      -34     
  Partials      254      254              
Impacted Files Coverage Δ
src/sentry/testutils/helpers/api_gateway.py 98.63% <ø> (ø)
src/sentry/silo/client.py 96.00% <100.00%> (ø)
src/sentry/types/region.py 95.50% <100.00%> (-0.29%) ⬇️
src/sentry/utils/snowflake.py 94.66% <100.00%> (ø)

... and 11 files with indirect coverage changes

Test with a bogus name rather than a bogus ID number, because
RegionSiloClient.__init__ now looks up by name.
@RyanSkonnord RyanSkonnord merged commit 0f8773f into master May 22, 2023
@RyanSkonnord RyanSkonnord deleted the remove-get-region-by-snowflake-id branch May 22, 2023 21:24
volokluev pushed a commit that referenced this pull request May 30, 2023
Rename Region.id field and expand docstring to explain that all lookups
should by done by name instead.
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 7, 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.

3 participants