Conversation
yakkomajuri
left a comment
There was a problem hiding this comment.
I know no review was requested, but this here should be fine. The juice should be over in the plugin server PR
|
Any idea how I can prove migrations are safe to run at scale? |
posthog/models/plugin.py
Outdated
| 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) |
There was a problem hiding this comment.
Could the default be an empty array? This way we can avoid null=True and only ever check whether the field .includes a capability.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Right, thanks, missed that.
|
This should be fine to run :). This point mostly applies to tables like |
|
Column/table additions with a default are pretty much always safe @neilkakkar, so this is an "obvious" case. :) Deletions and alters are controversial though. |
Changes
Adds Capabilities to the DB, set via PostHog/plugin-server#384. Part of PostHog/plugin-server#379
Checklist