[GitHub] [guacamole-client] mike-jumper commented on issue #345: GUACAMOLE-234: Migrate to Apache Directory API for LDAP Extension

2019-08-10 Thread GitBox
mike-jumper commented on issue #345: GUACAMOLE-234: Migrate to Apache Directory 
API for LDAP Extension
URL: https://github.com/apache/guacamole-client/pull/345#issuecomment-520194065
 
 
   OK, I've re-reviewed. I think everything looks fine with the exception of 
the comments above. Specifically:
   
   * Naming a function after its relevant noun feels funky 
(`referralConnection()`)
   * The documentation of the `searchHop` parameter is unclear to me.
   * One call to the logger included the exception at a level higher than 
debug. Exceptions should only be logged at the debug level or lower.
   * There are a few cases where `e.getMessage()` is included in the call to 
the logger, yet there is no `{}` to tell the logger where to put that message. 
I'm not sure if that will work. Even if it does, it's probably better to be 
explicit and use the formatting we've used elsewhere when intentionally 
including an exception message.
   * A single space seems to have magically materialized.
   


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [guacamole-client] mike-jumper commented on issue #345: GUACAMOLE-234: Migrate to Apache Directory API for LDAP Extension

2019-06-17 Thread GitBox
mike-jumper commented on issue #345: GUACAMOLE-234: Migrate to Apache Directory 
API for LDAP Extension
URL: https://github.com/apache/guacamole-client/pull/345#issuecomment-502880455
 
 
   @necouchman - I just looked over things quickly and things look fine at 
first glance. I'll do a more thorough review in a bit, but I think this is 
probably ready to move forward.
   
   There is a merge conflict noted by GitHub for `ConnectionService.java` which 
will need to be resolved.
   
   Earlier comments noted warnings being repeatedly logged by parts of the 
Apache Directory API:
   
   > ```
   > ...
   > 11:26:19.232 [NioProcessor-1] WARN o.a.d.a.l.m.entry.DefaultAttribute - 
ERR_13207_VALUE_ALREADY_EXISTS The value 'CN=DC1,OU=Sunday,OU=Domain 
Controllers,DC=example,DC=local' already exists in the attribute 
(msDS-RevealedDSAs)
   > 11:26:19.233 [NioProcessor-1] WARN o.a.d.a.l.m.entry.DefaultAttribute - 
ERR_13207_VALUE_ALREADY_EXISTS The value 'CN=DC1,OU=Sunday,OU=Domain 
Controllers,DC=example,DC=local' already exists in the attribute 
(msDS-RevealedDSAs)
   > 11:26:19.233 [NioProcessor-1] WARN o.a.d.a.l.m.entry.DefaultAttribute - 
ERR_13207_VALUE_ALREADY_EXISTS The value 'CN=DC1,OU=Sunday,OU=Domain 
Controllers,DC=example,DC=local' already exists in the attribute 
(msDS-RevealedDSAs)
   > ...
   > ```
   
   Anyone know whether this happens as frequently as it looks here (or what 
needs to be fixed to remove these warnings)? Will Guacamole logs be filled with 
LDAP warnings following merge?


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:
us...@infra.apache.org


With regards,
Apache Git Services