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.

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

build: add oracle to CI #6623

Merged
merged 3 commits into from
Sep 4, 2020
Merged

build: add oracle to CI #6623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
16aa88f
build: add oracle to build database services
imnotjames Aug 22, 2020
fd26897
only enable test 1972 for MySQL
imnotjames Aug 27, 2020
28ca3c9
disable github issue 3118 for oracle
imnotjames Aug 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,24 @@ commands:
if [ ! -d node_modules ]; then
npm install
fi
- run:
# This is pretty terrible but OracleDB requires you to grab the binaries OOB
# from the normal installation, place them in the LD Path
# also - not super well documented - grab `libaio` as well
# Because this is technically the same image as the runner we'll snag
# the libaio1 and place them in the same instantclient directory.
name: Download Required OracleDB Binaries
command: |
if [ ! -d node_modules/oracledb/instantclient_19_8 ]; then
curl -sf -o node_modules/oracledb/instantclient.zip $BLOB_URL
unzip -qqo node_modules/oracledb/instantclient.zip -d node_modules/oracledb/
rm node_modules/oracledb/instantclient.zip

DEBIAN_FRONTEND=noninteractive sudo apt-get -qq -y install libaio1
cp /lib/*/libaio.so.* node_modules/oracledb/instantclient_19_8/
fi
environment:
BLOB_URL: https://download.oracle.com/otn_software/linux/instantclient/19800/instantclient-basiclite-linux.x64-19.8.0.0.0dbru.zip
- save_cache:
name: Save node_modules cache
key: node_modules-{{ checksum "package-lock.json" }}
Expand Down Expand Up @@ -188,3 +206,10 @@ workflows:
- build
databases: "cockroachdb"
node-version: "12"
- test:
name: test (oracle) - Node v12
requires:
- lint
- build
databases: "oracle"
node-version: "12"
6 changes: 6 additions & 0 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ services:
ports:
- "26257:26257"

oracle:
image: imnotjames/oracle-xe:18
container_name: "typeorm-oracle"
ports:
- "1521:1521"

# sap hana (works only on linux)
# hanaexpress:
# image: "store/saplabs/hanaexpress:2.00.040.00.20190729.1"
Expand Down
7 changes: 5 additions & 2 deletions ormconfig.circleci-common.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,12 @@
"type": "oracle",
"host": "typeorm-oracle",
"port": 1521,
"sid": "xe",
"sid": "XE",
"username": "system",
"password": "oracle",
"logging": false
"logging": false,
"extra": {
"connectString": "typeorm-oracle:1521/XE"
}
}
]
14 changes: 10 additions & 4 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
"mssql": "^4.3.2",
"mysql": "^2.15.0",
"mysql2": "^1.6.5",
"oracledb": "^5.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

The reason why we didn't have this before in package.json is because it will cause many problems to users who want to pull typeorm source, simply run npm i without any headache and continue tweaking things.

Is it still problematic to install this package? If yes, I recommend to remove it, and manually add npm i oracledb command to CI config

Copy link
Contributor Author

@imnotjames imnotjames Sep 3, 2020 •
edited

Choose a reason for hiding this comment

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

I did not have any issues installing this - at least, no more than sqlite3 with node-gyp. I didn't have to install anything extra.

But - that's just me. I tried on Linux and a Mac running high sierra (with homebrew). Is there any way we can test that further?

Actually USING the Oracle driver with this package is a separate issue.. as long as you're not enabling it and just using it for the static analysis and the like it seems to work without extra effort.

Copy link
Member

Choose a reason for hiding this comment

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

I'm talking about these steps:

https://oracle.github.io/node-oracledb/INSTALL.html#-2-quick-start-node-oracledb-installation

From what I remember it was required to do all these steps otherwise npm i on typeorm root would fail. That's why I'm talking, if it still necessary to do these steps for users, it's better to remove oracledb from dependencies.

Copy link
Contributor Author

@imnotjames imnotjames Sep 4, 2020 •
edited

Choose a reason for hiding this comment

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

The steps required for the install to work only apply if there's no prebuilt binary.

This does not include fetching the client libraries and setting up the ld config path which is only required if you are actually exercising the OracleDB module - eg, trying to connect to OracleDB.

Instead, the steps include installing dependencies for building a nodejs binary extension using gyp - but ONLY if your architecture and node version does not have a prebuilt binary. The same requirement applies for sqlite3 as it's also a binary module.

If this is a concern we can move this to a peer or optional dependency? But then the same should be done for sqlite3 & better-sqlite.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't have any problems installing sqlite3 & better-sqlite.... Don't remember any at least. But oracle definitely is a problem to install.

Anyway, my point is - any user should be able to just clone the repo, run npm i and just use it. With oracle its complicated (and we had users complaining about it few years ago). And since it looks like nothing has changed, and it's still a problem. I didn't heard anything about sqlite from users regarding to its issues.

So I suggest to remove it, and make oracle tests to run only on CI, so it won't bring problems to users on their local machines.

Copy link
Contributor Author

@imnotjames imnotjames Sep 4, 2020 •
edited

Choose a reason for hiding this comment

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

Sure, I can do that. I'll get that later today. And as it stands, oracle tests wouldn't run locally unless you enabled them.

Also, the issues you're probably referring to are that the client libraries were needed to import the module before v3.1.0. However, that's been changed to not be the case.

The v4 release brought a refactor to use a different node API so the builds are portable allowing for a binary distributable to be used instead of having to always compile from source - using node pre gyp just like sqlite3.

So folks that are developing on typeorm would have the module installed but during runtime if they try to open a connection to OracleDB it throws an error.

Copy link
Member

Choose a reason for hiding this comment

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

okay, if they updated the installations steps, then we can left it in deps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to move forward in whatever direction you think is best, here. It's fine either way by me. The one thing I'd want to make sure is that there's at least a hint of somewhere with the expected version of oracledb so we can easily pull it in with CI without hard-coding the version in the CI config.

"pg": "^8.3.0",
"redis": "^2.8.0",
"remap-istanbul": "^0.13.0",
Expand Down
1 change: 1 addition & 0 deletions test/github-issues/1972/issue-1972.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ describe("github issues > #1972 STI problem - empty columns", () => {

before(async () => connections = await createTestingConnections({
entities: [__dirname + "/entity/*{.js,.ts}"],
enabledDrivers: ['mysql']
}));

beforeEach(() => reloadTestingDatabases(connections));
Expand Down
1 change: 1 addition & 0 deletions test/github-issues/3118/issue-3118.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ describe("github issues > #3118 shorten alias names (for RDBMS with a limit) whe
let connections: Connection[];
before(async () => connections = await createTestingConnections({
entities: [__dirname + "/entity/*{.js,.ts}"],
enabledDrivers: ["mysql", "postgres", "cockroachdb", "sap", "mariadb", "mssql"]
}));
beforeEach(() => reloadTestingDatabases(connections));
after(() => closeTestingConnections(connections));
Expand Down