Mt sdk #69

ashokbabuy · 2018-12-20T09:06:54Z

made changes to make it multi tenant with backward compatibility

offerakrabi · 2018-12-23T08:14:28Z

lib/handler.js

 */
-Handler.prototype.initialize = function () {
-    if(WaEnvUtils.isValidWaEnvVars()) {
+Handler.prototype.initialize = function (credentials) {


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?

@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.

@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?

offerakrabi · 2018-12-23T08:18:27Z

lib/nlu/factory.js

            let types = manifest.nlu;
            types.forEach(function (type) {
-                promises.push(getNLU(type, manifest.name));
+                promises.push(getNLU(type, manifest.name, credentials));


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

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

But what if you have regexp AND WCS?

deanhaber

I think there is a misunderstanding about what is the handler's job and what is the relations between all the classes in the SDK and the inheritance between them.
This code changes are VERY tightly coupled with WCS and will eliminate any possibility of using other NLU engines (including custom ones)

Furthermore - where are the unit tests?

deanhaber · 2018-12-24T08:15:12Z

lib/handler.js

 */
-Handler.prototype.initialize = function () {
-    if(WaEnvUtils.isValidWaEnvVars()) {
+Handler.prototype.initialize = function (credentials) {


@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?

deanhaber · 2018-12-24T08:16:01Z

lib/handler.js

-    if(WaEnvUtils.isValidWaEnvVars()) {
+Handler.prototype.initialize = function (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))){


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

deanhaber · 2018-12-24T08:16:24Z

lib/handler.js

+Handler.prototype.initialize = function (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))){
+        this.wcsCredentials = WaEnvUtils.setupWCSCredentialsfromVCAP(credentials);


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

deanhaber · 2018-12-24T08:17:08Z

lib/nlu/factory.js

            let types = manifest.nlu;
            types.forEach(function (type) {
-                promises.push(getNLU(type, manifest.name));
+                promises.push(getNLU(type, manifest.name, credentials));


But what if you have regexp AND WCS?

deanhaber · 2018-12-24T08:17:46Z

lib/nlu/factory.js

-
-    static getNLUs(manifest) {
+//pass the credentials sent from the skill to getNLU's
+    static getNLUs(manifest, credentials) {


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

deanhaber · 2018-12-24T08:18:14Z

lib/nlu/utils.js

+ * @returns {string}
+ */
+
+let isValidWaCredentials = function(credentials) {


You only check here WCS? what about other NLUs?

Ashok Yalamanchili added 6 commits December 18, 2018 12:41

MT first cut

f2bca68

updated factory and package

b97cd26

utils updated to take credentials

9b8d4a2

added comments and added additional checks

c9addc1

added tenantID as an optional parameter in swagger converse and evaluate

5982b9b

reverted the tenantID inclusion in swagger for backward compatibility

37740dc

ashokbabuy requested review from deanhaber and offerakrabi December 20, 2018 09:06

Ashok Yalamanchili added 4 commits December 20, 2018 15:09

added additional checks

aa0fb5c

typo fix

da60b6a

typo fix

d51c7ed

fix for WA rate limit to load workspace

2a5ea04

offerakrabi reviewed Dec 23, 2018

View reviewed changes

deanhaber suggested changes Dec 24, 2018

View reviewed changes

fix for comment related to nlu type check

7731bef

Mt sdk #69

Are you sure you want to change the base?

Mt sdk #69

Uh oh!

Conversation

ashokbabuy commented Dec 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deanhaber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants