Skip to content
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

Add Capabilities to Plugin model #4371

Merged
merged 5 commits into from
May 18, 2021
Merged

Add Capabilities to Plugin model #4371

merged 5 commits into from
May 18, 2021

Conversation

neilkakkar
Copy link
Collaborator

@neilkakkar neilkakkar commented May 17, 2021

Changes

Adds Capabilities to the DB, set via PostHog/plugin-server#384. Part of PostHog/plugin-server#379

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious

@timgl timgl temporarily deployed to posthog-pr-4371 May 17, 2021 16:50 Inactive
@timgl timgl temporarily deployed to posthog-pr-4371 May 17, 2021 17:17 Inactive
Copy link
Contributor

@yakkomajuri yakkomajuri left a comment

Choose a reason for hiding this comment

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

I know no review was requested, but this here should be fine. The juice should be over in the plugin server PR

@neilkakkar
Copy link
Collaborator Author

Any idea how I can prove migrations are safe to run at scale?

@@ -131,6 +131,7 @@ class PluginType(models.TextChoices):
source: models.TextField = models.TextField(blank=True, null=True)
latest_tag: models.CharField = models.CharField(max_length=800, null=True, blank=True)
latest_tag_checked_at: models.DateTimeField = models.DateTimeField(null=True, blank=True)
capabilities: models.JSONField = models.JSONField(default=None, null=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the default be an empty array? This way we can avoid null=True and only ever check whether the field .includes a capability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be an array, not an object, as alluded to here: PostHog/plugin-server#384 (comment)

Basically, we already need to store "type of capability" with the capability, and in the future I'm sure there will be more metadata we want to add (capabilitiesCapturedAt or whatever), making an object as the default a saner choice than an array.

No need to worry about indexing speed when querying as we'll have very few plugins anyway.

Copy link
Collaborator

@Twixes Twixes May 18, 2021

Choose a reason for hiding this comment

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

Or a default object, right. Just would be nice to avoid null checks. Unless we only fill this in for newly installed/updated plugins after merging this feature (using null as the "capabilities never captured yet" state) – though should probably just detect this and save ASAP to avoid incomplete states and nulls.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, thanks, missed that.

@mariusandra
Copy link
Collaborator

This should be fine to run :). This point mostly applies to tables like event, element, person, etc that could have millions of rows. For those tables, if you run a migration that needs to alter every row on the table (e.g. by setting a default field), usually the migration fails since it's not possible for get a lock on the table to run the migration without anything else touching it... and then it'll fail.

@Twixes
Copy link
Collaborator

Twixes commented May 18, 2021

Column/table additions with a default are pretty much always safe @neilkakkar, so this is an "obvious" case. :) Deletions and alters are controversial though.

@timgl timgl temporarily deployed to posthog-pr-4371 May 18, 2021 10:12 Inactive
@timgl timgl temporarily deployed to posthog-pr-4371 May 18, 2021 10:22 Inactive
@timgl timgl temporarily deployed to posthog-pr-4371 May 18, 2021 10:33 Inactive
@neilkakkar neilkakkar merged commit 7be1b0e into master May 18, 2021
@neilkakkar neilkakkar deleted the capabilities branch May 18, 2021 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants