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

impr(oracledb): 4 fix #3480

Merged
merged 2 commits into from Oct 15, 2019
Merged

impr(oracledb): 4 fix #3480

merged 2 commits into from Oct 15, 2019

Conversation

maximelkin
Copy link
Collaborator

@maximelkin maximelkin commented Oct 13, 2019

Idk how to test different oracledb packages
also maybe need to add some tests?

@maximelkin maximelkin changed the title [WIP] impr(oracledb): 4 fix [WIP]: impr(oracledb): 4 fix Oct 13, 2019
@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Oct 13, 2019

Travis needs to be updated to use latest driver, that would work as a test.

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Oct 14, 2019

Fixes #3396

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Oct 14, 2019

@maximelkin You can remove version from

- (echo $DB | grep oracledb) && npm install oracledb@3.1.2 || true
and that should suffice.

@@ -16,7 +16,6 @@ matrix:
env: TESTSCRIPT=test:everything DB="oracledb mssql mysql mysql2 postgres sqlite3" KNEX_TEST_TIMEOUT=60000
install:
- npm i
- (echo $DB | grep oracledb) && npm install oracledb@3.1.2 || true
Copy link
Collaborator

@kibertoad kibertoad Oct 14, 2019

Choose a reason for hiding this comment

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

wait, whole line shouldn't be removed :D. just the @3.1.2 part, so that latest driver version is picked up.

@maximelkin maximelkin closed this Oct 14, 2019
@maximelkin maximelkin reopened this Oct 14, 2019
@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Oct 14, 2019

@maximelkin Is anything left on this PR? (you kept it asWIP)

@maximelkin
Copy link
Collaborator Author

@maximelkin maximelkin commented Oct 14, 2019

I just want check existence of tests for lobs

@maximelkin maximelkin changed the title [WIP]: impr(oracledb): 4 fix impr(oracledb): 4 fix Oct 14, 2019
@kibertoad kibertoad merged commit c9e3057 into knex:master Oct 15, 2019
1 check passed
@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Oct 15, 2019

Thanks!

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Oct 15, 2019

@maximelkin Thank you for your most impressive contributions lately. Would you be interested in becoming one of the project maintainers?

@maximelkin
Copy link
Collaborator Author

@maximelkin maximelkin commented Oct 15, 2019

@kibertoad

Would you be interested in becoming one of the project maintainers?

Sorry, not interested

@MaliaGuerrero
Copy link
Contributor

@MaliaGuerrero MaliaGuerrero commented Nov 16, 2019

Hi - there's a bug with this. If you are reading from a BLOB, this function fails:

function readStream(stream, type)

You need to set data to a Buffer:

    let data = '';
    if (type === 'buffer') {
      data = Buffer.alloc(0);
    }

I'm trying to do a PR, but I can't push....so struggling with that.

@maximelkin
Copy link
Collaborator Author

@maximelkin maximelkin commented Nov 16, 2019

@MaliaGuerrero fork repo, push to it, then create PR from your fork to this repo

@MaliaGuerrero
Copy link
Contributor

@MaliaGuerrero MaliaGuerrero commented Nov 16, 2019

Yeah, I'm getting a 403 error. I'm attempting to work through it.

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 16, 2019

@MaliaGuerrero I think you are just trying to clone it, as I don't see you having created personal fork. Check this: https://help.github.com/en/github/getting-started-with-github/fork-a-repo

@MaliaGuerrero
Copy link
Contributor

@MaliaGuerrero MaliaGuerrero commented Nov 19, 2019

Thanks! That fixed it. I made a PR.

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