Added support for migrations with comments (mysql only)#25
Added support for migrations with comments (mysql only)#25bwjohnson-ss wants to merge 1 commit intoDavidHuie:masterfrom
Conversation
| // Perform the migration. | ||
| for _, cmd := range commands { | ||
| cmd = strings.ReplaceAll(cmd, "\n", "") // strip off leading whitespace | ||
| if strings.HasPrefix(cmd, "SELECT") && strings.HasSuffix(cmd, "AS comment") { |
There was a problem hiding this comment.
Do we want to normalize the query here with strings.ToLower?
There was a problem hiding this comment.
I'd say yes for any SQL keywords, but that would mess up the capitalization of other strings that we may not want to mess with.
| PRIMARY KEY (id) | ||
| ); | ||
|
|
||
| SELECT 'my comment' AS comment; |
There was a problem hiding this comment.
Perhaps any select should be printed out? I see no other reason to include selects in migrations.
There was a problem hiding this comment.
I could see that being helpful, but in the migrate func I specifically chose to use the transaction.QueryRow interface func since it would always return only one row. I guess you could allow someone to run a query that returned multiple rows, but then you'd open up more possibilities and you could inadvertently spam your gomigrate output with tons of rows.
| m.logger.Printf("Error executing query comment: %v", err) | ||
| return err | ||
| } | ||
| m.logger.Printf("Comment: %v", comment) |
There was a problem hiding this comment.
I'm not sure if I like the term "comment" to refer to this. Maybe "debug output" works best?
There was a problem hiding this comment.
I can see merit in that too, but would you prefer adding a new opt-in flag on the migrator to enable SELECT comments/debug statements? I could also be fine with no prefix label and just print out the raw comment/debug statement too. Thoughts?
There was a problem hiding this comment.
Since the term comment is used in the ANSI/ISO SQL lexicon, I think calling it a comment is perfectly valid. Perhaps SQL Comment: is more descriptive?
|
@DavidHuie Any thoughts to my above comments? |
|
Ping? @DavidHuie ? |
3 participants
I wanted a way for gomigrate to print out comments/status inside a migration. This only applies for mysql, since it's the only supported db type with multi-command migrations.