-
Notifications
You must be signed in to change notification settings - Fork 652
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
New Visualization Backups table #5776
Conversation
@juanignaciosl CR please (although is so dumb I'm not sure it needs CR) |
String :username, null: false | ||
Uuid :visualization, null: false | ||
String :export_vizjson, null: false | ||
DateTime :created_at, default: Sequel::CURRENT_TIMESTAMP |
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.
- It would be nice having a
:source
field with the cause of the backup (ghost tables, manual deletion, whatever). - We should push type safety and make
:export_vizjson
json type. I've had some discussions about this forcdb_conf
and we finally pushed for json type as well.
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.
- The source/type of backup will come but inside the
export_vizjson
, as it already requires some extra things like versioning, always exporting all layers, an owner... So taken care inside there. - The idea is to have a quickly working solution without much logic at this level, that's why also there are no constraints at other fields, so I'd rather build something really simple and if later given time to, improve the data type.
But thanks for the suggestion, if we iterate over this I'll surely change the type, is just that the biggest requisite is "have it fast" so making tradeoffs wherever I can to achieve it.
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.
👍
👍 after my suggestions, both up to you. |
New Visualization Backups table
Relates to #5710
Just the migration to add the DB table