Skip to content

feature: Update oracledb.js for additional binary path #1602

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.

Sign up for GitHub

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

Jump to bottom
Closed
lemankk wants to merge 3 commits into from
Closed

feature: Update oracledb.js for additional binary path #1602

lemankk wants to merge 3 commits into from

Conversation

@lemankk
Copy link

@lemankk lemankk commented Aug 31, 2023 •
edited
Loading

Hello, this PR is used for create additional parameter binaryPath when using initOracleClient(). It's open the way to load oracledb.node from different path of inside the default node_modules folder.

Usage:

initOracleClient({ binaryPath: '/path/to/folder-of-storing-oracledb.node' })

@oracle-contributor-agreement
Copy link

oracle-contributor-agreement bot commented Aug 31, 2023

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. label Aug 31, 2023
@cjbj
Copy link
Contributor

cjbj commented Aug 31, 2023

@lemankk please sign the OCA before submitting PRs. Thanks!

anthony-tuininga
anthony-tuininga reviewed Aug 31, 2023
View reviewed changes
lib/oracledb.js Outdated
'./node_modules/oracledb/' + nodbUtil.RELEASE_DIR + '/' + nodbUtil.BINARY_FILE,
'./node_modules/oracledb/' + nodbUtil.RELEASE_DIR + '/' + 'oracledb.node'
];
if (arg1 !== undefined && arg1.binaryPath !== undefined) {
Copy link
Member

@anthony-tuininga anthony-tuininga Aug 31, 2023

Choose a reason for hiding this comment

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

Here you can simply check if (options.binaryPath !== undefined) if you pass options through to this function.

Copy link
Author

@lemankk lemankk Aug 31, 2023

Choose a reason for hiding this comment

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

@anthony-tuininga Thanks for your advice. Changes has been updated for using options directly.

Copy link
Contributor

@cjbj cjbj Aug 31, 2023

Choose a reason for hiding this comment

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

There's no point making changes unless you sign the OCA ! (Sorry!)

Copy link
Author

@lemankk lemankk Aug 31, 2023

Choose a reason for hiding this comment

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

Sorry I just noticed that. Already submitted and under review. Do I need to close this PR until review completed?

Copy link
Contributor

@cjbj cjbj Aug 31, 2023

Choose a reason for hiding this comment

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

It fine to leave this open - we have the labels to indicate the overall OCA status

Copy link
Contributor

@cjbj cjbj Aug 31, 2023

Choose a reason for hiding this comment

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

Looks like it was approved so now we just need the technical part reviewed.

As a procedural note, we would copy this PR to our private repo, and then sync the changes in that private repo back here. I.e this PR would be closed without any apparent merge but the changes will eventually land.

anthony-tuininga
anthony-tuininga reviewed Aug 31, 2023
View reviewed changes
@oracle-contributor-agreement
Copy link

oracle-contributor-agreement bot commented Aug 31, 2023

Thank you for signing the OCA.

@oracle-contributor-agreement oracle-contributor-agreement bot added OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. labels Aug 31, 2023
@sharadraju
Copy link
Member

sharadraju commented Sep 8, 2023 •
edited
Loading

We are working on adding this feature internally with more comprehensive testing and will update you soon.

sharadraju added a commit that referenced this pull request Sep 15, 2023
@sharadraju
Add new Property for setting the binary location for the thick mode -…
0d72546 
… PR #1602
@sharadraju
Copy link
Member

sharadraju commented Sep 15, 2023 •
edited
Loading

@lemankk This PR is now available as a patch in this GitHub repo. Please take the latest version of the file /lib/oracledb.js
We have added a new property binaryDir for the initOracleClient() method to add the additional binary path.

It will added officially in the node-oracledb 6.2 release. Thank you for your contribution!

I will close this PR once 6.2 is out.

@sharadraju
Copy link
Member

sharadraju commented Oct 11, 2023

Closing this PR as this enhancement has been added in node-oracledb 6.2.
Thank you for your contribution!

@sharadraju sharadraju closed this Oct 11, 2023
@romanbalayan romanbalayan mentioned this pull request Jan 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anthony-tuininga anthony-tuininga anthony-tuininga left review comments

+1 more reviewer

@cjbj cjbj cjbj left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

OCA Verified All contributors have signed the Oracle Contributor Agreement. patch available

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lemankk @cjbj @sharadraju @anthony-tuininga