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

[@azure/search-documents] Change EndPoint in TestResources Json #20058

Merged
merged 3 commits into from
Jan 26, 2022

Conversation

sarangan12
Copy link
Member

Checklists

  • Added impacted package name to the issue description

Packages impacted by this PR:
@azure/search-documents

Issues associated with this PR:
#17577

Describe the problem that is addressed by this PR:
This PR addresses the issue of updating search-documents tests to handle sovereign cloud endpoints dynamically. Earlier, this was done by adding 2 different endpoints (one for US and one for China). Ben has suggested a better approach where we can use just one endpoint and choose the suffix dynamically.

What are the possible designs available to address the problem
No detailed design. This approach is already used in .NET SDK and we are following the same here.

If there are more than one possible design, why was the one in this PR chosen?
N/A

Does this PR needs any fixes in the SDK Generator? (If so, create an Issue in the Autorest/typescript repository and link it here)
None

Are there test cases added in this PR?(If not, why?)
This is just updating the Json file and no seperate test cases are required.

Provide a list of related PRs(if any)
#19781

Command used to generate this PR:(Applicable only to SDK release request PRs)
N/A

@benbp Please review and approve the PR.

@sarangan12 sarangan12 requested a review from benbp January 25, 2022 19:44
@sarangan12 sarangan12 requested a review from xirzec as a code owner January 25, 2022 19:44
@ghost ghost added the Search label Jan 25, 2022
@benbp
Copy link
Member

benbp commented Jan 25, 2022

/azp run js - search-documents - tests-weekly

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@benbp
Copy link
Member

benbp commented Jan 25, 2022

/azp run js - search-documents - tests-weekly

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ramya-rao-a
Copy link
Contributor

@sarangan12 Once this approach is proven to work, please check the owners of other cognitive services to ensure we are following the same approach:

  • Text Analytics
  • Form Recognizer
  • Metrics Advisor

@sarangan12 sarangan12 merged commit 5939b94 into Azure:main Jan 26, 2022
@sarangan12
Copy link
Member Author

sarangan12 commented Jan 28, 2022

Text Analytics (Already Completed)

  1. Test Code - here
  2. Test Resources JSON - here and here
  3. Azure SDK Tools JSON - here

Form Recognizer - Needs Changes (Created PR #20123)
Metrics Advisor - Needs Changes

@sarangan12
Copy link
Member Author

@ramya-rao-a

  1. This task has already been completed for Text Analytics.
  2. Code changes have been completed and merged for Form Recognizer with PR [@azure/ai-form-recognizer] Update EndPoints in Form Recognizer #20123
  3. The Metrics Advisor uses a different logic of using a static resource for their testing and the endpoint (and whole set of related values) are defined in tests.yml file (which has only the keys. values are read from Keyvault during CI). So, this task would need a wider scope for enabling endpoints for US & China. @KarishmaGhiya could add additional information if required. At this point, I am not updating metrics advisor. Thanks.

@ramya-rao-a
Copy link
Contributor

Sounds good, thanks @sarangan12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants