Skip to content

[Cognitive Services] Update endpoint URL template for TextAnalytics. #3491

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

Merged
annatisch merged 1 commit into from Jul 26, 2018
Merged

[Cognitive Services] Update endpoint URL template for TextAnalytics. #3491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion specification/cognitiveservices/data-plane/TextAnalytics/readme.nodejs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ nodejs:
package-name: azure-cognitiveservices-textanalytics
package-version: 1.0.0
output-folder: $(node-sdks-folder)/lib/services/textAnalytics
override-client-name: TextAnalyticsAPIClient
azure-arm: false
generate-license-txt: true
generate-package-json: true
Expand Down
25 changes: 20 additions & 5 deletions specification/cognitiveservices/data-plane/TextAnalytics/stable/v2.0/TextAnalytics.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,15 @@
"apim_key": []
}
],
"host": "api.cognitive.microsoft.com",
"basePath": "/text/analytics/v2.0",
"schemes": [
"https"
],
"x-ms-parameterized-host": {
"hostTemplate": "{Endpoint}/text/analytics/v2.0",
"useSchemePrefix": false,
"parameters": [
{
"$ref": "#/parameters/Endpoint"
}
]
},
"paths": {
"/keyPhrases": {
"post": {
Expand Down Expand Up @@ -572,5 +576,16 @@
}
}
}
},
"parameters": {
"Endpoint": {
"name": "Endpoint",
"description": "Supported Cognitive Services endpoints (protocol and hostname, for example: https://westus.api.cognitive.microsoft.com).",
Copy link
Contributor

@assafi assafi Jul 24, 2018

Choose a reason for hiding this comment

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

Please note that the Azure Portal resource "Endpoint" includes the /text/analytics/v2.0. We should make sure the documentation is very clear here, or change it to include the suffix.

Copy link
Contributor

@assafi assafi Jul 24, 2018

Choose a reason for hiding this comment

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

Also, please link to the documentation PR as well.

Copy link
Member Author

@yangyuan yangyuan Jul 25, 2018

Choose a reason for hiding this comment

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

Hi @assafi, we will update Azure Portal accordingly when we publish SDK.
As in description, I emphasized protocol and hostname and attached an example. Any suggestion how to make it more clear?

What which document you refer to?

Copy link
Contributor

@assafi assafi Jul 25, 2018

Choose a reason for hiding this comment

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

What do you mean by updating the Azure portal?

I'm referring to the TA documentation from your previous PR, which says:

Replace the location in client.BaseUri to the endpoint you signed up for. You can find the endpoint on Azure Portal resource. The endpoint typically looks like "https://[region].api.cognitive.microsoft.com/text/analytics/v2.0".

If you will be using a similar language to describe this you should note that the endpoint from the Azure portal contains a suffix which you exclude in this PR.

Ideally, we should update the SDK and its documentation at the same time, to avoid recent issues like lagging documentation. Which is why I asked the description of this PR to contain a link to the documentation PR.

Copy link
Member Author

@yangyuan yangyuan Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I will change Azure Portal resource page to remove the suffix to keep consistent. The document will also say:

You can find the endpoint on Azure Portal resource. The endpoint typically looks like "https://[region].api.cognitive.microsoft.com/"

Does it make sense?

About document PR, I agree SDK (actually SDK package) and its documentation should be updated at the same time. But this is swagger PR?
After merge swagger PR, I will send C# SDK PR. I prefer send the documentation PR along with C# SDK PR, because at that time I can test the code in the documents. Also the document won't be accidentally published before NuGet (sometimes you say "please hold", but...)

We don't need to worry about the lagging documentation. It is hard to control when documents get updated, but we can control when to publish NuGet.

But I can send a document PR now if it is your concern.

Copy link
Contributor

@assafi assafi Jul 25, 2018

Choose a reason for hiding this comment

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

Regarding the portal update - I don't think we need to change it. Not all users use the SDK, some simply write their own code in whatever language they want and they will need the full URL. It's important not to break existing or future customers just because we changed how a parameter looks in the SDK (because of some other swagger limitation). The solution for this should be better documentation and/or smarter SDK.

Regarding the documentation PR - sure, please attach it to the SDK PR.

Copy link
Member Author

@yangyuan yangyuan Jul 26, 2018

Choose a reason for hiding this comment

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

Moved to offline...

Copy link
Contributor

@assafi assafi Jul 26, 2018

Choose a reason for hiding this comment

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

We've agreed to go with baseEndpoint instead of Endpoint at a later update.

"x-ms-parameter-location": "client",
"required": true,
"type": "string",
"in": "path",
"x-ms-skip-url-encoding": true
}
}
}
2 changes: 1 addition & 1 deletion ...tiveservices/data-plane/TextAnalytics/stable/v2.0/examples/SuccessfulEntitiesRequest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"parameters": {
"Ocp-Apim-Subscription-Key": "{API key}",
"AzureRegion": "westus",
"Endpoint": "{Endpoint}",
"input": {
"documents": [
{
Expand Down
2 changes: 1 addition & 1 deletion ...veservices/data-plane/TextAnalytics/stable/v2.0/examples/SuccessfulKeyPhrasesRequest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"parameters": {
"Ocp-Apim-Subscription-Key": "{API key}",
"AzureRegion": "westus",
"Endpoint": "{Endpoint}",
"input": {
"documents": [
{
Expand Down
2 changes: 1 addition & 1 deletion ...iveservices/data-plane/TextAnalytics/stable/v2.0/examples/SuccessfulLanguagesRequest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"parameters": {
"Ocp-Apim-Subscription-Key": "{API key}",
"AzureRegion": "westus",
"Endpoint": "{Endpoint}",
"input": {
"documents": [
{
Expand Down
2 changes: 1 addition & 1 deletion ...iveservices/data-plane/TextAnalytics/stable/v2.0/examples/SuccessfulSentimentRequest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"parameters": {
"Ocp-Apim-Subscription-Key": "{API key}",
"AzureRegion": "westus",
"Endpoint": "{Endpoint}",
"input": {
"documents": [
{
Expand Down