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

adding prop and condition for OperationalInsightsEndpointResourceId #414

Merged
merged 8 commits into from
May 9, 2024

Conversation

andrewpethel
Copy link
Contributor

@andrewpethel andrewpethel commented Apr 4, 2024

Task:
Update Azure.PowerShell.Common so that the Add-AzEnvironment auto discovery feature is agnostic for Sovereign Clouds.

Problem Description:
The cmdlet's current implementation relies on the ExtendedProperties setting the OperationalInsightsEndpoint on the active AzureEnvironment. For user-added environments, such as those added via Add-AzEnvironment, both the -OperationalInsightsEndpoint and -OperationalInsightsEndpointResourceID parameters must be defined for the Invoke command to work. This requirement is documented, but could be clearer and more user-friendly.

Proposed Solution:
This PR updates the MapArmToAzureEnvironment method to auto-populate the OperationalInsightsEndpoint and OperationalInsightsEndpointResourceId values, following a pattern similar to Synapse. This update would leverage the auto-discovery feature used with Add-AzEnvironment. By implementing this change, we can improve the robustness of the cmdlet and enhance the user experience for those working with custom Azure environments, namely Sovereign Clouds.

All tests pass (updated screenshot -- April 24, 2024):

image

Nickcandy
Nickcandy previously approved these changes Apr 16, 2024
msJinLei

This comment was marked as outdated.

@Nickcandy Nickcandy self-requested a review April 17, 2024 05:10
@Nickcandy Nickcandy dismissed their stale review April 17, 2024 05:11

Something wrong with this pr

@msJinLei msJinLei dismissed their stale review April 17, 2024 10:07

will add new comments

src/Authentication.Abstractions/AzureEnvironment.cs Outdated Show resolved Hide resolved
src/Authentication.Abstractions/AzureEnvironment.cs Outdated Show resolved Hide resolved
src/Authentication.Abstractions/AzureEnvironment.cs Outdated Show resolved Hide resolved
src/Authentication.Abstractions/AzureEnvironment.cs Outdated Show resolved Hide resolved
@msJinLei msJinLei self-requested a review April 17, 2024 10:10
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

@andrewpethel thanks a lot for the contribution! This will be included in the upcoming milestone.

@isra-fel isra-fel merged commit baa2eed into Azure:main May 9, 2024
2 checks passed
@andrewpethel
Copy link
Contributor Author

andrewpethel commented May 9, 2024 via email

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

Successfully merging this pull request may close these issues.

4 participants