-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat: Create OrganizationContributors table in region dbs #103839
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #103839 +/- ##
=========================================
Coverage 80.61% 80.62%
=========================================
Files 9286 9291 +5
Lines 396341 396555 +214
Branches 25261 25261
=========================================
+ Hits 319526 319707 +181
- Misses 76355 76388 +33
Partials 460 460 |
|
This PR has a migration; here is the generated SQL for for --
-- Create model OrganizationContributors
--
CREATE TABLE "sentry_organizationcontributors" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "external_identifier" varchar(255) NOT NULL, "alias" varchar(255) NULL, "num_actions" integer NOT NULL, "date_added" timestamp with time zone NOT NULL, "date_updated" timestamp with time zone NOT NULL, "organization_id" bigint NOT NULL, "integration_id" bigint NOT NULL, CONSTRAINT "sentry_organizationcontributors_unique_org_contributor" UNIQUE ("organization_id", "integration_id", "external_identifier"));
ALTER TABLE "sentry_organizationcontributors" ADD CONSTRAINT "sentry_organizationc_organization_id_93cc3a35_fk_sentry_or" FOREIGN KEY ("organization_id") REFERENCES "sentry_organization" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_organizationcontributors" VALIDATE CONSTRAINT "sentry_organizationc_organization_id_93cc3a35_fk_sentry_or";
CREATE INDEX CONCURRENTLY "sentry_organizationcontributors_external_identifier_ef77d39a" ON "sentry_organizationcontributors" ("external_identifier");
CREATE INDEX CONCURRENTLY "sentry_organizationcontr_external_identifier_ef77d39a_like" ON "sentry_organizationcontributors" ("external_identifier" varchar_pattern_ops);
CREATE INDEX CONCURRENTLY "sentry_organizationcontributors_organization_id_93cc3a35" ON "sentry_organizationcontributors" ("organization_id");
CREATE INDEX CONCURRENTLY "sentry_organizationcontributors_integration_id_d13b5795" ON "sentry_organizationcontributors" ("integration_id");
CREATE INDEX CONCURRENTLY "sentry_organizationcontributors_date_updated_idx" ON "sentry_organizationcontributors" ("date_updated"); |
brendanhsentry
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.
I think you'll need to add your model to src/sentry/models/__init__.py. Then, run the command to generate the migration.
brendanhsentry
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.
I'd wait for the migration team to sign off before merging, but LGTM
|
This PR has a migration; here is the generated SQL for for --
-- Create model OrganizationContributors
--
CREATE TABLE "sentry_organizationcontributors" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "integration_id" bigint NOT NULL, "external_identifier" varchar(255) NOT NULL, "alias" varchar(255) NULL, "num_actions" integer NOT NULL, "date_added" timestamp with time zone NOT NULL, "date_updated" timestamp with time zone NOT NULL, "organization_id" bigint NOT NULL, CONSTRAINT "sentry_orgcont_unique_org_cont" UNIQUE ("organization_id", "integration_id", "external_identifier"));
ALTER TABLE "sentry_organizationcontributors" ADD CONSTRAINT "sentry_organizationc_organization_id_93cc3a35_fk_sentry_or" FOREIGN KEY ("organization_id") REFERENCES "sentry_organization" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_organizationcontributors" VALIDATE CONSTRAINT "sentry_organizationc_organization_id_93cc3a35_fk_sentry_or";
CREATE INDEX CONCURRENTLY "sentry_organizationcontributors_integration_id_d13b5795" ON "sentry_organizationcontributors" ("integration_id");
CREATE INDEX CONCURRENTLY "sentry_organizationcontributors_external_identifier_ef77d39a" ON "sentry_organizationcontributors" ("external_identifier");
CREATE INDEX CONCURRENTLY "sentry_organizationcontr_external_identifier_ef77d39a_like" ON "sentry_organizationcontributors" ("external_identifier" varchar_pattern_ops);
CREATE INDEX CONCURRENTLY "sentry_organizationcontributors_organization_id_93cc3a35" ON "sentry_organizationcontributors" ("organization_id");
CREATE INDEX CONCURRENTLY "sentry_orgcont_date_upd_idx" ON "sentry_organizationcontributors" ("date_updated"); |
| integration_id = HybridCloudForeignKey("sentry.Integration", on_delete="CASCADE") | ||
|
|
||
| external_identifier = models.CharField(max_length=255, db_index=True) | ||
| alias = models.CharField(max_length=255, null=True, blank=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.
Does it make sense to have an FK to this table instead of having these fields?
sentry/src/sentry/models/commitauthor.py
Lines 27 to 40 in 153511a
| @region_silo_model | |
| class CommitAuthor(Model): | |
| __relocation_scope__ = RelocationScope.Excluded | |
| organization_id = BoundedBigIntegerField(db_index=True) | |
| # display name | |
| name = models.CharField(max_length=128, null=True) | |
| email = models.CharField(max_length=200) | |
| # Format varies by provider: | |
| # - GitHub/GitHub Enterprise: "github:username", "github_enterprise:username" | |
| # - Other providers: null | |
| # - Legacy data(?): integer (rare) | |
| external_id = models.CharField(max_length=164, null=True) |
Or is the format there totally different?
| date_added = models.DateTimeField(auto_now_add=True) | ||
| date_updated = models.DateTimeField(auto_now=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.
You can use DefaultFieldsModel here instead of adding these manually
| ] | ||
| indexes = [ | ||
| models.Index( | ||
| fields=["date_updated"], |
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 just for handling retention deletes?
|
|
||
| external_identifier = models.CharField(max_length=255, db_index=True) | ||
| alias = models.CharField(max_length=255, null=True, blank=True) | ||
| num_actions = BoundedIntegerField(default=0) |
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.
Should the default here be 1? From the spec, it seems like we'll only create these rows when a user has taken an action
| date_updated = models.DateTimeField(auto_now=True) | ||
|
|
||
| class Meta: | ||
| app_label = "sentry" |
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 it worth considering whether this should be part of a separate module? Is this specific to prevent?
|
|
||
| organization = FlexibleForeignKey("sentry.Organization", on_delete=models.CASCADE) | ||
|
|
||
| integration_id = HybridCloudForeignKey("sentry.Integration", on_delete="CASCADE") |
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 just want to check here - is there any potential problem with allowing people to remove these rows by deleting their integration? Could they circumvent billing, or could it cause us problems in understanding billing?
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.
No, they won't be able to circumvent billing as the seat should be created after their 2nd action. As for understanding billing, it should be no different than deleting a uptime detector. The only difference is that Seer seats can only be ASSIGNED (not disabled, removed, or reallocated).
Updated version of https://github.com/getsentry/getsentry/pull/18880
1 pager with table info: https://www.notion.so/sentry/1-Pager-AI-Code-Review-Sub-Spec-2a98b10e4b5d80e29737f9c58f54efb9
Adds a new table that will hold all of the OrganizationContributors for a particular sentry org / integration_id combo.
If a user belongs and/or swaps to a separate Github org or sentry org, there will be a separate entry for that individual.
num_actions gets updated every time a successful seer request occurs, and num_actions gets cleared at the start of every billing period
Closes https://linear.app/getsentry/issue/ENG-5929/create-organizationcontributors-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.