Skip to content
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

Jump to bottom

feat: enable Provider verbose logging for statements #851

Merged
merged 1 commit into from
Nov 20, 2024
Merged

feat: enable Provider verbose logging for statements #851

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
20 changes: 12 additions & 8 deletions provider_run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ func (p *Provider) runIndividually(
}
if useTx {
return beginTx(ctx, conn, func(tx *sql.Tx) error {
if err := runMigration(ctx, tx, m, direction); err != nil {
if err := p.runMigration(ctx, tx, m, direction); err != nil {
return err
}
return p.maybeInsertOrDelete(ctx, tx, m.Version, direction)
Expand All @@ -189,12 +189,12 @@ func (p *Provider) runIndividually(
//
// For now, we guard against this scenario by checking the max open connections and
// returning an error in the prepareMigration function.
if err := runMigration(ctx, p.db, m, direction); err != nil {
if err := p.runMigration(ctx, p.db, m, direction); err != nil {
return err
}
return p.maybeInsertOrDelete(ctx, p.db, m.Version, direction)
case TypeSQL:
if err := runMigration(ctx, conn, m, direction); err != nil {
if err := p.runMigration(ctx, conn, m, direction); err != nil {
return err
}
return p.maybeInsertOrDelete(ctx, conn, m.Version, direction)
Expand Down Expand Up @@ -382,19 +382,19 @@ func isEmpty(m *Migration, direction bool) bool {

// runMigration is a helper function that runs the migration in the given direction. It must only be
// called after the migration has been parsed and initialized.
func runMigration(ctx context.Context, db database.DBTxConn, m *Migration, direction bool) error {
func (p *Provider) runMigration(ctx context.Context, db database.DBTxConn, m *Migration, direction bool) error {
switch m.Type {
case TypeGo:
return runGo(ctx, db, m, direction)
return p.runGo(ctx, db, m, direction)
case TypeSQL:
return runSQL(ctx, db, m, direction)
return p.runSQL(ctx, db, m, direction)
}
return fmt.Errorf("invalid migration type: %q", m.Type)
}

// runGo is a helper function that runs the given Go functions in the given direction. It must only
// be called after the migration has been initialized.
func runGo(ctx context.Context, db database.DBTxConn, m *Migration, direction bool) (retErr error) {
func (p *Provider) runGo(ctx context.Context, db database.DBTxConn, m *Migration, direction bool) (retErr error) {
defer func() {
if r := recover(); r != nil {
retErr = fmt.Errorf("panic: %v\n%s", r, debug.Stack())
Expand Down Expand Up @@ -426,7 +426,8 @@ func runGo(ctx context.Context, db database.DBTxConn, m *Migration, direction bo

// runSQL is a helper function that runs the given SQL statements in the given direction. It must
// only be called after the migration has been parsed.
func runSQL(ctx context.Context, db database.DBTxConn, m *Migration, direction bool) error {
func (p *Provider) runSQL(ctx context.Context, db database.DBTxConn, m *Migration, direction bool) error {

if !m.sql.Parsed {
return fmt.Errorf("sql migrations must be parsed")
}
Expand All @@ -437,6 +438,9 @@ func runSQL(ctx context.Context, db database.DBTxConn, m *Migration, direction b
statements = m.sql.Down
}
for _, stmt := range statements {
if p.cfg.verbose {
p.cfg.logger.Printf("Excuting statement: %s", stmt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I want these in my logs when enabling verbose logging. I.e., I care about the final output of the migration, and less about the individual statements. This almost feels more like debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tricky then, because this is exactly the same as

goose/migration_sql.go

Line 38 in 512a496

verboseInfo("Executing statement: %s\n", clearStatement(query))

Here, I've just added the same behaviour for the provider as the goose usage without provider.

This almost feels more like debugging.

Not really, since this is executed in an ideal world under some CD tooling, by any team, if would be nice to ensure that we have enough logs to assess a situation.
We had an issue where adding 40 columns, only 14 where added. We discovered why, but it would have helped to have the Goose Executing Statement log.

For me, this is verbose logging behaviour that is supposed to be handled the right way by the provider. If, what is WithVerbose used for ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay fair, I think I can get behind this.

}
if _, err := db.ExecContext(ctx, stmt); err != nil {
return err
}
Expand Down