Skip to content
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

Fixes #116 - Ldap integration test #119

Merged

Conversation

luan-cestari
Copy link
Collaborator

Related to this issue #116 (this PR also incorporate the changes of #117 )

@luan-cestari
Copy link
Collaborator Author

Fixed some log dependency issues. Fixed the possible problem with cache that we found (but we didn't get evidences of the root cause). It need to finsih an unit test but I don't have enough knowledge about LDAP and Apache DS API, if someone could help with that, I can update this PR

@luan-cestari
Copy link
Collaborator Author

The unit test remaining just need to create a valid object into LDAP server to be queried

@coveralls
Copy link

Coverage Status

Coverage increased (+6.85%) to 64.69% when pulling 52ea785 on luan-cestari:LDAPIntegrationTest#116 into df553a9 on lightblue-platform:master.

@jewzaam
Copy link
Member

jewzaam commented Mar 19, 2015

@luan-cestari does this fix #116? And is there any more work to do before review? Your last comment makes me think something needs done with a unit test still.

@luan-cestari luan-cestari changed the title Ldap integration test#116 Fixes #116 - Ldap integration test Mar 19, 2015
@luan-cestari
Copy link
Collaborator Author

@jewzaam it probably solve it as we don't have enough data about the issue, so I wouldn't guarantee it solves the problem. The remaining test is something good to have, I ping some people on the channel for help, I think Derek is working on it.

try {
userRoles.addAll(getUserRolesFromCache(userName));
List<String> userRolesFromCache = getUserRolesFromCache(userName);
if( userRolesFromCache != null && userRolesFromCache.isEmpty() ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Condition should be !userRolesFromCache.isEmpty() (missing !)

@jewzaam
Copy link
Member

jewzaam commented Mar 19, 2015

I'll merge after condition for using roles from cache is update.
Per note in this PR and @derek63 working on something for the unit test I am skipping review of the test. I'll expect a separate PR coming with heavy updates.

Missed the '!' on line 66. Now it is correct: if( userRolesFromCache != null && !userRolesFromCache.isEmpty() ) {
@luan-cestari
Copy link
Collaborator Author

Fixed the line commented by @jewzaam

@coveralls
Copy link

Coverage Status

Coverage increased (+6.85%) to 64.69% when pulling 8391878 on luan-cestari:LDAPIntegrationTest#116 into df553a9 on lightblue-platform:master.

jewzaam added a commit that referenced this pull request Mar 19, 2015
@jewzaam jewzaam merged commit efcbc52 into lightblue-platform:master Mar 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants