-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Make storage account optional in the eventhub binder configuration. #23640
Make storage account optional in the eventhub binder configuration. #23640
Conversation
ZhuXiaoBing-cn
commented
Aug 18, 2021
- Fix that when there is the only producer, make storage account optional.
… storage account related content.
I don't think this fix the issue, for example and there is not test case to verify if this is fixed or not |
) | ||
.run(context -> { | ||
assertThat(context).hasSingleBean(EventHubClientFactory.class); | ||
EventHubClientFactory eventHubClientFactory = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not necessary to check the eventHubClientFactory object, but we need to make sure beans of EventHubOperation and EventHubNamespaceManager exist and should not have a bean of StorageAccountManager.
@@ -127,6 +126,10 @@ private String getStorageConnectionString(AzureEventHubProperties properties, | |||
final String accountKey = properties.getCheckpointAccessKey(); | |||
final StorageConnectionStringProvider provider; | |||
|
|||
if (accountName == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also add a condition on bean of StorageAccountManager, with the rely on property of account-name.
private AzureProperties azureProperties; | ||
|
||
private ApplicationContextRunner contextRunner = new ApplicationContextRunner() | ||
.withPropertyValues(EVENT_HUB_PROPERTY_PREFIX + "connection-string=Endpoint=sb://eventhub-test-1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add UTs with other types of credential configuration, and make sure it could work without storage configs.
@@ -105,6 +105,38 @@ public void testResourceManagerProvided() { | |||
}); | |||
} | |||
|
|||
@Test | |||
public void testEventHubOperationProvidedNotStorageUnder () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems the method name is not integral?
AZURE_PROPERTY_PREFIX + "subscription-id=sub" | ||
) | ||
.run(context -> { | ||
assertThat(context).hasSingleBean(EventHubNamespaceManager.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check StorageAccountManager bean here and for below test.
* Check StorageAccountManager bean is in unit tests.
/azp run java - spring - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM