Feature/2690: Multiple migration directories #2735

kibertoad · 2018-07-28T13:28:15Z

No description provided.

…re/2690-multiple-migration-directories

kibertoad · 2018-07-28T13:48:35Z

(missing an essential test and some necessary code, closing for now)

kibertoad · 2018-07-29T23:16:15Z

Still broken, need to figure out the black magic of running integration tests locally and fix what's remaining.

kibertoad · 2018-08-06T23:17:45Z

src/migrate/Migrator.js

@@ -42,6 +40,7 @@ const CONFIG_DEFAULT = Object.freeze({
  schemaName: null,
  directory: './migrations',
  disableTransactions: false,
+  sortDirsSeparately: false,


The idea behind this flag is that while we are forced to sort all migrations at once for backwards compatibility (so that their execution order does not change), it is typically much more convenient to execute directories sequentially and only worry about migration ordering within the scope of a single directory (where it is very visible).

kibertoad · 2018-08-06T23:18:24Z

@elhigu This ended up being way more exhausting than I originally estimated it to be, but it finally seems to be done. Please review :)

kibertoad · 2018-08-10T09:58:42Z

@tgriesser @elhigu Any chance for a review :)?

elhigu

I gave this brief review and everything did look nice and clean and tested. I would appreciate someone else giving a review too, but I'll merge this in a week if no one else checks this out. It seems to be backwards compatible so at least it shouldn't break anything :)

PIng me near to end of the month :)

JakeGinnivan · 2018-08-20T06:12:12Z

Hi, this is actually quite related to what I would like to do, but it would be great to make the knex migrations plugable.

The existing could become knex-file-migrations. I would like to have an in memory provider which would allow me to put my migrations into my bundle, solving a bunch of code sharing and build issues for me.

Thoughts?

elhigu · 2018-08-20T06:41:22Z

@JakeGinnivan I wouldn't like to have it separated to different npm package, which would increase overhead for developers. Also I don't understand with that description how exactly you would like to make it plugable. Please open feature request about it with detailed info. This is not good place to talk about it.

JakeGinnivan · 2018-08-20T07:45:05Z

Will just open a pull request and use that as a discussion point :)

elhigu · 2018-08-23T10:36:34Z

CHANGELOG.md

@@ -3,6 +3,8 @@

 # 0.15.2 - 19 Jul, 2018

+- Introduced support for specifying multiple directories for the Migrator


This actually seems to be in wrong position in changelog and needs link to PR number

Right, sorry, will move to unreleased part.

elhigu · 2018-08-23T10:41:09Z

src/migrate/Migrator.js

@@ -295,9 +299,17 @@ export default class Migrator {
  // passing any `variables` given in the config to the template.
  _writeNewMigration(name, tmpl) {
    const { config } = this;
-    const dir = this._absoluteConfigDir();
+    const dirs = this._absoluteConfigDir();
+    if (dirs.length > 1) {


Maybe here it would be more usable to use the last migration dir where to put new migration and add that to documentation. Otherwise one would always need separate configuration just for being able to create new migration...

elhigu · 2018-08-23T10:45:02Z

src/migrate/Migrator.js

@@ -97,7 +93,7 @@ export default class Migrator {
        .tap(validateMigrationList)
        .then((val) => this._getLastBatch(val))
        .then((migrations) => {
-          return this._runBatch(map(migrations, 'name'), 'down');
+          return this._runBatch(migrations, 'down');


Was _runBatch somehow changed to allow to take migration list as an input instead of migration names?

_runBatch per se no, it merely passes migrations param to getNewMigrations method which does expect the migration list.

This change was needed because batch execution needs to be aware of directories as well.

JakeGinnivan · 2018-08-24T03:10:15Z

Hey @kibertoad and @elhigu, don't want to complicate this. But if #2775 goes ahead, conceptually the migrator is not running against folders anymore.

The same refactoring would still work but it would be a missed opportunity to simply put this logic inside the FsMigrations class. It would also significantly reduce the changes to the Migrator class.

Because the PRs are so related, it would be great to talk about my PR in relation to this one. Thoughts?

JakeGinnivan · 2018-08-24T03:12:31Z

Obviously my preference would be to have this PR rebase on top of my PR.

Another side note, is this behaviour could be gained without a PR to knex at all if someone wanted a different behaviour. This one makes sense to put in core, but not all custom migration layouts would.

kibertoad · 2018-08-24T08:39:28Z

@elhigu All comments addressed, build pass. Is anything remaining?

elhigu · 2018-08-24T09:39:16Z

@JakeGinnivan thanks for the heads up. I haven't had time to check that your PR yet to see how it reflects to this one to decide which should be on top of which.

elhigu · 2018-08-24T09:39:44Z

Thanks @kibertoad !

falkenhawk · 2018-08-24T09:47:21Z

i'm afraid that knex migrate:make command is not covered - where will it put a new migration when mutliple directories are configured?

kibertoad · 2018-08-24T09:48:24Z

@unhawkable See 306b8bd

Probably I should update documentation to codify that behaviour.

falkenhawk · 2018-08-24T09:52:53Z

ok thanks i see now. It will take the last dir. It would be super-cool though if it would ask for a directory to put the new migration in in an interactive mode with a list to choose from 👍 - just a suggestion ;)

elhigu · 2018-08-24T10:01:21Z

@unhawkable yes, there is room for improvement if someone needs better control on that.

JakeGinnivan · 2018-08-25T02:20:59Z

@elhigu @kibertoad while working on updating #2775 I think I found a small issue. If you have migrations with the same name in multiple folders then there will be errors as only the filename is stored as the migration name in the migrations table.

I am going to update this to be the full relative path to the migration.

JakeGinnivan · 2018-08-25T03:52:41Z

Ah, I see now. If you store the relative path it may introduce a number of other compatibility issues and breaking changes. Ignore ☝️

elhigu · 2018-08-29T15:52:47Z

yep, and prevent moving migration from one directory to another.

* Kill queries after timeout for PostgreSQL * Fix cancellation connection acquiring and test. * Fix releasing connection in case query cancellation fails * Add support for native enums on Postgres (#2632) Reference https://www.postgresql.org/docs/current/static/sql-createtype.html Closes #394 Signed-off-by: Will Soto <will.soto9@gmail.com> * Removal of 'skim' (#2520) * Allow overwriting log functions (#2625) * Example build of custom log functions * Handle logger object better for transactions * Adjust test to ignore sqlite warning message * Fixed onIn with empty values array (#2513) * Drop support for strong-oracle (#2487) * Remove babel-plugin-lodash (#2634) While in theory, this may reduce the bundle size, in practice it adds a ton of overhead during startup due to the number of additional requires. Bundle size also shouldn't matter for server side modules. * Add json support to the query builder for mysql (#2635) * Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version * fix wrapIdentifier not being called in postgres alter column (#2612) * fix wrapIdentifier not being called in postgres alter column * add test for wrapIdentifier call in postgres alter column * add comment regarding issue * add issue & PR #'s in comment * Remove readable-stream and safe-buffer (#2640) * Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version * Use native Buffer and Stream implementations * fixes 2630 (#2642) * Timeout errors shouldn't silently ignore the passed errors, but rather reject with original error. Fixes #2582 (#2626) * chore: add Node.js 10 (#2594) * chore: add Node.js 10 * chore: trigger new build * chore: update lockfile * chore: trigger new build * fix: use npm i instead of npm ci * Fix mssql driver crashing (#2637) * Run SQL Server tests locally running SQL server in docker * WIP mssql test stuff * Patched MSSQL driver to not crash knex anymore * Removed semicolon from rollback stmt for oracle (#2564) * Remove WebSQL dialect (#2647) * Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version * Use native Buffer and Stream implementations * Remove WebSQL dialect * add homepage field to package.json (#2650) * Make the stream catch errors in the query (#2638) * Make the stream catch errors in the query * Fix another case in which stream doesnt emits error * Linter stuff * Remove setTimeout in tests * Make a test not to check the MySQL error code * Fix stream error catching for MariaDB and PostgreSQL * Fix stream error catching in Oracle * Throw the error after emitting it to the stream * Throw the error without instantiating a new Error * Various fixes to mssql dialect (#2653) * Fixed float type of mssql to be float * Many tests where postgres test was not actually ran at all * Migrations to be mssql compatible Mssql driver doesn't handle if multiple queries are sent to same transaction concurrently. * Prevented mssql failing when invalid schema builder was executed by accident Instead of trying to generate sql from broken schema calls, just make exception to leak before query compiling is started * Fixed mssql trx rollback to always throw an error Also modified some connection test query to be mssql compatible * Fixed various bugs from MSSQL driver to make tests run * Fixed mssql unique index to be compatible with other dialect implementations * Enable running mssql tests on CI * Test for #2588 * Updated tests to not be dependend on tables left from previous test rans * Trying to make mssql server work on travis * Updated changelog and version * Drop mariadb support (#2681) * Dropped support for mariasql * ESLint * Fixed docs to build again * Fix knex.initialize() and adds test (#2477) * Fix knex.initialize() and adds test * Fix knex.initialize() and adds test * Added test for reinitializing pool after destroy * Fixed destroy / reinitialization test * Fixed the fix * Implement missing schema support for mssql dialect * chore: Update dependencies. Remove estraverse (#2691) * Update dependencies. Remove estraverse * Fix compatibility with new Sinon * Increase mssql timeout * Normalized and validated driverNames of test db clients and fixed oracle test setup (#2692) * Normalized and validated driverNames of test db clients and fixed oracle test setup * Fixed failed queries from old query building tests which hadn't been ran in ages * Allow selecting node version which is used to run oracledb docker tests * Improved sql tester error messages * Fixed rest of the oracledb tests * Removed invalid flag from docker-compose * Print mssql logs if initialization fails * Fixed syntax error + final tests * Added restart of failure for mssql DB initialization to try again if server was not ready * Printout always mssql logs after container is started * Fixed wait time printing after trying to connect * Use npm run oracledb:test for testing oracle in travis * Add Prettier (#2697) * Add prettier * Run files through prettier * Istanbul -> NYC (#2700) * istanbul -> nyc * Update mocha * Enforce code coverage (#2702) * Enforce code coverage * Update coverage numbers with current values * version assignment on base class, copy on tx client, fix #2705 * Update changelog * v0.15.1 * Added build step to test script. Fixes #2541 (#2712) * Revert "Added build step to test script. Fixes #2541 (#2712)" (#2714) This reverts commit 90ed8db. * Allow oracle failures for now * Fix issue with select(0) (#2711) * Fix issue with select(0). Fixes #2658 * Refactor migrator (#2695) * Refactor migrator * Fix exports * Fix ESLint * Fix migrator * Fix reference to config * Split some more * Fix table builder * Fix argument order * Merge branch 'master' of https://github.com/tgriesser/knex into feature/2690-support-multiple-migration-dirs # Conflicts: # src/migrate/index.js # test/index.js # test/unit/migrate/migrator.js * Fix #2715 (#2721) * Fix #2715, explicitly set precision in datetime & timestamp * Allow for precision in knex.fn.now, mysql time * Add test for datetime with precision * Bump changelog * 0.15.2 * Fix issues with warnPromise when migration does not return a promise. Fixes #2725 (#2730) * Add tests for multiple union arguments with callbacks and builders (#2749) * Add tests for multiple union arguments * Add some callback and raw tests to union unit tests * Compile with before update so that bindings are put in correct order (#2733) * Fix join using builder withSchema. (#2744) * Use Datetime2 for MSSQL datetime + timestamp types (#2757) * Use Datetime2 for MSSQL datetime + timestamp types Datetime2 is now the recommended column type for new date work: https://docs.microsoft.com/en-us/sql/t-sql/data-types/datetime-transact-sql?view=sql-server-2017 * Add tests for MSSQL datetime2 changes * General/document breaking change (#2774) * Add changelog entry for a breaking change * Improve entry * Allow timestamp with timezone on mssql databases (#2724) * Allow timestamp with timezone on mssql databases * Change timestamp parameter to use object instead of boolean * Update dependencies (#2772) * Feature/2690: Multiple migration directories (#2735) * Implement support for multiple migration directories * Add tests for new functionality * Fix tape tests * Pass migration directory upwards * Fix multiple directory support * Fix bugs * Rollback correctly * Fix remaining tests * Address comments * #2758: Implement fail-fast logic for dialect resolution (#2776) * Implement fail-fast logic for dialect resolution, clean-up code around. * Remove method that was deprecated long time ago * Address additional comments * Try addressing comments * Set client explicitly * Fix compatibility with older Node versions * Use columnize instead of wrap in using(). (#2713) * Use columnize instead of wrap in using(). This is an attempt to fix #2136. Also added an integration test, couldn't find any existing. * Exclude MSSQL from test as it does not support .using * Change test to not use subquery * Change test * Introduced abstraction for getting migrations (#2775) * Introduced abstraction for getting migrations This would allow a webpack migration source which is compatible with bundling. * Fixed migration validation with custom migration source * Fixed issues after rebasing on muti directory PR * Renamed parameter and fixed error message * Addressed some PR comments * Finished comment * Moved filename extension filtering into fs-migrator * Added test showing in memory custom migration source * Cleaned up how to get config * Fixed failing test * Hopefully fix tests * Fix Node.js 10 support in tests * Test for correctly releasing cancel query connection

kibertoad added 4 commits July 22, 2018 23:15

Implement support for multiple migration directories

dd6e1e2

Add tests for new functionality

3bb71d6

Merge branch 'master' of https://github.com/tgriesser/knex into featu…

c10869c

…re/2690-multiple-migration-directories

Fix tape tests

3b5e8ba

kibertoad closed this Jul 28, 2018

Pass migration directory upwards

97a5e5d

kibertoad reopened this Jul 29, 2018

Fix multiple directory support

2df3e74

kibertoad closed this Jul 29, 2018

Fix bugs

a52735c

kibertoad reopened this Aug 6, 2018

kibertoad closed this Aug 6, 2018

kibertoad reopened this Aug 6, 2018

kibertoad closed this Aug 6, 2018

kibertoad reopened this Aug 6, 2018

kibertoad added 2 commits August 7, 2018 00:30

Rollback correctly

2f8af01

Fix remaining tests

e4a7325

kibertoad commented Aug 6, 2018

View reviewed changes

sgoll mentioned this pull request Aug 8, 2018

Ignore duplicate variants of same migration (e.g. .js/.ts) #2756

Open

elhigu approved these changes Aug 18, 2018

View reviewed changes

kibertoad mentioned this pull request Aug 19, 2018

Document changes in migration config knex/documentation#129

Merged

JakeGinnivan mentioned this pull request Aug 22, 2018

Introduced abstraction for getting migrations #2775

Merged

elhigu reviewed Aug 23, 2018

View reviewed changes

kibertoad added 2 commits August 24, 2018 00:21

Address comments

306b8bd

Merge

6a334e5

elhigu merged commit 91f23bc into knex:master Aug 24, 2018

elhigu mentioned this pull request Aug 24, 2018

Support multiple migration directories #2690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/2690: Multiple migration directories #2735

Feature/2690: Multiple migration directories #2735

kibertoad commented Jul 28, 2018

kibertoad commented Jul 28, 2018

kibertoad commented Jul 29, 2018

kibertoad Aug 6, 2018

kibertoad commented Aug 6, 2018

kibertoad commented Aug 10, 2018

elhigu left a comment •

edited

JakeGinnivan commented Aug 20, 2018

elhigu commented Aug 20, 2018

JakeGinnivan commented Aug 20, 2018

elhigu Aug 23, 2018

kibertoad Aug 23, 2018

elhigu Aug 23, 2018

kibertoad Aug 23, 2018

elhigu Aug 23, 2018

kibertoad Aug 23, 2018

kibertoad Aug 23, 2018

JakeGinnivan commented Aug 24, 2018

JakeGinnivan commented Aug 24, 2018

kibertoad commented Aug 24, 2018

elhigu commented Aug 24, 2018

elhigu commented Aug 24, 2018

falkenhawk commented Aug 24, 2018

kibertoad commented Aug 24, 2018

falkenhawk commented Aug 24, 2018

elhigu commented Aug 24, 2018

JakeGinnivan commented Aug 25, 2018

JakeGinnivan commented Aug 25, 2018

elhigu commented Aug 29, 2018

		@@ -3,6 +3,8 @@

		# 0.15.2 - 19 Jul, 2018

		- Introduced support for specifying multiple directories for the Migrator

Feature/2690: Multiple migration directories #2735

Feature/2690: Multiple migration directories #2735

Conversation

kibertoad commented Jul 28, 2018

kibertoad commented Jul 28, 2018

kibertoad commented Jul 29, 2018

Choose a reason for hiding this comment

kibertoad commented Aug 6, 2018

kibertoad commented Aug 10, 2018

elhigu left a comment • edited

Choose a reason for hiding this comment

JakeGinnivan commented Aug 20, 2018

elhigu commented Aug 20, 2018

JakeGinnivan commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeGinnivan commented Aug 24, 2018

JakeGinnivan commented Aug 24, 2018

kibertoad commented Aug 24, 2018

elhigu commented Aug 24, 2018

elhigu commented Aug 24, 2018

falkenhawk commented Aug 24, 2018

kibertoad commented Aug 24, 2018

falkenhawk commented Aug 24, 2018

elhigu commented Aug 24, 2018

JakeGinnivan commented Aug 25, 2018

JakeGinnivan commented Aug 25, 2018

elhigu commented Aug 29, 2018

elhigu left a comment •

edited