jbertram commented on issue #2879: ARTEMIS-2535 Add ignorePartialResultException option to LDAPLoginModule URL: https://github.com/apache/activemq-artemis/pull/2879#issuecomment-548061786 Given that this issue looks to be well known as well as the relative simplicity of the code changes I'm inclined to give this my blessing. That said, this can't be merged as is. The PR test suite failed due to the failure of `org.apache.activemq.artemis.core.security.jaas.LDAPLoginModuleTest#testLogin`. I pulled your PR branch and ran the test locally and I can confirm the failure isn't spurious. To my surprise, the problem appears to be the removal of a seemingly innocuous bit of code that isn't even really related to the goal of this PR. In the method `org.apache.activemq.artemis.spi.core.security.jaas.LDAPLoginModule#resolveDN` this `if` statement was removed: ``` if (results.hasMore()) { // ignore for now } ``` I'm not sure what but *something* is happening in that call which is important. If removed the aforementioned test fails. If added back the test succeeds. The test also succeeds if the `if` statement is replaced with this: ``` results.close(); ``` My recommendation would be to just leave the `if` statement alone since it's not directly related to your PR. Also, I recommend adding something to your commit statement about the lack of testing so it will be clear in the future to whatever poor soul is digging through this code. Once the PR build passes successfully I'll merge it.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
