Skip to content

Update Regions.cs - Adding new region for USSec #4381

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.

Sign up for GitHub

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

Closed
trande4884 wants to merge 1 commit into from
Closed

Update Regions.cs - Adding new region for USSec #4381

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion Microsoft.Azure.Cosmos/src/Regions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,10 +261,15 @@ public static class Regions
public const string USSecEast = "USSec East";

/// <summary>
/// Name of the Azure USNat West region in the Azure Cosmos DB service.
/// Name of the Azure USSec West region in the Azure Cosmos DB service.
/// </summary>
public const string USSecWest = "USSec West";

/// <summary>
/// Name of the Azure USSec West Central region in the Azure Cosmos DB service.
/// </summary>
public const string USSecWestCentral = "USSec West Central";
Copy link
Member

@kundadebdatta kundadebdatta Apr 1, 2024

Choose a reason for hiding this comment

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

The current version of Cosmos.Direct package version 3.32.1 have the USSecWestCentral region marked as internal. See this code path for more details. This means that when the customer select this region, it will eventually fail during validation.

We will need to wait until a new version of Cosmos.Direct package is released which will mark this region as public. Can you use this TSG to create a new version of the direct package which contains the required changes please?

Copy link
Member

@kirankumarkolli kirankumarkolli Apr 2, 2024

Choose a reason for hiding this comment

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

@kundadebdatta how about adding a UT which iterates through all public and tries to use them for both ApplicationRegion and Preferred regions>

It will catch any gaps that might slip through right?

Copy link
Member

@kundadebdatta kundadebdatta Apr 2, 2024

Choose a reason for hiding this comment

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

Sounds great to me. That way, we could catch the invalid/ unavailable regions faster.

Copy link
Member Author

@trande4884 trande4884 Apr 2, 2024

Choose a reason for hiding this comment

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

@kirankumarkolli and @kundadebdatta Taylor and I are largely unfamiliar with this code, are you able to make the suggested changes to "catch gaps that might slip"? What is a "UT"? Some assistance on getting this done would be great!

Copy link
Member

@kundadebdatta kundadebdatta Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@trande4884 : "UT" basically stands for "Unit Test". While you work on the Direct package release, I can take this and update your PR to add the Unit Test to cover the scenario described.

Copy link
Member

@kundadebdatta kundadebdatta Apr 2, 2024

Choose a reason for hiding this comment

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

One more point - Can you please create a feature branch and submit a PR to address these changes. Forking is not supported for our pipelines and hence the PR will miss the necessary gates required for merging. Cc: @trande4884.

Copy link
Member

@kirankumarkolli kirankumarkolli Apr 3, 2024

Choose a reason for hiding this comment

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

With forking we need to explictly kick start the pipeline, I will do it. That's okey.

Copy link
Member

@ealsur ealsur Apr 3, 2024

Choose a reason for hiding this comment

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

This PR is the same as #4386?

Copy link
Contributor

@Tayham Tayham Apr 3, 2024

Choose a reason for hiding this comment

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

#4386 is going to be the PR going forward. This PR can be abandoned / closed.


/// <summary>
/// Name of the Azure Switzerland North region in the Azure Cosmos DB service.
/// </summary>
Expand Down