feat(crons): Remove slug null=True #45276
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
evanpurkhiser
requested review from
rjo100 and
a team
and removed request for
a team
github-actions
bot
added
the
Scope: Backend
|
This PR has a migration; here is the generated SQL for --
-- Alter field slug on monitor
--
ALTER TABLE "sentry_monitor" ADD CONSTRAINT "sentry_monitor_slug_ad0f637f_notnull" CHECK ("slug" IS NOT NULL) NOT VALID;
ALTER TABLE "sentry_monitor" VALIDATE CONSTRAINT "sentry_monitor_slug_ad0f637f_notnull"; |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #45276 +/- ##
==========================================
- Coverage 80.23% 80.23% -0.01%
==========================================
Files 4724 4724
Lines 198821 198821
Branches 12020 12020
==========================================
- Hits 159525 159520 -5
- Misses 39034 39039 +5
Partials 262 262
|
JoshFerge
reviewed
Mar 1, 2023
There was a problem hiding this comment.
how often is this table written / read to? from what i'm understanding, the first statement will still lock, albeit not for long. https://medium.com/doctolib/adding-a-not-null-constraint-on-pg-faster-with-minimal-locking-38b2c00c4d1c
evanpurkhiser
force-pushed
the
evanpurkhiser/feat-crons-remove-slug-null-true
branch
from
1cf382e to
40265a1
Compare
|
reading https://develop.sentry.dev/database-migrations/#adding-not-null-to-columns, seems safe -- LGTM once tests pass |
rjo100
approved these changes
Mar 2, 2023
| @@ -155,7 +155,7 @@ def save(self, *args, **kwargs): | |||
| # NOTE: We ONLY set a slug while saving when creating a new monitor and | |||
| # the slug has not been set. Otherwise existing monitors without slugs | |||
| # would have their guids changed | |||
| if self._state.adding is True and self.slug is None: | |||
| if self._state.adding is True and self.slug == "": | |||
There was a problem hiding this comment.
just say and not self.slug. empty strings eval to False
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After the completion of #45270 all slugs now have values
This is safe as we will be adding the constraint as
NOT VALIDand runningVALIDATEimmediately after (which does not lock)