-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
/azp run js - ai-form-recognizer - tests-weekly |
Azure Pipelines successfully started running 1 pipeline(s). |
* The authentication scope will be set from this audience. | ||
* See {@link KnownFormRecognizerAudience} for known audience values. | ||
*/ | ||
audience?: string; |
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.
Is this the accepted approach to set the scope across all the different packages/languages?
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.
Reference:
I went through the notes, issues, references, and the recorded Arch board meeting on this.
And this was the approach followed in other places, which aligns with the discussion from the arch board meeting.
- .NET - Update cloud configuration API azure-sdk-for-net#23141
- JS -
azure-sdk-for-js/sdk/containerregistry/container-registry/src/containerRegistryClient.ts
Lines 36 to 41 in 809f05c
/** * Gets or sets the audience to use for authentication with Azure Active Directory. * The authentication scope will be set from this audience. * See {@link KnownContainerRegistryAudience} for known audience values. */ audience?: string; - JAVA - https://github.com/samvaity/azure-sdk-for-java/blob/e0d055cbe7f575c5ba004d1b07a9a48487cf6f6e/sdk/formrecognizer/azure-ai-formrecognizer/src/main/java/com/azure/ai/formrecognizer/models/FormRecognizerAudience.java#L11-L19
I'll however get it reviewed by architects and others. 🙂
*/ | ||
export enum KnownFormRecognizerAudience { | ||
/** Azure China */ | ||
AzureResourceManagerChina = "https://cognitiveservices.azure.cn/.default", |
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 is nothing special about these links relating to FormRecognizer
, should it be KnownCognitiveServicesAudience
instead and declared at some common place?
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.
That would make sense if we had a common package for cognitive services which we don't...
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.
Yes, sad.
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 duplicate the code and use better name for now.
/azp run js - ai-form-recognizer - tests-weekly |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run js - ai-form-recognizer - tests-weekly |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run js - ai-form-recognizer - tests-weekly |
Azure Pipelines successfully started running 1 pipeline(s). |
trigger: none | ||
|
||
stages: | ||
- template: /eng/pipelines/templates/stages/archetype-sdk-tests.yml | ||
parameters: | ||
PackageName: "@azure/ai-form-recognizer" | ||
ServiceDirectory: formrecognizer | ||
Location: "${{ parameters.Location }}" | ||
SupportedClouds: 'Public,UsGov,China' | ||
EnvVars: | ||
AZURE_CLIENT_ID: $(aad-azure-sdk-test-client-id) |
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.
These will need to be changed to not be hardcoded to the public cloud values, but rather the env vars dynamically set by the test infra, e.g.
AZURE_CLIENT_ID: $(FORMRECOGNIZER_CLIENT_ID)
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.
@HarshaNalluru it's possible app configuration reads the APPCONFIGURATION_CLIENT_ID, etc. variables and doesn't even use those old ones anymore.
/azp run js - ai-form-recognizer - tests-weekly |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run js - ai-form-recognizer - tests-weekly |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run js - ai-form-recognizer - tests-weekly |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run js - ai-form-recognizer - tests-weekly |
Hi @HarshaNalluru. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
Hi @HarshaNalluru. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
API change check APIView has identified API level changes in this PR and created following API reviews. |
… harshan/issue/19442
/azp run js - ai-form-recognizer - tests-weekly |
Azure Pipelines successfully started running 1 pipeline(s). |
Hi @HarshaNalluru. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
Hi @HarshaNalluru. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
Hi @HarshaNalluru. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing |
Packages impacted by this PR
@azure/ai-form-recognizer
Issues associated with this PR
Fixes #19442
Describe the problem that is addressed by this PR
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Adds the audience option in the client options to support the UsGov and China clouds
Based on the prior art and the architecture board discussions(links below), this design was chosen.
Provide a list of related PRs/issues
azure-sdk-for-js/sdk/containerregistry/container-registry/src/containerRegistryClient.ts
Lines 36 to 41 in 809f05c
https://dev.azure.com/azure-sdk/internal/_wiki/wikis/internal.wiki/604/Sovereign-Cloud-Testing
Are there test cases added in this PR? (If not, why?)
No, but the existing test cases are updated to handle the multiple clouds by checking the endpoint.
Checklists