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

[WIP] fix: replace missing Oralce XE image #187

Closed
wants to merge 10 commits into from
Closed

[WIP] fix: replace missing Oralce XE image #187

wants to merge 10 commits into from

Conversation

hacksparrow
Copy link
Contributor

Replacement for Oracle XE image which was taken down.

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@hacksparrow hacksparrow force-pushed the fix/test-build branch 2 times, most recently from 1a7382d to e179de3 Compare September 3, 2019 12:19
fix: replace missing Oralce XE image
98e17ce 
Replacement for Oracle XE image which was taken down
@dhmlau
Copy link
Member

dhmlau commented Sep 6, 2019

@hacksparrow, the error seems to be related to null password:

09:46:32      Uncaught Error: ORA-01005: null password given; logon denied
09:46:32   
09:46:32 
09:46:32   2) "before all" hook in "{root}":
09:46:32      Error: ORA-01005: null password given; logon denied```

Perhaps the image wasn't setup properly?

@hacksparrow
Copy link
Contributor Author

Did you run source setup.sh before npm test?

@hacksparrow hacksparrow force-pushed the fix/test-build branch 3 times, most recently from 7d2d395 to 19d83fb Compare September 11, 2019 15:50
dhmlau
dhmlau reviewed Sep 11, 2019
View reviewed changes
package.json Outdated
@@ -16,7 +16,8 @@
"scripts": {
"lint": "eslint .",
"lint:fix": "eslint . --fix",
"test": "mocha --UV_THREADPOOL_SIZE=100 test/*.test.js node_modules/juggler-v3/test.js node_modules/juggler-v4/test.js",
"pretest": "source ./setup.sh",
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 not sure if we need to have the setup.sh as the pretest, because it will imply that whoever running the tests need to use the oracle instance in docker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do we setup Oracle for CI?

Copy link
Member

Choose a reason for hiding this comment

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

@rmg, do you know?

Copy link
Contributor

Choose a reason for hiding this comment

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

CI is using a static test Oracle instance and injecting the credentials for the tests to pick them up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, using source here isn't portable. You should either run it directly (./setup.sh) or via bash (bash ./setup.sh).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rmg apart from an Oracle instance, we have to populate some data in it. I got setup.sh to do it's work (pulling the desired Oracle image, running it, and populating data), but ORACLE_USER and ORACLE_PASSWORD are not available to the app, despite export ORACLE_USER=SYSTEM ORACLE_PASSWORD=oracle && mocha --UV_THREADPOOL_SIZE=100 test/*.test.js node_modules/juggler-v3/test.js node_modules/juggler-v4/test.js.

On the "Environment variables" page of the project on Jenkins, there's no entry for ORACLE_USER and ORACLE_PASSWORD, while it is there for Cloudant, DashDB, Redis etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the static instance already has the data populated, we will not need to run setup.sh, just need ORACLE_USER and ORACLE_PASSWORD set in the "Environment variables" for the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

The credentials are already provided via rc. The tests all have a line like this in them:

const config = require('rc')('loopback', {test: {oracle: {}}}).test.oracle;

That is pulling values from ~/.loopbackrc, which is being injected in the test environment specifically to provide those credentials. The instance should already contain the test data as far as I'm aware. The purpose of setup.sh was to create a local DB that matches that environment.

@dhmlau
Copy link
Member

dhmlau commented Sep 13, 2019

@rmg, I believe @hacksparrow mentioned we need the following two env variables set in order for the CI to pass: ORACLE_USER and ORACLE_PASSWORD. Do you think you could help? Thanks.

rmg
rmg reviewed Sep 17, 2019
View reviewed changes
test/init/init.js Outdated
@@ -10,6 +10,8 @@ let DataSource = juggler.DataSource;

const config = require('rc')('loopback', {test: {oracle: {}}}).test.oracle;
config.maxConn = 64;
config.user = process.env.ORACLE_USER;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is throwing away the credentials provided by CI.

rmg
rmg reviewed Sep 17, 2019
View reviewed changes
test/oracle.connectionpool.test.js Outdated
@@ -16,6 +16,8 @@ let db, config;

before(function() {
config = require('rc')('loopback', {dev: {oracle: {}}}).dev.oracle;
config.user = process.env.ORACLE_USER;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is throwing away the credentials provided by CI.

rmg
rmg reviewed Sep 17, 2019
View reviewed changes
test/oracle.discover.test.js Outdated
@@ -17,6 +17,8 @@ let db;
describe('discoverModels', function() {
before(function() {
const config = require('rc')('loopback', {dev: {oracle: {}}}).dev.oracle;
config.user = process.env.ORACLE_USER;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is throwing away the credentials provided by CI.

@hacksparrow
Copy link
Contributor Author

Closing in favor of #189.

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

@dhmlau dhmlau dhmlau left review comments

@rmg rmg rmg left review comments

@b-admike b-admike Awaiting requested review from b-admike

@jannyHou jannyHou Awaiting requested review from jannyHou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hacksparrow @dhmlau @rmg