-
Notifications
You must be signed in to change notification settings - Fork 40.4k
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
Add support for anonymousReadOnly in LdapProperties #11744
Conversation
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.
Thanks @eddumelendez - I've added a couple of comments.
@@ -60,6 +60,11 @@ | |||
*/ | |||
private Map<String, String> baseEnvironment = new HashMap<>(); | |||
|
|||
/** | |||
* Enable anonymous read only. |
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.
This description could use some love, something like
Whether read-only operations should use an anonymous environment.
import org.springframework.boot.test.util.TestPropertyValues; | ||
import org.springframework.context.annotation.AnnotationConfigApplicationContext; | ||
import org.springframework.boot.autoconfigure.AutoConfigurations; | ||
import org.springframework.boot.test.context.runner.ApplicationContextRunner; |
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.
Can the refactoring of the test be done in a separate commit please? First that with the test unchanged and then an extra commit on top with the feature.
8db9b0f
to
f897408
Compare
f897408
to
6fb14ed
Compare
* pr/11744: Polish "Add support for anonymousReadOnly in LdapProperties" Add support for anonymousReadOnly in LdapProperties Move tests to use ApplicationContextRunner
Thanks @eddumelendez - regarding the test refactoring, that's not what I had in mind by "separate commit". What I meant here is first a commit that upgrades the existing tests and then one commit on top that implements the feature (with the relevant tests). I've restructured your PR with that and I've also polished it |
@snicoll I'll take it into account next time. Thanks! 👍 |
See gh-11722