Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions lib/handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,19 @@ let Handler = function () {

/**
* Initializes the handler, sets up the connection to wcs
* @params credentials which includes apikey, workspace_id, workspace_name, iam_apikey description, iam_apikey, workspace url, version, version_date
*/
Handler.prototype.initialize = function () {
if(WaEnvUtils.isValidWaEnvVars()) {
Handler.prototype.initialize = function (credentials) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

where is this called from? traditionally it was called by the skill on startup, if this has not changed then how does it get the credentials? if it has changed then are you creating a new handler for each request?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@offerakrabi it is still being called from the skill only. The skill is getting credentials on start up from VCAP_SERVICES of the bound conversation service and then makes a WA API call to get the workspace_id based on the skill name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ashokbabuy This is a singleton - meaning this is the ONLY instance for ALL calls, including all NLU engines.
It is wrong to pass credentials if it is not related to ALL engines.
What do you in a case where there is an engine that is not WCS? that requires different credentials IN ADDITION to WCS?
And how is it multi tenant if you pass here only 1 set of credentials?

//check if credentials are coming from the handler initialize call and also valid from the index.js of the skill
if((credentials && WaEnvUtils.isValidWaCredentials(credentials))){
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is a specific check for a specific type of NLU engine here. How can you cover here all future types?

this.wcsCredentials = WaEnvUtils.setupWCSCredentialsfromVCAP(credentials);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this is only way we can pass credentials? from VCAP SERVICES?

} else if(WaEnvUtils.isValidWaEnvVars()) {
this.wcsCredentials = WaEnvUtils.setupWCSCredentialsFromEnv();
} else {
let rawJson = fs.readFileSync(path.join(process.env.skillSDKResDir, nluFolder, wcsFileName));
this.wcsCredentials = JSON.parse(rawJson);
}
if (this.wcsCredentials) {
if(this.wcsCredentials){
let credentials = this.wcsCredentials.credentials;
setupWcs(this, credentials.url, credentials.username, credentials.password, credentials.version_date, credentials.version, credentials.iam_apikey)
}
Expand Down Expand Up @@ -339,7 +343,8 @@ let setupWcs = function (handler, wcsUrl, wcsUsername, wcsPassword, versionDate,
options = {
version: version,
iam_apikey: apiKey,
url: wcsUrl
url: wcsUrl,
export: false
}

} else {
Expand Down
15 changes: 9 additions & 6 deletions lib/nlu/factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,13 @@ class Factory {
});
}


static getNLUs(manifest) {
//pass the credentials sent from the skill to getNLU's
static getNLUs(manifest, credentials) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How one set of credentials can cover ALL types of engines?

return new Promise(function (resolve, reject) {
let promises = [];
let types = manifest.nlu;
types.forEach(function (type) {
promises.push(getNLU(type, manifest.name));
promises.push(getNLU(type, manifest.name, credentials));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if the skill uses also a regexp engine what credentials will be passed? it doesn't use any credentials

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

From the skill, we will not be passing the credentials if the nlu is of the type regexp.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But what if you have regexp AND WCS?

});

Promise.all(promises.map(reflect)).then(function (results) {
Expand All @@ -124,11 +124,14 @@ class Factory {

module.exports = Factory;


function getNLU(type, name) {
//pass the credentials from the skill
function getNLU(type, name, credentials) {
return new Promise(function (resolve, reject) {
let nlu;
if(WaEnvUtils.isValidWaEnvVars() && type === 'wcs') {
if(credentials && WaEnvUtils.isValidWaCredentials(credentials) && type === 'wcs'){
nlu = WaEnvUtils.setupWCSCredentialsfromVCAP(credentials);
}
else if(WaEnvUtils.isValidWaEnvVars() && type === 'wcs') {
nlu = WaEnvUtils.setupWCSCredentialsFromEnv();
} else {
nlu = require(`${resPath}/nlu/${type}`);
Expand Down
52 changes: 51 additions & 1 deletion lib/nlu/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,17 @@ let isValidWaEnvVars = function() {
process.env.WCS_WORKSPACE_LANGUAGE));
};

/**
* check that all the WA credentials sent from the skill are valid
* @returns {string}
*/

let isValidWaCredentials = function(credentials) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You only check here WCS? what about other NLUs?

return ((process.env.WCS_USERNAME && process.env.WCS_URL && process.env.WCS_PASSWORD && process.env.WCS_VERSION_DATE
&& process.env.WCS_VERSION) ||
(credentials.apikey && process.env.WCS_WORKSPACE_LANGUAGE && credentials.workspace_name && credentials.workspace_name && credentials.url));
};


/**
* Sets up the handler's wcs credentials from environment variables
Expand Down Expand Up @@ -55,8 +66,47 @@ let setupWCSCredentialsFromEnv = function () {
return options;
};

/**
* Sets up the handler's wcs credentials from VCAP credentials read from the skill
* @param handler
*/

let setupWCSCredentialsfromVCAP = function (credentials) {

let options = {
"workspace": {
[process.env.WCS_WORKSPACE_LANGUAGE]: {
"name": credentials.workspace_name,
"workspace_id": credentials.workspace_id
}
}
};
// new way of wa authentication - with iam api-key
if(credentials.apikey) {
options.credentials = {
"url": credentials.url,
"version": credentials.version_date,
"iam_apikey": credentials.apikey
}
}
// deprecated way of wa authentication
else if(process.env.WCS_PASSWORD && process.env.WCS_USERNAME) {
options.credentials = {
"url": process.env.WCS_URL,
"version": process.env.WCS_VERSION,
"version_date": process.env.WCS_VERSION_DATE,
"password": process.env.WCS_PASSWORD,
"username": process.env.WCS_USERNAME
}
}
return options;

};


module.exports = {
isValidWaEnvVars: isValidWaEnvVars,
setupWCSCredentialsFromEnv: setupWCSCredentialsFromEnv
isValidWaCredentials: isValidWaCredentials,
setupWCSCredentialsFromEnv: setupWCSCredentialsFromEnv,
setupWCSCredentialsfromVCAP: setupWCSCredentialsfromVCAP
};
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "skill-sdk-nodejs",
"version": "0.0.18",
"version": "0.0.19",
"description": "This is the SDK to be used when developing a skill for Watson Assistant Solutions using Node.js",
"main": "index.js",
"scripts": {
Expand Down