-
Notifications
You must be signed in to change notification settings - Fork 276
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.
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
Use v5 API host in QnAMakerDialog or construct v4 API host #2004
Conversation
Pull Request Test Coverage Report for Build 121114
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline lets us hold this PR, as we have PR lined up which has conflict.
@@ -352,8 +363,22 @@ export class QnAMakerDialog extends WaterfallDialog { | |||
const endpoint = { | |||
knowledgeBaseId: this.knowledgeBaseId, | |||
endpointKey: this.endpointKey, | |||
host: this.hostName | |||
host: this.constructHttpsHostName(this.hostName) | |||
}; | |||
return new QnAMaker(endpoint); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hostname format for QnAMaker V5 also need to be accommodated. Sample hostname for QnAMaker V5 is https://qnamaker-acom.azure.com/qnamaker/v5.0-preview.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vipeketi, per our offline discussions and the internal doc, what if we look for "qnamaker/v5.0"
in this.hostName
?
private getQnAClient(): QnAMaker {
const host = this.hostName.includes('qnamaker/v5.0')
? this.hostName
: this.constructHttpsHostName(this.hostName);
const endpoint = {
knowledgeBaseId: this.knowledgeBaseId,
endpointKey: this.endpointKey,
host
};
return new QnAMaker(endpoint);
}
I'll iterate a couple of times to make the code clearer (e.g. change constructHttpsHostName()
to constructV4ApiHost()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v5 handling looks good to me. When source code is downloaded from portal , the appsettings pre-populate complete url like https://xxx.azurewebsites.com/qnamaker. Can we skip just the hostname as value for this.hostName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't skip the template literal because that's a breaking change, but we can add logic to examine the provided value:
If the hostname looks like this: xxx.azurewebsites.net
, then it's probably in the appsettings of a downloaded or deployed bot through Azure. We can apply the logic from your recent PR to the samples: microsoft/BotBuilder-Samples@b834e98.
If it looks like https://xxx.azurewebsites.net/qnamaker
, then it's probably gathered from the QnAMaker portal, and it doesn't need to be touched up in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can accomodate support for https://xxx.azurewebsites.net/qnamaker and qnamaker/v5.0, I am good with the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
Do not merge without review from @vipeketi
Fixes breaking change in #1999 while still addressing #1885
Description
This PR changes the logic in QnAMakerDialog.getQnAClient() to use the dialog instance's
hostName
if it contains'qnamaker/v5'
otherwise use the pre-#1999 behavior of constructing a v4 API endpoint with a template literal.This PR also adds missing validation to the QnAMakerDialog constructor (C#) and fixes the doc string on the QnAMakerEndpoint interface.
Specific Changes
QnAMakerDialog.getQnAClient()
host
QnAMaker