From 09a87f4f8d249cbff86d97e03d9e00c82de39224 Mon Sep 17 00:00:00 2001 From: Brian Lauber Date: Wed, 12 Feb 2020 18:38:42 -0500 Subject: [PATCH 1/2] Fix: Knex CLI calls process.chdir() before opening Knexfile --- bin/cli.js | 18 +++++++------- test/cli/knexfile-test.spec.js | 21 ++++++++++++++++ .../knexfile-with-resolve.js | 24 +++++++++++++++++++ 3 files changed, 54 insertions(+), 9 deletions(-) create mode 100644 test/jake-util/knexfile-relative/knexfile-with-resolve.js diff --git a/bin/cli.js b/bin/cli.js index 3c58182053..be8a750c12 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -29,6 +29,15 @@ const fsPromised = { }; function initKnex(env, opts) { + checkLocalModule(env); + if (process.cwd() !== env.cwd) { + process.chdir(env.cwd); + console.log( + 'Working directory changed to', + color.magenta(tildify(env.cwd)) + ); + } + if (opts.esm) { // enable esm interop via 'esm' module require = require('esm')(module); @@ -48,15 +57,6 @@ function initKnex(env, opts) { env.configuration.ext = path.extname(p).replace('.', ''); } - checkLocalModule(env); - if (process.cwd() !== env.cwd) { - process.chdir(env.cwd); - console.log( - 'Working directory changed to', - color.magenta(tildify(env.cwd)) - ); - } - const resolvedConfig = resolveEnvironmentConfig(opts, env.configuration); const knex = require(env.modulePath); return knex(resolvedConfig); diff --git a/test/cli/knexfile-test.spec.js b/test/cli/knexfile-test.spec.js index e84f58edc0..159e94d088 100644 --- a/test/cli/knexfile-test.spec.js +++ b/test/cli/knexfile-test.spec.js @@ -77,6 +77,27 @@ module.exports = { ); }); + // This addresses the issue that was reported here: + // + // https://github.com/knex/knex/issues/3660 + // + context( + 'and the knexfile itself resolves paths relative to process.cwd()', + function() { + it("changes the process's cwd to the directory that contains the knexfile before opening the knexfile", () => { + const knexfile = 'test/jake-util/knexfile-relative/knexfile.js'; + const expectedCWD = tildify(path.resolve(path.dirname(knexfile))); + + return execCommand( + `node ${KNEX} migrate:latest --knexfile=test/jake-util/knexfile-relative/knexfile-with-resolve.js --knexpath=../knex.js`, + { + expectedOutput: `Working directory changed to ${expectedCWD}`, + } + ); + }); + } + ); + // FYI: This is only true because the Knex CLI changes the CWD to // the directory of the knexfile. it('Resolves migrations relatively to knexfile', () => { diff --git a/test/jake-util/knexfile-relative/knexfile-with-resolve.js b/test/jake-util/knexfile-relative/knexfile-with-resolve.js new file mode 100644 index 0000000000..52c4322529 --- /dev/null +++ b/test/jake-util/knexfile-relative/knexfile-with-resolve.js @@ -0,0 +1,24 @@ +// This knexfile recreates the issue that was identified here: +// +// https://github.com/knex/knex/issues/3660 +// + +const path = require('path'); + +// Notice: this path will resolve relative to `process.cwd()` . +// This will cause a problem if the Knex CLI opens the knexfile +// before changing the cwd. +const MIGRATIONS_DIR = path.resolve('knexfile_migrations'); + +module.exports = { + client: 'sqlite3', + connection: { + filename: __dirname + '/../test.sqlite3', + }, + migrations: { + directory: MIGRATIONS_DIR, + }, + seeds: { + directory: './knexfile_seeds', + }, +}; From 99b932fb851a34669a4e116873bef7d2b379f6d9 Mon Sep 17 00:00:00 2001 From: Brian Lauber Date: Wed, 12 Feb 2020 20:35:27 -0500 Subject: [PATCH 2/2] Cleaned up the logic for inferring config.ext ... ... this was mostly to kick off another Travis CI build since the previous one seems to have failed randomly. --- bin/cli.js | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/bin/cli.js b/bin/cli.js index be8a750c12..771425c6c0 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -28,6 +28,17 @@ const fsPromised = { writeFile: promisify(fs.writeFile), }; +function openKnexfile(configPath) { + const config = require(configPath); + + // FYI: By default, the extension for the migration files is inferred + // from the knexfile's extension. So, the following lines are in + // place for backwards compatibility purposes. + config.ext = config.ext || path.extname(configPath).replace('.', ''); + + return config; +} + function initKnex(env, opts) { checkLocalModule(env); if (process.cwd() !== env.cwd) { @@ -44,19 +55,9 @@ function initKnex(env, opts) { } env.configuration = env.configPath - ? require(env.configPath) + ? openKnexfile(env.configPath) : mkConfigObj(opts); - // FYI: By default, the extension for the migration files is inferred - // from the knexfile's extension. So, the following lines are in - // place for backwards compatibility purposes. - if (!env.configuration.ext) { - const p = env.configPath || opts.knexpath; - - // TODO: Should this property be documented somewhere? - env.configuration.ext = path.extname(p).replace('.', ''); - } - const resolvedConfig = resolveEnvironmentConfig(opts, env.configuration); const knex = require(env.modulePath); return knex(resolvedConfig);