knex / knex

A query builder for PostgreSQL, MySQL, CockroachDB, SQL Server, SQLite3 and Oracle, designed to be flexible, portable, and fun to use.
https://knexjs.org/
MIT License
18.96k stars 2.1k forks source link

knex migration doesn't find the config file #2952

Open francoisromain opened 5 years ago

francoisromain commented 5 years ago

Environment

Knex version: 0.16.2 Database + version: postgres 11 OS: mac os 10.13.6

Select applicable tempalate from below. If issue is about oracledb support tag @ atiertant. For MSSql tag @ smorey2 . Rest of dialects doesn't need tags.

Bug

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

the knex config file: /tools/knex/knex.js

require('dotenv').config()

const connection = {
  host: process.env.PGHOST,
  port: process.env.PGPORT,
  database: process.env.PGDATABASE,
  user: process.env.PGUSER,
  password: process.env.PGPASSWORD
}

const knexConfig = {
  client: 'pg',
  connection,
  migrations: {
    directory: './tools/knex/migrations'
  },
  seeds: {
    directory: './tools/knex/seeds'
  }
}

module.exports = knexConfig

when I run npx knex --knexfile=./tools/knex/config.js migrate:rollback, it makes an error Error: ENOENT: no such file or directory, scandir '/Volumes/HD/Documents/my-app/migrations'

from the config file it should look for migrations in ./tools/knex/migrations and not in ./migrations

This was working well in previous knex version 0.15.2

jlsan92 commented 5 years ago

Same issue here! 💣

kibertoad commented 5 years ago

Could you please provide more details about the use-case? Which directory are you running the command from? Also by the looks of it it doesn't fail to find config file, it fails to find migrations directory - or am I misunderstanding something?

francoisromain commented 5 years ago

Hello @kibertoad I run the command from the root of the project with npx knex --knexfile=./tools/knex/config.js migrate:rollback. The config file is located at ./tools/knex/knex.js and the migrations are at ./tools/knex/migrations.

The error says it look for the migrations directory at its default place (./migrations), while the config file indicates it's located at ./tools/knex/migrations.

So my guess is it didn't find the config file.

You can find this project here: https://github.com/MTES-MCT/camino-api, currently working with this conf and knex 0.15.2 and bugging on 0.16.2.

thank you

kibertoad commented 5 years ago

I've just reproduced the issue, and as I was suspecting, problem is not with config file resolution - it throws a different error when that happens, "Cannot found module <...>". Problem seems to be with resolving path to migrations. Will try to address it now.

kibertoad commented 5 years ago

TBH, I'm not sure previous knex behaviour was correct to begin with. --knexfile used to duplicate behaviour of CWD param - path to migrations and seeds was calculated not relatively to CWD, but relatively to knexfile, which wasn't documented @elhigu Is it desired behaviour? Should we calculate path to migrations and seeds relatively from path to knexfile?

elhigu commented 5 years ago

I think that the correct default behaviour is to have directories resolved relative to knexfile by default. Not relative to directory where client command has been ran.

kibertoad commented 5 years ago

@elhigu OK, will do the change now then.

kibertoad commented 5 years ago

0.16.3 is out with a fix for this.

kibertoad commented 5 years ago

@francoisromain Could you please check if a new version resolves issue for you?

francoisromain commented 5 years ago

@kibertoad yes, this works now https://github.com/MTES-MCT/camino-api/commit/043a3899bd7be9743b9588f3680fc4b2eb4e4dc6. Thank you very much for the quick fix.

kibertoad commented 5 years ago

np!

demisx commented 5 years ago

This change actually broke the builds for us. We use knexfile.js in different places (code and npm scripts), e.g.:

// in test-helper.ts
const knexConfig = require('../../config/db/knexfile')
const env = appConfig.get('env')
const config = knexConfig[env].migrations
...
await Model.knex().migrate.latest(config)
...
// npm scripts  in package.json
{
  ...
  "db:migrate": "knex --knexfile config/db/knexfile.js migrate:latest",
}

It worked properly prior to v 16.3 where both would expect path relative to current dir, but after 16.3 upgrade, now one expects a relative path to current directory and the other one relative to knexfile.js location. Not sure how to make them both happy now w/o creating symlinks.

kibertoad commented 5 years ago

Hell yeah, one more edgecase :D. @elhigu Do you think we should modify non-CLI migrations to behave like CLI migrations in this case? Alternatively we may try to strictly reconstruct 0.15.x behavior, but considering that it wasn't documented nor covered with tests...

kibertoad commented 5 years ago

@demisx Do I understand correctly that problem is with how path to migrations/seeds is resolved?

elhigu commented 5 years ago

I think its ok to figure out some reasonable set of functionality which enables as wide range use cases as possible and document + test that. If 0.15.x behavior was good and enables all necessary use cases that would be the preference-

kibertoad commented 5 years ago

@demisx Could you please check how 0.15.x behaved under your circumstances and if it would be an improvement over how 0.16.3 works for you?

demisx commented 5 years ago

I've just tried 0.15.2 and didn't notice any difference with 0.16.3. I still had to set different paths in knexfile.js depending whether it is required from source code or being passed as a CLI argument. The version that works the best for us a is the previous 0.16.2. Looks like the path there is relative to the project root directory and all paths in knexfile.js are relative to the project root. This 0.16.2 behavior is ideal for us as it allows to keep consistent paths across all environments:

// In knexfiles.js (v0.16.2)
...
  development: {
    client: 'pg',
    migrations: {
      directory: 'config/db/migrations',
    },
    seeds: {
      directory: 'config/db/seeds',
    },
  },

  test: {
    client: 'pg',
    migrations: {
      directory: 'config/db/migrations',
    },
    seeds: {
      directory: 'config/db/seeds',
    },
    // debug: true,
  },

  e2e: {
    client: 'pg',
    migrations: {
      directory: 'config/db/migrations',
    },
    seeds: {
      directory: 'config/db/seeds',
    },
  },

  ci: {
    client: 'pg',
    migrations: {
      directory: 'config/db/migrations',
    },
    seeds: {
      directory: 'config/db/seeds',
    },
  },
...
kibertoad commented 5 years ago

@demisx Thanks for checking it! @elhigu I wonder if we could use some kind of a config switch here?..

elhigu commented 5 years ago

You mean switch in command line client or new attribute in knex config? The first solution would be fine, latter solution I would not like. Its hard to think what would be the best solution for this, since I dont know all the use cases nor I have had to put deep though in this.

kibertoad commented 5 years ago

Either would work for me, if you prefer CLI parameter we can go with it.

developer239 commented 5 years ago

I am using typescript my knexfile.ts looks like this:

require('dotenv').config() // tslint:disable-line:no-var-requires

import dbConfig from './src/database/sql/dbConfig'

module.exports = dbConfig

I had to tell knex that it has to look for knexfile.ts.

Repository with working setup is here: https://github.com/developer239/localized-graphql-koa-typescript Commit that fixed the issue is here: https://github.com/developer239/localized-graphql-koa-typescript/commit/170f024e1bbd698e0704b7617db4717a8e08a161

image

stf8 commented 5 years ago

We experienced the same drawback @demisx described (we moved from 0.15.2 to 0.17.5 recently). My understanding is that now the behavior of the knex cli is not consistant with a typical node require loading.

Eg, in our apps we have :

const knex = require('knex')(require('./config/knexfile))[process.env.NODE_ENV || 'development'];

While at the same time one could use :

./node_modules/knex/bin/cli.js --cwd . --knexfile ./config/knexfile migrate:run

It seems to me the --cwd option is now kinda misleading as it's really not the working directory from which one could expect every path to be resolved, whatever is the actual location of the knexfile.

As of now I did not find a clean workaround, one could hack the required knexfile, so that some stuff are ignored, but the very same problem happens with seeds, and we also load a bunch of files from them to fill in the db (both cli and from node tooling) so playing with directory properties in the knexfile did not work.

In the end as a quick & dirty way to have our CI pipelines back up and running we created bunch of symlinks in the folder containing the config file (to the actual migrations/seeds folders but also to any folder that might be used in seeds using require or fs.read*).

kibertoad commented 5 years ago

@stf8 I'll try to address it this week.

marniwild commented 3 years ago

Probably didn't do a npx knex init. Solved problem for me