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.

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

[Text Analytics] Auto-generated README.md #14655

Closed

Conversation

deyaaeldeen
Copy link
Member

@deyaaeldeen deyaaeldeen commented Apr 1, 2021

By this work: Azure/autorest.typescript#895. It does not have an Examples section which sets off the CI readme verification alarm.

Related issue: https://github.com/Azure/azure-sdk-for-js/issues/14449
Swagger

sdk/textanalytics/ai-text-analytics/README.md Outdated Show resolved Hide resolved
sdk/textanalytics/ai-text-analytics/README.md Show resolved Hide resolved
#### Using an API Key

Use the [Azure Portal][azure_portal] to browse to your Text Analytics resource and retrieve an API key, or use the [Azure CLI][azure_cli] snippet below:
To create a client object to access the Text Analytics API, you will need the `endpoint` of your Text Analytics resource and a `credential`. The Azure Text Analytics Client can use Azure Active Directory credentials to authenticate.
Copy link
Contributor

Choose a reason for hiding this comment

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

How does the code generator know to talk about "endpoints" vs AAD vs "connection string". Same question for the "Client API key authentication is used in most of the examples," that comes a few lines later

Copy link
Member Author

Choose a reason for hiding this comment

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

I think doing this, even if it is not true for all services, is better than nothing. I can remove it though.


```bash
npm install @azure/identity
```

You will also need to [register a new AAD application][register_aad_app] and grant access to Text Analytics by assigning the `"Cognitive Services User"` role to your service principal (note: other roles such as `"Owner"` will not grant the necessary permissions, only `"Cognitive Services User"` will suffice to run the examples and the sample code).
You will also need to register a new AAD application and grant access to Text Analytics by assigning the suitable role to your service principal (note: roles such as `"Owner"` will not grant the necessary permissions).
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do this if we were relying on the credentials that used Azure CLI or VS Code?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can remove this as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sadasant can you asnwer Ramya's question?

Copy link
Contributor

Choose a reason for hiding this comment

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

Users don’t need to grant specific permission to the Azure CLI credential to use it to authenticate in the cases that I have tried. I have never tried Text Analytics though. I would make sure to corroborate first (by trying, let’s say, InteractiveBrowserCredential on NodeJS with no parameters)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm taking a second look, one moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, DefaultAzureCredential tries: EnvironmentCredential, then ManagedIdentityCredential, then AzureCliCredential, then VisualStudioCodeCredential. From these:

  • EnvironmentCredential doesn't have default values.
  • ManagedIdentityCredential only has a default behavior if shipped on an environment with a system assigned credential (not local dev).
  • AzureCliCredential assumes users authenticated using the Azure CLI.
  • VisualStudioCodeCredential assumes users are trying this inside VSCode, with a specific extension installed and configured well.

This means that, the main reasonable scenario that doesn't need an AAD application is the Azure CLI case. For that case we would need to add a section asking users to authenticate using the CLI above this section.

The "no configuration" credentials are really InteractiveBrowser and DeviceCode, which are not yet in DefaultAzureCredential.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like in .Net, they always point users to create an AAD application: https://github.com/Azure/azure-sdk-for-net/tree/master/sdk/textanalytics/Azure.AI.TextAnalytics#create-textanalyticsclient-with-azure-active-directory-credential

I think this is a good default too. If we don't ask users to do az login, we should ask them to create an AAD application.

Copy link
Contributor

Choose a reason for hiding this comment

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

From my understanding, you need to do the whole registering of application to get the "client id". So, all credentials that don't need client id should not need registering of the application either. That would be Azure CLI, VS Code and Managed Identity right?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is true. Other two that don't need this "client id" are InteractiveBrowser and DeviceCode. However, all of these credentials need some special setup: Azure CLI, VS Code and Managed Identity. If we don't provide instructions to create an AAD app, we should at least provide instructions to az login so that DefaultAzureCredential can be certain to work locally.

sdk/textanalytics/ai-text-analytics/README.md Outdated Show resolved Hide resolved
An **result**, such as `AnalyzeSentimentResult`, is the result of a Text Analytics operation, containing a prediction or predictions about a single text input. An operation's result type also may optionally include information about the input document and how it was processed.

The **error** object, `TextAnalyticsErrorResult`, indicates that the service encountered an error while processing the document and contains information about the error.

Copy link
Member

Choose a reason for hiding this comment

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

In our track 2 sdks we always have these user scenario examples in the readme. Do we want to keep these on the autogenerated client just like the other readmes? @deyaaeldeen @ramya-rao-a

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@KarishmaGhiya I am not sure if I correctly understand your comment. Do you mean why the code generator can not auto generate such user scenarios?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. So first, do we want to keep that section of "Examples" and if yes, then can the code generator autogenerate it? If not, then how was that there previously? Was this a manual sdk and then became auto-generated?

@deyaaeldeen
Copy link
Member Author

Closed in favor of Azure/autorest.typescript#971

@deyaaeldeen deyaaeldeen deleted the textanalytics/autogen-readme branch May 28, 2021 00:40
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jun 4, 2021
AKS: add listcredentials param in 2021-05-01 api (Azure#14655)

Co-authored-by: Li Ma <lima2@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants