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

chore: add Node.js 10 #2594

Merged
merged 5 commits into from
Jun 10, 2018
Merged

chore: add Node.js 10 #2594

merged 5 commits into from
Jun 10, 2018

Conversation

DanielRuf
Copy link
Contributor

No description provided.

@kibertoad
Copy link
Collaborator

Unfortunately, there are no prebuilt Oracle binaries for Node 10 yet, so this won't pass the CI. Not sure if we want to run tests on incomplete set of dbs.

@DanielRuf
Copy link
Contributor Author

Thanks for the hint, I'll check if CI passes and see if we can find a solution as 10 is the current stable version now.

@kibertoad
Copy link
Collaborator

@DanielRuf
Copy link
Contributor Author

Ok so this can not be merged yet and I need further info from the maintainers so we can address the Node.js 10 issue(s).

@DanielRuf
Copy link
Contributor Author

Looks better now. Can we retrigger Node.js 6? We finally caught real bugs on Node.js 10 (in our unit tests).

@kibertoad
Copy link
Collaborator

@DanielRuf close/open pr to rerun.

@kibertoad
Copy link
Collaborator

@DanielRuf can you explicitly bump oracle driver version in package.json?

@kibertoad
Copy link
Collaborator

@DanielRuf we didn't commit lock file before, are you sure we want to start doing it now?

@DanielRuf
Copy link
Contributor Author

we didn't commit lock file before, are you sure we want to start doing it now?

Check the failing builds, npm ci is used and needs synced lockfiles ;-)

@DanielRuf
Copy link
Contributor Author

@DanielRuf can you explicitly bump oracle driver version in package.json?

Which package do you mean in package.json?

@kibertoad
Copy link
Collaborator

My mistake, I thought we are using it as dev depency, but apparently only travis does.

@kibertoad
Copy link
Collaborator

@DanielRuf Both failures look like accidents, can you try rerunning the tests?

@DanielRuf
Copy link
Contributor Author

@DanielRuf Both failures look like accidents, can you try rerunning the tests?

Rerunning them will not fix them. 10 has actual failing tests (probably due to dependencies) and 6 has an issue finding the needed apt packages.

Completely different.

I'll have to do some testing and research, probably by enabling Travis CI on my fork.

@kibertoad
Copy link
Collaborator

@DanielRuf
Are you sure about it?
https://travis-ci.org/tgriesser/knex/jobs/389603382 shows one test that I've definitely seen failing randomly before.

@DanielRuf
Copy link
Contributor Author

It is one of two jobs which fail. Also a right promise / timeout should solve this.

@DanielRuf
Copy link
Contributor Author

We should keep an eye on it and try to resolve this with promises or rerunning the tst if it fails.

@DanielRuf
Copy link
Contributor Author

randomely failing things are never good

@kibertoad
Copy link
Collaborator

Yeah. I think it makes sense to create a separate issue for this.

@kibertoad
Copy link
Collaborator

@DanielRuf Are you sure about package-lock being necessary, though? It always worked fine before, and all this PR changes is adding one more env.

@DanielRuf
Copy link
Contributor Author

DanielRuf commented Jun 8, 2018

@DanielRuf Are you sure about package-lock being necessary, though?

Yes, see https://travis-ci.org/tgriesser/knex/jobs/389597882#L817 and https://travis-ci.org/tgriesser/knex/builds/389597881

@kibertoad
Copy link
Collaborator

kibertoad commented Jun 8, 2018

I wonder where "out-of-date" package-lock.json comes in from if there isn't supposed to be one. Maybe it has something to do with cache used?.. Additional "npm install" somewhere in travis script might help.
@elhigu Are you ok with adding package-lock for compatibility with newer npm, or should we investigate other options?

Copy link
Member

@elhigu elhigu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced at all of having lockfile committed in repo in a way that it is automatically included in every PR. It will make my work horrible, when every PR is conflicting due to lockfile changes and it adds always thousand lines of spam to every PR to be reviewed.

For now I suppose one can just remove oracle build from node 10 if that is a problem. Package lock has not been necessary earlier, so I don't see why it would suddenly become obligatory for this PR.

@DanielRuf
Copy link
Contributor Author

Package lock has not been necessary earlier, so I don't see why it would suddenly become obligatory for this PR.

Either someone changed it to npm ci on Travis or Travis changed the defaults. I'll change the install step to npm i and revert the change with the lockfile.

@kibertoad
Copy link
Collaborator

@DanielRuf no, it is most likely due to npm version change in newer node. If everything else fails, you could try locking node version at 10.2.1, I think it has old enough npm to just work.

@elhigu
Copy link
Member

elhigu commented Jun 9, 2018

I dont think that locking node version is a viable solution, it will just break with node 11 and above. Overriding travis scripts nay work or maybe there is some flag for npm wich uses old behavior.

@DanielRuf
Copy link
Contributor Author

DanielRuf commented Jun 9, 2018

It is not the default behaviour of Node.js 10 or npm to use npm ci ;-)

And locking the Node.js version is not a solution here.

I will add the install part to the Travis config.

@@ -1,4 +1,5 @@
yarn.lock
package-lock.json
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change is meaningless, so probably makes sense to restore it to how it previously was to avoid noise in history

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not meaningless, please check all changes (I just moved it to the other lockfile ignore rule).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better as the lockfile ignore rules are both at the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is more consistent. To make it clear we can also add some comments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I meant the ordering. And yes, it is more consistent, you are right.

@elhigu
Copy link
Member

elhigu commented Jun 10, 2018

it seems to work now 🎉 thank you!

@elhigu elhigu merged commit 1a6aa56 into knex:master Jun 10, 2018
@DanielRuf DanielRuf deleted the chore/add-nodejs-10 branch June 10, 2018 11:07
@DanielRuf
Copy link
Contributor Author

@DanielRuf
Copy link
Contributor Author

So Travis CI changed the default behavior.

elhigu pushed a commit that referenced this pull request Sep 26, 2018
* 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
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

Successfully merging this pull request may close these issues.

None yet

3 participants