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.

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

Overly verbose logging when migrations directory specified #3921

Closed
anandnimkar opened this issue Jul 11, 2020 · 15 comments
Closed

Overly verbose logging when migrations directory specified #3921

anandnimkar opened this issue Jul 11, 2020 · 15 comments

Comments

@anandnimkar
Copy link
Contributor

anandnimkar commented Jul 11, 2020

Environment

Knex version: 0.21.2
Database + version: Irrelevant
OS: Irrelevant

Bug

  1. Explain what kind of behaviour you are getting and how you think it should do

Due to changes introduced in #3839, on every call to migrate.latest() I now get two messages in my logs stating "FS-related option specified for migration configuration. This resets migrationSource to default FsMigrations"

I think this is overly verbose and possibly unnecessary. I run migrations programmatically at the start of every invocation of a Lambda function and now this is showing up in my logs.

At the very least, can we just have this message once per migrate.latest() call? Because getMergedConfig (in the file lib/migrate/configuration-merger.js) is called multiple times, it is showing up twice.

I also introduced a separate issue to ensure that getMergedConfig is called with the appropriate client logger when supplied. See #3919 and #3920.

  1. Error message

See 1 above.

  1. Reduced test code, for example in https://npm.runkit.com/knex or if it needs real
    database connection to MySQL or PostgreSQL, then single file example which initializes
    needed data and demonstrates the problem.

Set migrations directory in knexfile, then run migrate.latest() or migrate.rollback()

@ksmithut
Copy link
Contributor

ksmithut commented Jul 14, 2020

I might also add that I'm fine if there's a more preferred method of setting custom migration directories (perhaps setting migrationSource to an instance of FsMigrations), but it would be nice if it were documented (and supported in the types).

Edit: I still get one of the warnings when not specifying any fs related options when running knex.migrate.make() and it's not respecting what I put into migrationSource:

const Knex = require('knex')
const { FsMigrations } = require('knex/lib/migrate/sources/fs-migrations')

const knex = Knex({
  client: 'pg',
  connection: 'postgres://postgres:postgres@localhost:5432/postgres',
  migrations: {
    // directory: 'migrations2',
    tableName: 'migrations',
    migrationSource: new FsMigrations('migrations2', false)
  }
})

knex.migrate.make('test')

@elhigu
Copy link
Member

elhigu commented Jul 15, 2020

You can set migration directory in configuration (https://knexjs.org/#Migrations-API)

{
  migrations: {
    directory: ['myCustomMigrationDir1','myCustomMigrationDir2']
  }
}

@ksmithut
Copy link
Contributor

@elhigu Right, the problem is that when using the knex js api for running migrations, you get verbose logging "FS-related option specified for migration configuration. This resets migrationSource to default FsMigrations" when using that option using the latest version

@zjr
Copy link

zjr commented Jul 16, 2020

Same if running knex migrate from the command line. I get the message twice.

@jalfd
Copy link

jalfd commented Jul 17, 2020

It seems this message is logged even if you don't specify a migrationSource in your config. In that case, you don't really need to know that it's being reset to the default, do you?

We're getting this message hundreds of times when running our test suite, and it seems a bit excessive if there's not actually any problem.

@jonahmolinski
Copy link

@jalfd exactly!

There should be a way to suppress this message or clear documentation on how to avoid it.

@elhigu
Copy link
Member

elhigu commented Jul 18, 2020

@jalfd exactly!

There should be a way to suppress this message or clear documentation on how to avoid it.

Overriding loggers can be used to filter error messages (really useful in tests). So additional configuration switches for supressing specific warnings has not been added for a long time. That might not be clear from docs though.

@jonahmolinski
Copy link

Overriding loggers can be used to filter error messages (really useful in tests). So additional configuration switches for supressing specific warnings has not been added for a long time. That might not be clear from docs though.

Maybe I am missing something, but I pass in a logger via the "log" parameter in config and it's not used for this message. It is used for the debug SQL statements however. So it must be working...

How can one set the logger that is used for that message? Cheers!

@elhigu
Copy link
Member

elhigu commented Jul 18, 2020

Overriding loggers can be used to filter error messages (really useful in tests). So additional configuration switches for supressing specific warnings has not been added for a long time. That might not be clear from docs though.

Maybe I am missing something, but I pass in a logger via the "log" parameter in config and it's not used for this message. It is used for the debug SQL statements however. So it must be working...

How can one set the logger that is used for that message? Cheers!

You need to use pattern matching in logger function to check log messages to filter out. Docs how to set loggers is here https://knexjs.org/#Installation-log

@joshkel
Copy link

joshkel commented Jul 19, 2020

It looks like it's straightforward and reasonable to modify Knex to only log if the migrationSource is actually reset; see #3936.

@StephanHoyer

This comment has been minimized.

@elhigu

This comment has been minimized.

@kevholder
Copy link

kevholder commented Sep 24, 2020

@elhigu can you share the code to fix this via logger configuration so everyone that googles this has a simple solution?

Edit: this seems to work:

const knex = createKnex{
  // ...
  log: {
    warn(message) {
      // see https://github.com/knex/knex/issues/3921#issuecomment-698612208
      if (
        message ===
        "FS-related option specified for migration configuration. This resets migrationSource to default FsMigrations"
      ) {
        return;
      }
      console.log(message);
    },
  },
}

@kibertoad
Copy link
Collaborator

I'll release a proper fix for this today.

@kibertoad
Copy link
Collaborator

Fix released in 0.21.6, sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants