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

Don't print migration output when running the initial setup migration #1023

Closed
sgrif opened this Issue Jul 13, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@sgrif
Member

sgrif commented Jul 13, 2017

I mostly want the initial setup migration to be invisible. We shouldn't have this in the output:

Running migration 00000000000000

This just requires special casing that version in the function which prints that line.

@juliusdelta

This comment has been minimized.

Contributor

juliusdelta commented Jul 20, 2017

Would love to work on this @sgrif if that's cool.

@Eijebong

This comment has been minimized.

Member

Eijebong commented Jul 20, 2017

Sure 👍

@juliusdelta

This comment has been minimized.

Contributor

juliusdelta commented Jul 20, 2017

Awesome. i'll try to submit a PR tomorrow afternoon. Ty

@juliusdelta

This comment has been minimized.

Contributor

juliusdelta commented Jul 20, 2017

So there's a few ways to accomplish this, but the most apparent but "not so elegant" way to do it is wrapping an if statement before the writeln!() function that outputs Running Migration 00000000000000.

Something like:
/diesel/src/migrations/mod.rs#L267-L277

fn run_migration<Conn>(conn: &Conn, migration: &Migration, output: &mut Write)
    -> Result<(), RunMigrationsError> where
        Conn: MigrationConnection,
{
    conn.transaction(|| {
        if migration.version() !== "00000000000000" {
            try!(writeln!(output, "Running migration {}", migration.version()));
        }
        try!(migration.run(conn));
        try!(conn.insert_new_migration(migration.version()));
        Ok(())
    })
}

It definitely seems like it could be cleaner but I didn't want to over think it.

The thought hole I went down was: at this point the version field is already defined on the migration at this point right? So just checking if it exists isn't possible in this scenario.

I suppose I could also check if the first version equals the last version which might look nicer imo but it's not as clear if you're reading it -> If I'm understanding the architecture correctly that is.

Thoughts? @sgrif @Eijebong

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 20, 2017

That change is pretty much exactly what I want. There's really no need to overthink it (we should probably have a test that diesel setup followed by diesel migration run has no output lines with "Running migration") to make sure that a future change to the versioning doesn't break this)

@juliusdelta

This comment has been minimized.

Contributor

juliusdelta commented Jul 20, 2017

Sounds good! 👍 I'll get that done. Thanks for the direction!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment