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

Add redshift support without changing cli or package.json #2233

Merged
merged 49 commits into from Feb 3, 2018

Conversation

Projects
None yet
5 participants
@acofer
Contributor

acofer commented Sep 21, 2017

Hi. We (at Skuid) are trying to get mssql support by updating to latest knex while keeping the redshift support added in https://github.com/trevorsmith/knex. I noticed that the original pull request from that repo included (probably unwanted) changes to cli.js and package.json. Those are reverted in this PR - is it possible that it could go in now?

The integration tests are not working for me because I don't have local instances of mariadb, oracle, mssql, etc. to test against, and I haven't added docker/dockerode configuration for them to run against containers. If that is the only roadblock to accepting, I can do it?

@elhigu

I'm not going to support any new dialect additions unless there is also travis CI configuration running tests for that platform.

If CI works and also the integration tests are ran for redshift in Travis and dialect is properly tested and documented it can be merged in.

And lockfile shouldn't have been committed.

@@ -78,7 +78,7 @@
"pre_test": "npm run lint && npm run docker_test",
"tape": "node test/tape/index.js | tap-spec",
"debug_tape": "node-debug test/tape/index.js",
"test": "npm run pre_test && istanbul --config=test/.istanbul.yml cover node_modules/mocha/bin/_mocha -- --check-leaks -t 10000 -b -R spec test/index.js && npm run tape"
"test": "node_modules/mocha/bin/mocha --inspect --check-leaks -t 10000 -b -R spec test/index.js"

This comment has been minimized.

@elhigu

elhigu Sep 22, 2017

Collaborator

Why have you removed coverage and tape tests and pre_test script call?

This comment has been minimized.

@acofer

acofer Sep 22, 2017

Contributor

This is just a progress commit. I should have cut a feature branch. I'll ping you when it's ready.

@@ -1,7 +1,7 @@
#!/usr/bin/env bash
if [ -n "$(which docker)" ]; then
DOCKER_IMAGES=("mysql:5.7" "postgres:9.6")
DOCKER_IMAGES=("mysql:5.7" "postgres:9.6" "quay.io/skuid/docker-aws-redshift")

This comment has been minimized.

@elhigu

elhigu Sep 22, 2017

Collaborator

these docker images are for connection breaking etc tests, not used for general travis CI integration tests.

@@ -97,9 +97,15 @@ export default class Migrator {
forceFreeMigrationsLock(config) {
this.config = this.setConfig(config);
console.log("LOOKING FOR LOCKTABLENAME");

This comment has been minimized.

@elhigu

elhigu Sep 22, 2017

Collaborator

leftover debug prints should be removed

@elhigu

Database setup for CI should be done in travis configuration and most of the other changes are just breaking/disabling the existing test.

@acofer

This comment has been minimized.

Contributor

acofer commented Oct 11, 2017

Tests pass woooo! Sorry it took a few days, we had some fires to put out.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Oct 11, 2017

Awesome! I'll give this still one final review and merge it in if everything seems to be in order.

@elhigu

elhigu approved these changes Oct 12, 2017

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Oct 12, 2017

I think that only thing that is still missin is link to documentation PR, which adds info about this dialect to docs repo and to readme.

@acofer

This comment has been minimized.

Contributor

acofer commented Oct 12, 2017

Cool! I'll write and file a docs PR tomorrow or early next week.

@acofer

This comment has been minimized.

Contributor

acofer commented Oct 13, 2017

dialect: 'redshift',
connection: testConfig.redshift || {
adapter: 'postgresql',
database: 'dev',

This comment has been minimized.

@elhigu

elhigu Oct 29, 2017

Collaborator

knex_test has been used as a test DB name in most of the tests, I think this should have it too with the same name

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Oct 29, 2017

This needs rebase before it can be merged in and would be good to change the test db name to knex_test.

@acofer

This comment has been minimized.

Contributor

acofer commented Nov 27, 2017

Ping?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 29, 2017

Well deserved ping. Sorry I've been really busy and I still am and might be until the 10th of January. I hope I manage to give this final review before that though.

connection: knex.client.connectionSettings,
acquireConnectionTimeout: 300
})
const knexConfig = _.clone(knex.client.config);

This comment has been minimized.

@elhigu

elhigu Feb 1, 2018

Collaborator

Since this was shallow clone you should be overriding the whole knexConfig.pool = { min: 0, max :1 } or otherwise you will modify knex.client.config.pool (https://runkit.com/embed/8zngcllrbqta)

@@ -11,9 +11,11 @@ module.exports = function(knex) {
describe('dropTable', function() {
it('has a dropTableIfExists method', function() {
this.timeout(30000);

This comment has been minimized.

@elhigu

elhigu Feb 1, 2018

Collaborator

(process.env.KNEX_TEST_TIMEOUT || 5000) + 30000 or Math.max (KNEX_TEST_TIMEOUT, 30000) would be better, because mocha timeout is actually set to 60 seconds in CI

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 1, 2018

I'm sorry for taking forever with this. Now I just found two small things to change and I'm ready to merge.

@elhigu elhigu merged commit 5f81e8a into tgriesser:master Feb 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 3, 2018

Thank you very much

@spiderbites

This comment has been minimized.

spiderbites commented Feb 5, 2018

Very excited to see this land in knex, is there any timeline for a new release that includes the new dialect?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 6, 2018

This week

@elhigu elhigu referenced this pull request Mar 27, 2018

Closed

Redshift client #1255

@mscheffer

This comment has been minimized.

mscheffer commented May 22, 2018

FYI, a timestamp with a default value will trigger an error when inserting data into it (im talking about redshift)

is there a dedicated topic where we can report details like this one ?

to recap, you need a plain timestamp, don't set any flags about null values, default values etc, i think this applies to other field types as well.

GJ making knex compatible with redshift

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 22, 2018

Just by reportin new issue with complete, but minimal code example how to reproduce the problem. Please check contributing.md and the issue template.

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