Merge built-in environments and discovered environemnts #203
Conversation
| @@ -0,0 +1,57 @@ | |||
| [ | |||
| { | |||
| "portal": "https://portal.azure.us", | |||
There was a problem hiding this comment.
I think we might want to add a specific new piece of data with only one enbtry, just for completeness. We want to make sure we protect ourselves from future bugs where we make assumptions about this payload.
There was a problem hiding this comment.
Sure. I will add it.
| var armEnvironments = AzureEnvironment.InitializeBuiltInEnvironments(null, httpOperations: TestOperationsFactory.Create().GetHttpOperations()); | ||
|
|
||
| // Check AzureCloud is added to public environment list even discovery endpoint doesn't return AzureCloud. | ||
| Assert.True(AzureEnvironment.PublicEnvironments.ContainsKey(EnvironmentName.AzureCloud)); |
There was a problem hiding this comment.
I would assert this for all of the built-in environments
There was a problem hiding this comment.
Sure. I will add them.
| @@ -0,0 +1,57 @@ | |||
| [ | |||
| { | |||
There was a problem hiding this comment.
I also got a report on the same thread that there are additional errors that prevent module load when customers use the ARM_CLOUD_METADATA_URL environment variable to point at a metadata endpoint that does not have the 'AzureCloud' entry. I believe we have a test using the environment variable for discovery, and we should ensure that it also convers this scenario.
There was a problem hiding this comment.
You are right. The discovery approach via default DNS, parameter, or environment variable are going into the same logic. It should be covered.
|
AzureEnvironment.InitializeBuiltInEnvironments is an internal method and public cloud map will be populated after this method. So, in the test case, check public environment variable is not correct because only InitializeBuiltInEnvironments is called. I only check public cloud environment is correctly returned in test cases. |
2 participants
Fix for this issue
Azure/azure-powershell#12633