Playlist: Add create+update timestamps to the database #76295
Conversation
| @@ -82,7 +82,6 @@ func (*OSSMigrations) AddMigration(mg *Migrator) { | |||
| accesscontrol.AddManagedPermissionsMigration(mg, accesscontrol.ManagedPermissionsMigrationID) | |||
| accesscontrol.AddManagedFolderAlertActionsMigration(mg) | |||
| accesscontrol.AddActionNameMigrator(mg) | |||
| addPlaylistUIDMigration(mg) | |||
There was a problem hiding this comment.
note: this does not remove the migration -- the same migrations are now added in addPlaylistMigrations
There was a problem hiding this comment.
I know this doesn't remove the migration, but we must not change the migrations list! It is additive only - that's set out in our contribution guidelines and changing past migrations can cause unexpected behavior in existing instances: https://github.com/grafana/grafana/blob/main/contribute/backend/database.md#migrations
ryantxu
requested review from
suntala,
yangkb09 and
mildwonkey
and removed request for
a team
|
|
||
| func addPlaylistUIDMigration(mg *Migrator) { |
There was a problem hiding this comment.
^^ this was split as a different function before, but now all migrations added from the same entry point
There was a problem hiding this comment.
But this also changes the order of the migrations, doesn't it? What's the benefit of merging it into one function?
There was a problem hiding this comment.
removed the cleanup -- the only reason i did it was to reuse the same table structure created in playlistV2() but does not really matter, we can create a new one for every migration
There was a problem hiding this comment.
I don't think we can change existing migrations the way this PR does, but I'll get a second opinion on that. Otherwise, this looks (and works) ok to me!
🤔 Not directly related, but has anyone suggested we add a provisioning source field to all of our kinds, to differentiate between records saved via k8s, the UI, terraform, etc?
| @@ -82,7 +82,6 @@ func (*OSSMigrations) AddMigration(mg *Migrator) { | |||
| accesscontrol.AddManagedPermissionsMigration(mg, accesscontrol.ManagedPermissionsMigrationID) | |||
| accesscontrol.AddManagedFolderAlertActionsMigration(mg) | |||
| accesscontrol.AddActionNameMigrator(mg) | |||
| addPlaylistUIDMigration(mg) | |||
There was a problem hiding this comment.
I know this doesn't remove the migration, but we must not change the migrations list! It is additive only - that's set out in our contribution guidelines and changing past migrations can cause unexpected behavior in existing instances: https://github.com/grafana/grafana/blob/main/contribute/backend/database.md#migrations
| } | ||
| // Add columns used for kubernetes dual write synchronization | ||
| mg.AddMigration("Add playlist column created_at", NewAddColumnMigration(playlistV2, &Column{ | ||
| Name: "created_at", Type: DB_BigInt, Nullable: false, Default: "0", |
There was a problem hiding this comment.
Shouldn't this be DATETIME SQL type (same about updated_at)?
There was a problem hiding this comment.
with DATETIME, you can't apply a default to an existing structure :( so it can not be a required field, and that leads to needing pointers 🤮
There was a problem hiding this comment.
I'm not saying this as an argument in either direction, but I poked through the code and we're more likely to use an int64 than DATETIME in other tables, too 🤷🏻
|
removed the migration cleanup -- does not really matter, so no issue with it |
yes -- entity api has an origin field to help with this. In k8s it will be stored as an annotation on any resource. I don't think it is worth adding to the existing tables though. |
What is this feature?
This adds created+updated epoc columns to the playlist database
Why do we need this feature?
The k8s api service needs something to track when a resource changes
Who is this feature for?
This is to help move k8s development forward.
After running the playlist table will look like:
Extracted from #75709