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

[Form Recognizer] Sovereign clouds support #20522

Closed
wants to merge 34 commits into from
Closed
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
c01f303
Sovereign cloulds support
HarshaNalluru Feb 24, 2022
1e224f2
Update sdk/formrecognizer/ai-form-recognizer/CHANGELOG.md
HarshaNalluru Feb 24, 2022
6fb733c
SupportedClouds: 'Public,UsGov,China'
HarshaNalluru Feb 28, 2022
4b60789
Merge branch 'harshan/issue/19442' of https://github.com/HarshaNallur…
HarshaNalluru Feb 28, 2022
1396d04
remove location
HarshaNalluru Feb 28, 2022
e5bfacc
subscription().subscriptionId
HarshaNalluru Feb 28, 2022
31d73e0
function getAudience(): KnownFormRecognizerAudience
HarshaNalluru Feb 28, 2022
0a2fd3f
tests.yml update
HarshaNalluru Feb 28, 2022
72b494d
dummy commit (trigger pipeline)
HarshaNalluru Mar 1, 2022
202f01c
env file
HarshaNalluru Mar 1, 2022
118a7c7
Revert "env file"
HarshaNalluru Mar 1, 2022
9b85338
changes from feedback
HarshaNalluru Mar 1, 2022
cd5f600
Update sdk/formrecognizer/ai-form-recognizer/test/public/training.spe…
HarshaNalluru Mar 1, 2022
6a6ebf4
Update sdk/formrecognizer/ai-form-recognizer/CHANGELOG.md
HarshaNalluru Mar 1, 2022
8339675
more feedback
HarshaNalluru Mar 1, 2022
4f91175
improve docs
HarshaNalluru Mar 1, 2022
8a72062
elaborate
HarshaNalluru Mar 1, 2022
5e7fbd8
export { FormRecognizerAudience } from "./constants";
HarshaNalluru Mar 1, 2022
9d5e322
Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…
HarshaNalluru Mar 2, 2022
b748056
MORE FEEDBACK
HarshaNalluru Mar 2, 2022
61a3a1d
Update sdk/formrecognizer/ai-form-recognizer/CHANGELOG.md
HarshaNalluru Mar 3, 2022
10acd7d
Update sdk/formrecognizer/ai-form-recognizer/tests.yml
HarshaNalluru Mar 3, 2022
b6c7973
testing unparallel sample runs
HarshaNalluru Mar 9, 2022
51efcf7
chnages
HarshaNalluru Feb 1, 2023
d22872a
Merge branch 'harshan/issue/19442' of https://github.com/HarshaNallur…
HarshaNalluru Feb 1, 2023
4929132
merge main
HarshaNalluru Feb 1, 2023
764c398
Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…
HarshaNalluru Feb 2, 2023
014da41
fix build
HarshaNalluru Feb 2, 2023
acf524d
Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…
HarshaNalluru Feb 3, 2023
cde7237
custom forms test
HarshaNalluru Feb 3, 2023
89f473b
pdf file stream
HarshaNalluru Feb 3, 2023
143ae75
recordings
HarshaNalluru Feb 3, 2023
5326004
w2-single.png
HarshaNalluru Feb 3, 2023
baee1b6
Merge branch 'harshan/fix-FR-test-failures' of https://github.com/Har…
HarshaNalluru Feb 3, 2023
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
Prev Previous commit
Next Next commit
improve docs
HarshaNalluru authored Mar 1, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 4f9117570799d08a62f4ad2b17b5bc390cb5d9a9
15 changes: 15 additions & 0 deletions sdk/formrecognizer/ai-form-recognizer/src/constants.ts
Original file line number Diff line number Diff line change
@@ -3,6 +3,21 @@

/**
* Defines known cloud audiences for Form Recognizer.
Copy link
Member

Choose a reason for hiding this comment

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

Would be helpful to customers to have a brief description of what this means, practically speaking.

*
* --- More about national clouds ---
*
* National clouds are physically isolated instances of Azure.
Copy link
Member

Choose a reason for hiding this comment

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

Some more comments on this section:

  • "physically isolated" could be interpreted to mean an air-gapped or offline system, which isn't what we're referring to here.
  • it helps readability if each line is no longer than about 120 characters (I recommend setting up 80 and 120 character rulers in VSCode settings and splitting documentation lines at 120 characters, so that the section is still readable in text views without automatic line breaks (such as GitHub)
  • I don't know if we need to describe what a sovereign cloud is so much as how to use this value, which your last sentences get at. What about just:
/**
 * Defines the known cloud audiences for Form Recognizer.
 *
 * To authenticate with Azure Active Directory (using a `TokenCredential`) in a [Sovereign Cloud](<link here>) 
 * environment, provide the appropriate value below as the `audience` option when creating a 
 * `DocumentAnalysisClient` or `DocumentModelAdministrationClient`.
 *
 * The default value is suitable for Form Recognizer resources created in the Azure Public Cloud, so this value
 * is only required to use Form Recognizer in a different cloud environment.
 */

* These regions of Azure are designed to make sure that data residency, sovereignty, and compliance requirements are honored within geographical boundaries.
*
* For more information, refer https://docs.microsoft.com/azure/active-directory/develop/authentication-national-cloud.
*
* As of now, FormRecognizer supports the following audiences.
*
* -------
*
* For authentication with Azure Active Directory, use this as "audience" as part of the constructor client options.
*
* You should only need this to be set as "audience" if you are using AAD/token credential and if you are using a cloud other than the `AzurePublicCloud`(default).
*/
export enum FormRecognizerAudience {
Copy link
Member

Choose a reason for hiding this comment

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

is this not publicly exported? I don't see it in the APIView

Copy link
Member Author

Choose a reason for hiding this comment

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

oh shoot, I did wonder, but didn't realize.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

/** Azure China */
Original file line number Diff line number Diff line change
@@ -70,6 +70,11 @@ export interface FormRecognizerCommonClientOptions extends CommonClientOptions {
apiVersion?: FormRecognizerApiVersion;
/**
* Gets or sets the audience to use for authentication with Azure Active Directory.
Copy link
Member

Choose a reason for hiding this comment

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

The doc string would be clearer if it communicated that it is only necessary to override this setting if you're 1. using AAD/TokenCredential and 2. are using a cloud other than Azure Public.

Copy link
Member

Choose a reason for hiding this comment

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

gets/sets feels out of place for a property of an object that is consumed by the client constructor. Setting it after the fact won't change the scope used, right?

The rest of this doc string is great, but for the first line, how about just:

"The audience (scope) to use for authentication with Azure Active Directory."

Copy link
Member Author

Choose a reason for hiding this comment

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

gets/sets feels out of place for a property of an object that is consumed by the client constructor.

Copied this from some other package.

* Setting this option is only necessary
* - if you are using AAD/token credential
* (and)
* - if you are using a cloud other than the `AzurePublicCloud` ("https://cognitiveservices.azure.com/.default")
*
* The authentication scope will be set from this audience.
* See {@link FormRecognizerAudience} for known audience values.
*/