Skip to content

[@azure/search-documents] Enable national cloud support for Azure Search SDK#22887

Merged
sarangan12 merged 4 commits intoAzure:mainfrom
sarangan12:Issue22813
Aug 15, 2022
Merged

[@azure/search-documents] Enable national cloud support for Azure Search SDK#22887
sarangan12 merged 4 commits intoAzure:mainfrom
sarangan12:Issue22813

Conversation

@sarangan12
Copy link
Copy Markdown
Contributor

Packages impacted by this PR

@azure/search-documents

Issues associated with this PR

#22813

Describe the problem that is addressed by this PR

This PR is to enable national cloud support for Azure Search Documents. This PR uses the .NET PR - Azure/azure-sdk-for-net#30316 as reference.

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?

This is a standard straight forward implementation. No specific design concerns.

Provide a list of related PRs (if any)

Azure/azure-sdk-for-net#30316

Checklists

  • Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)

@ghost ghost added the Search label Aug 12, 2022
@azure-sdk
Copy link
Copy Markdown
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-search-documents

Copy link
Copy Markdown
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

A nice little feature, just one concern about the audience typing on the options bag

export interface SearchClientOptions extends ExtendedCommonClientOptions {
// @deprecated
apiVersion?: string;
audience?: SearchAudience;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm concerned about taking the enum type here, since presumably there will be additional national clouds or perhaps even unlisted ones that can be passed as well. I feel like this should be string but have a ref comment that mentions using the SearchAudience enum for convenience.

@bterlson @joheredi thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@xirzec You are right. Modified to make it string and changed the other name to include Known

Copy link
Copy Markdown
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Looks great!

@sarangan12 sarangan12 merged commit 9d93006 into Azure:main Aug 15, 2022
sarangan12 added a commit that referenced this pull request Jun 3, 2024
…Ks (#29871)

### Packages impacted by this PR

1. @azure/monitor-ingestion
2. @azure/monitor-query

### Issues associated with this PR

1. [#27280](#27280) 
2. [#28821](#28821) 

### Describe the problem that is addressed by this PR

In order to support sovereign clouds, we must be able to allow the SDK
users to specify the endpoints and audience parameters. The users could
already specify/modify the endpoint parameter. This PR will enable the
users to do the same for audience.

### 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?

No special design concerns to discuss.

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

#22887

### Command used to generate this PR:**_(Applicable only to SDK release
request PRs)_

### Checklists
- [X] Added impacted package name to the issue description
- [ ] Does this PR needs any fixes in the SDK Generator?** _(If so,
create an Issue in the
[Autorest/typescript](https://github.com/Azure/autorest.typescript)
repository and link it here)_
- [X] Added a changelog (if necessary)

@KarishmaGhiya @srnagar Please review and approve the PR.

@xirzec FYI

---------

Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>
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