Skip to content

Add Capabilities to Plugin model#4371

Merged
neilkakkar merged 5 commits intomasterfrom
capabilities
May 18, 2021
Merged

Add Capabilities to Plugin model#4371
neilkakkar merged 5 commits intomasterfrom
capabilities

Conversation

@neilkakkar
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
Contributor Author

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

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
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member

@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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Member

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.

5 participants