-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat: Create new repository settings table #104645
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #104645 +/- ##
===========================================
+ Coverage 80.51% 80.53% +0.01%
===========================================
Files 9329 9330 +1
Lines 400854 400936 +82
Branches 25708 25708
===========================================
+ Hits 322732 322875 +143
+ Misses 77670 77609 -61
Partials 452 452 |
| enabled_code_review = models.BooleanField(default=False) | ||
| code_review_triggers = ArrayField( | ||
| models.CharField(max_length=32, choices=CodeReviewTrigger.as_choices()), | ||
| default=list, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense for enable_code_review to be on the Repository, and then have this table be
RepositorySetting
- id
- repository_id
- trigger
unique (repository_id, trigger)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking keep it separate in the event we end up adding more repository settings, I think one thing that was brought up in the ecosystem office hours was some org setting that has been commonly asked for to also be a repository setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is potentially something we can do w.r.t. having a single column where the presence of anything in the list means enabled and for what triggers, while an empty list could signal disabled, but I think that's a little less intuitive and relies on "triggers" being edible in the future too
| class RepositorySettings(Model): | ||
| """ | ||
| Stores code review settings for a repository. | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ever going to be other settings, or just code review settings? If only code review, might be worth reflecting in the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The former, general table for repository settings (example in the above comment too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realized you meant the comment was saying something separate 🤦 Updated in ffc6047
suejung-sentry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving on the business logic side, matches what we discussed
| __relocation_scope__ = RelocationScope.Global | ||
|
|
||
| repository = FlexibleForeignKey( | ||
| "sentry.Repository", on_delete=models.CASCADE, unique=True, db_index=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, today I learned sentry.Repository should be capitalized here to match the model but lowercase in the migration above haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah django is funny like that.
| @region_silo_model | ||
| class RepositorySettings(Model): | ||
| """ | ||
| Stores code review settings for a repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update to "Stores settings for a repository" to reflect it's gonna be generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh oops didn't see the other review - dupe of this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh crap yeah i totally glanced over that
|
This PR has a migration; here is the generated SQL for for --
-- Create model RepositorySettings
--
CREATE TABLE "sentry_repositorysettings" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "enabled_code_review" boolean NOT NULL, "code_review_triggers" varchar(32)[] NOT NULL, "repository_id" bigint NOT NULL UNIQUE);
ALTER TABLE "sentry_repositorysettings" ADD CONSTRAINT "sentry_repositoryset_repository_id_ac6f0692_fk_sentry_re" FOREIGN KEY ("repository_id") REFERENCES "sentry_repository" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_repositorysettings" VALIDATE CONSTRAINT "sentry_repositoryset_repository_id_ac6f0692_fk_sentry_re"; |
| __relocation_scope__ = RelocationScope.Global | ||
|
|
||
| repository = FlexibleForeignKey( | ||
| "sentry.Repository", on_delete=models.CASCADE, unique=True, db_index=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah django is funny like that.
| ("enabled_code_review", models.BooleanField(default=False)), | ||
| ( | ||
| "code_review_triggers", | ||
| django.contrib.postgres.fields.ArrayField( | ||
| base_field=models.CharField(max_length=32), default=list, size=None | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Repository has a config json blob field. If you don't need to do filtering/aggregations on these fields you could use the JSON field. Alternatively, you could add these columns to sentry_repository directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markstory Spoke to integrations during office hours and we didn't wanna use the config blob for 2 reasons:
- That config field is a text field rather than a json column, and is marked as a legacy field in the code too:
sentry/src/sentry/models/repository.py
Line 43 in e87d194
config = LegacyTextJSONField(default=dict) - We wanted to keep the repository table clean of any settings, opting to do a join/expand for any list/get APIs that require this additional information. The current config column looked to just have a "name:" key/value for the most part (GitHub provider repos), other providers might've had different blobs but didn't explicitly scan many of em.
Some more context on what we're building can be found in the doc @suejung-sentry started here
https://www.notion.so/sentry/Seer-Settings-Backend-2bf8b10e4b5d80268863fb6107d5a374
markstory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DDL looks good. I personally would have put the settings fields onto sentry_repository as it makes interacting with settings less complicated, but you have reasonable arguments for using a separate table as well.
…4666) For seer launch, there's a toggle where an org can turn on auto-enable code review on repos. As in sentry org turns this toggle on, they add a new repo in github that has sentry github app installed on it, that means we should consider this repo enabled for code review. For new repos, they get written to `sentry_repository` table upon repositoryCreated gh webhook, or if the user manually adds the repo using sentry UI. Instead of doing this by finding every existing repo.objects.create (and remembering to do it if any new cases arise), probably most durable to add as a signal. There are downsides to using signals but it seems like a common pattern across sentry codebase to use these. Add that signal handler in this PR Closes https://linear.app/getsentry/issue/ENG-6094/handle-auto-enable-code-review-org-setting Depends on #104645
This PR aims to create our new repository settings table, with a foreign key back to the organization repository table.
Table currently stores if code review is enabled, as well as the triggers associated with it being enabled. The triggers are associated with the enum CodeReviewTrigger, also instantiated on the model
Closes https://linear.app/getsentry/issue/ENG-6089/create-repository-settings-table
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.