-
Notifications
You must be signed in to change notification settings - Fork 138
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
Add support for MariaDB/MySQL #1229
Conversation
dc.Port, | ||
escapeStr(dc.Database)) | ||
|
||
log.Printf("DB Connection string: %s:*********tcp(%s:%d)/%s?parseTime=true", |
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.
@ char missing after the starred-out password
@@ -226,7 +267,7 @@ func Ping(db *sql.DB) error { | |||
// SQLite uses ? | |||
func ModifySQLStatement(sql string, databaseProvider string) string { | |||
|
|||
if databaseProvider == "sqlite" { | |||
if databaseProvider == "sqlite" || databaseProvider == "mysql" { |
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.
Can we not use the constants that were added?
components/app-core/backend/main.go
Outdated
@@ -283,9 +286,19 @@ func initSessionStore(db *sql.DB, databaseProvider string, pc interfaces.PortalC | |||
sessionStore.Options.Secure = true | |||
return sessionStore, err | |||
} | |||
// Store depends on the DB Type | |||
if databaseProvider == "mysql" { |
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.
Can we use the constant?
components/app-core/backend/main.go
Outdated
@@ -273,6 +274,8 @@ func initConnPool(dc datastore.DatabaseConfig) (*sql.DB, error) { | |||
func initSessionStore(db *sql.DB, databaseProvider string, pc interfaces.PortalConfig, sessionExpiry int) (HttpSessionStore, error) { | |||
log.Debug("initSessionStore") | |||
|
|||
sessionsTable := "sessions" | |||
|
|||
// Store depends on the DB Type | |||
if databaseProvider == "pgsql" { |
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.
Use constant for pgsql ?
|
||
_, err := txn.Exec(createTokens) | ||
if err != nil { | ||
fmt.Printf("Failed ot migrate due to: %v", err) |
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.
typo "ot"
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 function has no return params - how do we report that the migration failed - all we are doing is printing an error.
|
||
_, err = txn.Exec(createCnsisTable) | ||
if err != nil { | ||
fmt.Printf("Failed ot migrate due to: %v", err) |
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.
Typo "ot"
createIndex := "CREATE INDEX tokens_user_guid ON tokens (user_guid);" | ||
_, err = txn.Exec(createIndex) | ||
if err != nil { | ||
fmt.Printf("Failed ot migrate due to: %v", err) |
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.
Typo "ot"
createIndex = "CREATE INDEX tokens_cnsi_guid ON tokens (cnsi_guid);" | ||
_, err = txn.Exec(createIndex) | ||
if err != nil { | ||
fmt.Printf("Failed ot migrate due to: %v", err) |
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.
Same typo - there are many, please fix!
|
||
_, err := txn.Exec(consoleConfigTable) | ||
if err != nil { | ||
fmt.Printf("Failed ot migrate due to: %v", err) |
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.
Same typo - many occurrences
@nwmac I've addressed all comments. Once this is merged I'll update the PRs that depend on this. This should be merged without squashing, since that will simplify the other PRs |
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.
LGRM
go
migrations