Storage identity remove resource manager provider by yevster · Pull Request #15837 · Azure/azure-sdk-for-java

yevster · 2020-09-30T19:37:58Z

No description provided.

…dk-for-java into storage-identity

…th default identity

…ections

…th default identity

…ections

…ava into storage-identity

…rovider Merging from master.

…n and break when attmepting to use 2.0 version of resource management

…rovider

string.

jialindai · 2020-10-28T07:51:02Z

sdk/spring/azure-identity-spring-library/pom.xml

+        <artifactId>azure-spring-boot-service</artifactId>
+        <version>1.0.0</version>
+    </parent>
+    <artifactId>azure-identity-spring-library</artifactId>


Suggest change the artifact id to azure-identity-spring

@mnriem, any objection to eliminating the Spring Environment identity library entirely and moving this functionality to the azure-spring-cloud-context module?

jialindai · 2020-10-28T08:24:00Z

...library/src/main/java/com/microsoft/azure/identity/spring/SpringEnvironmentTokenBuilder.java

+            credentials.put(name, credential);
+            return;
+        }
+


For system assigned identity, tenant id is needed.
For user assigned identity, tenant id and one optional client id is needed.

Code here has some problems work with system assigned identity and user assigned identity.

Would it be simpler to always fall back to DefaultAzureCredentialBuilder？

I defer to @mnriem on this.

For system assigned you do not need tenant id. User-assigned was not targeted and would need to be added if required.

jialindai · 2020-10-28T08:41:20Z

...ary/src/test/java/com/microsoft/azure/identity/spring/SpringEnvironmentTokenBuilderTest.java

+		} catch (Throwable t) {
+			assertEquals(IllegalStateException.class, t.getClass(),
+					"Unexpected exception class on missing configuration field.");
+		}


We can use assertThrows() here.

I prefer not to use Exception annotations for generic exceptions (e.g. IllegalStateException). Because these can be thrown from just about anywhere, it's important to ensure that it gets thrown from a particular place, not just anywhere in the test method.

jialindai · 2020-10-28T08:52:08Z

...library/src/main/java/com/microsoft/azure/identity/spring/SpringEnvironmentTokenBuilder.java

+     */
+    public SpringEnvironmentTokenBuilder fromEnvironment(Environment environment) {
+        populateNamedCredential(environment, "");
+        String credentialNamesKey = AZURE_CREDENTIAL_PREFIX + "names";


Is it possible to consider make use of @ConfigurationProperties and model different credentials as Map, so that we don't need the 'names' here.

This is a repackaging of @mnriem's code, so perhaps he can answer better. My understanding is that a custom map is needed because property values could come from multiple sources, and this serves to consolidate them in a single place.

The 'names' defines the order of the chain.

jialindai · 2020-10-28T09:01:14Z

.../azure-spring-boot-sample-storage-resource/src/main/java/com/example/StorageApplication.java

+
+/**
+ * @author Warren Zhu
+ */


Remove @author annotation

jialindai · 2020-10-28T09:02:29Z

...-samples/azure-spring-boot-sample-storage-resource/src/main/resources/application.properties

+# and use numbers and lower-case letters only.
+spring.cloud.azure.storage.account=ybstortst2
+
+spring.cloud.azure.resource-group=test


resource group name should be replaced with some placeholder

jialindai · 2020-10-28T09:02:51Z

...-samples/azure-spring-boot-sample-storage-resource/src/main/resources/application.properties

+spring.cloud.azure.resource-group=test
+
+# Change into your containerName and blobName
+blob=azure-blob://ybstrcntnr/testblob.txt


blob uri should be replaced with some placeholder

jialindai · 2020-10-28T09:17:06Z

...les/azure-spring-boot-sample-storage-resource/src/test/resources/application-test.properties

+spring.cloud.azure.storage.account=springcloud
+
+spring.cloud.azure.region=westUS
+spring.cloud.azure.auto-create-resources=true


Better to use engineering system's capability to provision Azure resource dynamically. Better not to leave storage URL in config file.

jialindai · 2020-10-28T09:18:24Z

...zure-spring-boot-sample-storage-resource/src/test/java/com/example/StorageApplicationIT.java

+@AutoConfigureMockMvc
+@TestPropertySource(
+        locations = "classpath:application-test.properties")
+public class StorageApplicationIT {


Better to use dynamic provisioned Azure resource for test. SDK engineering system provide such capability.

It's also OK to remove test case for sample.

jialindai · 2020-10-28T09:19:21Z

sdk/spring/azure-spring-boot-samples/azure-spring-cloud-eventhubs-binder-sample/pom.xml

@@ -1,90 +1,98 @@
 <?xml version="1.0" encoding="UTF-8"?>


Should use space instead of tab.

…rovider

saragluna · 2020-10-28T09:29:25Z

@yevster I've pushed a commit to your branch to solve conflicts. Please remember to pull before push if you want to add more commits.

saragluna · 2020-10-28T09:31:14Z

sdk/spring/azure-spring-boot-samples/azure-spring-boot-sample-storage-resource/README.adoc

@@ -0,0 +1,115 @@
+= Spring Cloud Azure Storage Starter Sample


We already have the sample in our samples, https://github.com/Azure/azure-sdk-for-java/tree/master/sdk/spring/azure-spring-boot-samples/azure-spring-cloud-sample-storage-resource, so I suggest we remove this one.

@yevster I've pushed a commit to your branch to solve conflicts. Please remember to pull before push if you want to add more commits.

@saragluna, I had in-flight changes fixing the event hub binders sample. Now, I see that sample, along with several others have been removed. May I ask why?

I'll try to schedule a meeting this morning (your time) to discuss.

parameter not set (i.e. that bean would not be initialized)

…rovider

…source-manager-provider

…ings

stliu · 2021-06-10T07:33:28Z

is this still active?

chenrujun · 2021-06-11T00:12:01Z

I think it can be closed. Because we use this PR instead: #17119

@saragluna , please close this PR if you think it's OK to close it.

stliu · 2021-06-15T14:31:18Z

i'm closing this PR since it's been a long time inactive, please reopen if needed

mnriem and others added 24 commits August 5, 2020 10:34

In progress

03f8ab4

Throw an exception when configuration is incomplete

7266ac5

Added class level JavaDoc

0de9b40

Resolve POM conflict

3964b68

Added unit tests

9fb49ba

Merge branch 'credential-helper' of https://github.com/mnriem/azure-s…

9acd2b9

…dk-for-java into storage-identity

Changing the Identity Helper into a builder

bedcf50

Adding storage resource sample, autoconfiguration for blob storage wi…

9ad8a79

…th default identity

Attempting to generate an endpoint string

f9adbe0

Provisional workaround for using IdentityToken in Shared service conn…

9cac0d3

…ections

Merge branch 'master' into storage-identity

c2bbb46

In progress

41f32fa

Throw an exception when configuration is incomplete

8c1297b

Added class level JavaDoc

f0322af

Resolve POM conflict

8a6385c

Added unit tests

9305ee1

Changing the Identity Helper into a builder

92fd3dd

Adding storage resource sample, autoconfiguration for blob storage wi…

06d8aa9

…th default identity

Attempting to generate an endpoint string

723e463

Provisional workaround for using IdentityToken in Shared service conn…

62c5533

…ections

Merge branch 'storage-identity' of github.com:yevster/azure-sdk-for-j…

b96c739

…ava into storage-identity

XML syntax fix

3f7e8ac

Storage resource demo works

670e4a2

Removing tight Resource Manager Provider coupling

cfef365

ghost added the azure-spring All azure-spring related issues label Sep 30, 2020

yevster added 5 commits October 2, 2020 17:29

Merge branch 'master' into storage-identity-remove-resource-manager-p…

f7f9414

…rovider Merging from master.

Fixing autoconfiguration for legacy resource management to not kick i…

9662b85

…n and break when attmepting to use 2.0 version of resource management

Merge branch 'master' into storage-identity-remove-resource-manager-p…

6f780a9

…rovider

Resource demo works with file legacy file credentail'

ef3f807

Resource sample works with environment parameters

525c5b4

yevster added 3 commits October 22, 2020 17:34

Fixing autoconfig dependencies for Topic binders

bbc811e

Fixing startup failure when namespace is provided instead of connection

01b1378

string.

Updating to incorporate upstream library signature changes

cc74b40

jialindai suggested changes Oct 28, 2020

View reviewed changes

Merge branch 'master' into storage-identity-remove-resource-manager-p…

cdb5ed4

…rovider

include version tag should be on the same line

bccd6e2

saragluna requested changes Oct 28, 2020

View reviewed changes

yevster added 15 commits October 28, 2020 16:49

Spring Cloud Stream - Event Hub binder demo now works

9a206f1

merging with master

f19af87

Fixing build failures

7bec935

Preventing reliance on resource management bean when resource group

456d383

parameter not set (i.e. that bean would not be initialized)

Merge branch 'master' into storage-identity-remove-resource-manager-p…

9b1792b

…rovider

Fixing NPE in auto-configuration

fcb8bc8

Fixing NPE in auto-configuration

a6cfcfe

Fixing package names broken in merge

8277c89

Checkstyle appeasement

005e67f

Checkstyle appeasement

552af0c

Merge branch 'context-config-npe-fix' into storage-identity-remove-re…

2456b26

…source-manager-provider

Eliminating ServiceBus ARM dependency when not pulled in thorugh sett…

8558375

…ings

Deleting duplicate sample

dde0594

Removing module for superfluous storage resource demeo

2a7d5a3

Fixing parent pom of identity library

5531c84

saragluna mentioned this pull request Nov 3, 2020

Spring storage remove resource manager provider #17119

Merged

chenrujun assigned saragluna Jun 11, 2021

stliu closed this Jun 15, 2021

		@@ -0,0 +1,115 @@
		= Spring Cloud Azure Storage Starter Sample

Conversation

yevster commented Sep 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saragluna commented Oct 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yevster Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stliu commented Jun 10, 2021

Uh oh!

chenrujun commented Jun 11, 2021

Uh oh!

stliu commented Jun 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yevster Oct 28, 2020 •

edited

Loading