Skip to content

Added support for migrations with comments (mysql only) #25

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.

Sign up for GitHub

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

Open
bwjohnson-ss wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added support for migrations with comments (mysql only) #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions gomigrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"os"
"path/filepath"
"sort"
"strings"
)

type migrationType string
Expand Down Expand Up @@ -238,6 +239,18 @@ func (m *Migrator) ApplyMigration(migration *Migration, mType migrationType) err

// 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") {
Copy link
Owner

@DavidHuie DavidHuie Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to normalize the query here with strings.ToLower?

Copy link
Author

@bwjohnson-ss bwjohnson-ss Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

row := transaction.QueryRow(cmd)
var comment string
err = row.Scan(&comment)
if err != nil {
m.logger.Printf("Error executing query comment: %v", err)
return err
}
m.logger.Printf("Comment: %v", comment)
Copy link
Owner

@DavidHuie DavidHuie Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I like the term "comment" to refer to this. Maybe "debug output" works best?

Copy link
Author

@bwjohnson-ss bwjohnson-ss Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@matt-subsplash matt-subsplash Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

continue
}
result, err := transaction.Exec(cmd)
if err != nil {
m.logger.Printf("Error executing migration: %v", err)
Expand Down
4 changes: 4 additions & 0 deletions test_migrations/test1_mysql/1_test_up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@ CREATE TABLE test (
PRIMARY KEY (id)
);

SELECT 'my comment' AS comment;
Copy link
Owner

@DavidHuie DavidHuie Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps any select should be printed out? I see no other reason to include selects in migrations.

Copy link
Author

@bwjohnson-ss bwjohnson-ss Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


CREATE TABLE test2 (
id INT NOT NULL AUTO_INCREMENT,
PRIMARY KEY (id)
);

SELECT 'my other comment' AS comment;

CREATE TABLE tt (c text NOT NULL);

INSERT INTO tt VALUES('a');
Expand Down