-
Notifications
You must be signed in to change notification settings - Fork 7
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
Move Canteen Static Data in Migration #251
Conversation
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.
I have left some inline style comments, have fixed a typo and have added the migration to the migration.go
Overall, nice job
Co-authored-by: Frank Elsinga <[email protected]>
Co-authored-by: Frank Elsinga <[email protected]>
Co-authored-by: Frank Elsinga <[email protected]>
Co-authored-by: Frank Elsinga <[email protected]>
Co-authored-by: Frank Elsinga <[email protected]>
Co-authored-by: Frank Elsinga <[email protected]>
Co-authored-by: Frank Elsinga <[email protected]>
Co-authored-by: Frank Elsinga <[email protected]>
Co-authored-by: Frank Elsinga <[email protected]>
Co-authored-by: Frank Elsinga <[email protected]>
Co-authored-by: Frank Elsinga <[email protected]>
Co-authored-by: Frank Elsinga <[email protected]>
I've also altered some of the older migrations to start the server on an empty database. It will be checked whether the table exists and if not, the new table will be created. @CommanderStorm please have a look at it that we are not overriding tables on the active database. |
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.
Overall, I would have merged this, exept for the added if !tx.Migrator().HasTable(&File{}) {
Why did you add them? (don't they kind of defeat the point of a migration?)
The has table is not needed anymore, this was a relic of testing a successful migration. |
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.
LGTM 🎉
Moves the database initialization into the migration. The db initializer is not needed anymore. Furthermore, halve the database request can be deleted since we do not need to check whether the entry already exists in the table if the table is newly cleaned.
Logging is set to silent and only in case of errors the errors will be logged.