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

knex migration doesn't find the config file #2952

Closed
francoisromain opened this Issue Dec 12, 2018 · 21 comments

Comments

Projects
None yet
6 participants
@francoisromain
Copy link

francoisromain commented Dec 12, 2018

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

@francoisromain francoisromain changed the title knex migration don't find the config file knex migration doesn't find the config file Dec 12, 2018

francoisromain added a commit to MTES-MCT/camino-api that referenced this issue Dec 12, 2018

@jlsan92

This comment has been minimized.

Copy link

jlsan92 commented Dec 13, 2018

Same issue here! 💣

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 13, 2018

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

This comment has been minimized.

Copy link

francoisromain commented Dec 14, 2018

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 15, 2018

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 15, 2018

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

This comment has been minimized.

Copy link
Collaborator

elhigu commented Dec 16, 2018

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 16, 2018

@elhigu OK, will do the change now then.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 19, 2018

0.16.3 is out with a fix for this.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 19, 2018

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

@francoisromain

This comment has been minimized.

Copy link

francoisromain commented Dec 19, 2018

@kibertoad yes, this works now MTES-MCT/camino-api@043a389. Thank you very much for the quick fix.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 19, 2018

np!

@demisx

This comment has been minimized.

Copy link

demisx commented Dec 20, 2018

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 20, 2018

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 20, 2018

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

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Dec 20, 2018

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 20, 2018

@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

This comment has been minimized.

Copy link

demisx commented Dec 20, 2018

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 20, 2018

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

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Dec 21, 2018

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 21, 2018

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

@developer239

This comment has been minimized.

Copy link

developer239 commented Jan 19, 2019

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: developer239/localized-graphql-koa-typescript@170f024

image

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