-
Notifications
You must be signed in to change notification settings - Fork 613
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
GH-2806: Pulsar binder defaults properties config #2811
Conversation
- Ensure that the Pulsar binder default properties can be properly expressed via spring.cloud.stream.pulsar.default property prefix. - Add the binder child context bean with the name binderName_binderProducingContext into the parent application context so that individual beans from the binder context can be easily queried. Resolves spring-cloud#2806
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.
Nice work @sobychacko ! A couple of minor comments to address.
...cloud-stream/src/main/java/org/springframework/cloud/stream/binder/DefaultBinderFactory.java
Show resolved
Hide resolved
private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() | ||
.withUserConfiguration(DefaultPropertiesTestApp.class) | ||
.withPropertyValues( | ||
"--spring.pulsar.client.service-url=" + PulsarTestContainerSupport.getPulsarBrokerUrl(), |
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 ❤️ the tests.
One question, is the discrepancy in the property pair prefix intended? (some have --
)
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.
Hmm, it was not intended. Didn't cause an issue. Do you want me to take care of them? or will you adjust on merge?
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.
If you can take care of them that would be good as I was hoping to just merge from Github for this one.
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! Thanks for the updates @sobychacko . I will merge soon.
Closed via 34aae4b |
Resolves #2806