Fix spring cloud env configuration #21613
Conversation
yiliuTo
requested review from
backwind1233,
chenrujun,
saragluna and
stliu
as code owners
ghost
added
the
yiliuTo
added
the
Client
yiliuTo
force-pushed
the
fix-spring-cloud-env-configuration
branch
from
a220fb5 to
71f4a9c
Compare
|
/azp run java - spring - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
have you seen this one ? https://github.com/Azure/azure-sdk/pull/1550/files |
| /** | ||
| * Enum to define the authority host uri for all Azure Cloud environments: Azure, Azure China... | ||
| */ | ||
| public enum AzureCloud { |
There was a problem hiding this comment.
To me this should be part of azure-core instead of be part of our spring module, since all azure sdk requires this kind of configuration.
would you confirm?
| @@ -39,9 +39,12 @@ private TokenCredential populateTokenCredential(String prefix, boolean createDef | |||
| String tenantId = getPropertyValue(prefix, "tenant-id"); | |||
| String clientId = getPropertyValue(prefix, "client-id"); | |||
| String clientSecret = getPropertyValue(prefix, "client-secret"); | |||
| AzureCloud azureCloud = getPropertyValue(AzureCloud.class, prefix, "cloud-name", AzureCloud.Azure); | |||
There was a problem hiding this comment.
I'm really not a big fan of using this cloud-name as property name, wired and not clear
| } | ||
|
|
||
| private static AzureEnvironment toAzureEnvironment(AzureCloud azureCloud) { |
There was a problem hiding this comment.
why not we just use the AzureEnvironment from sdk?
| /** | ||
| * Enum to define the authority host uri for all Azure Cloud environments: Azure, Azure China... | ||
| */ | ||
| public enum AzureCloud { |
There was a problem hiding this comment.
This configuration is available here in azure-identity SDK,
Since spring identity SDK takes a compile dependency on azure-identity.
It can use the AzureAuthorityHosts class from it.
There was a problem hiding this comment.
Thanks, we will refactor our implemention here then
| @@ -30,7 +30,7 @@ | |||
|
|
|||
| private String resourceGroup; | |||
|
|
|||
| private AzureEnvironments environment = AzureEnvironments.Azure; | |||
There was a problem hiding this comment.
you're not deprecating this property as change log stated but removing it
|
Close this PR for #22396 refactors it. |
This pr is to fix the bug in #21336 that spring cloud libraries cannot support all azure clouds. The solution is to provide another property "spring.cloud.azure.cloud-name" to allow users to specify the cloud name, and deprecate the old one "spring.cloud.azure.environment".