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

Added unit testing for LDAP group sync #5669

Merged

Conversation

stevekuznetsov
Copy link
Contributor

Addresses technical debt left over for the 3.1 push for LDAP group sync.

Depends on #5616
Depends on UPSTREAM go-ldap/ldap#38

/cc @deads2k @liggitt

@smarterclayton smarterclayton added this to the 1.1.x milestone Nov 4, 2015
@smarterclayton smarterclayton changed the title [post-3.1] added unit testing for LDAP group sync Added unit testing for LDAP group sync Nov 4, 2015
@stevekuznetsov stevekuznetsov mentioned this pull request Nov 12, 2015
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2015
@stevekuznetsov
Copy link
Contributor Author

@deads2k This is next. Rebased and ready to go.

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2015
// newDefaultTestGroup returns a new LDAP entry with the following characteristics:
// dn: cn=testGroup,ou=groups,dc=example,dc=com
// cn: testGroup
func newDefaultTestGroup() *ldap.Entry {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should accept the testGroupDN as argument so that I don't have to know a global var to reason about the tests.

@deads2k
Copy link
Contributor

deads2k commented Nov 25, 2015

I think all three test files have will have similar comments. Update them all.

@stevekuznetsov stevekuznetsov force-pushed the skuznets/ldap-testing branch 3 times, most recently from 27a32b7 to 5fc972d Compare November 25, 2015 22:55
@stevekuznetsov
Copy link
Contributor Author

@deads2k addressed comments

"github.com/openshift/origin/pkg/auth/ldaputil/testclient"
)

func newTestADLDAPInterface(client ldap.Client) *ADLDAPInterface {
Copy link
Contributor

Choose a reason for hiding this comment

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

When you're making things like this in the future. BaseDN is likely to be a string that test needs to know about and might care to change. Take it as an argument. Same one could be passed to newUser and the proper DN could be built. This is good enough (with newTestUser) that I'm willing to merge it.

@deads2k
Copy link
Contributor

deads2k commented Nov 30, 2015

lgtm [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4182/) (Image: devenv-rhel7_2834)

@deads2k
Copy link
Contributor

deads2k commented Nov 30, 2015

Looks unrelated. @stevekuznetsov check for flake issue: https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4180/

re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 5fc972d

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 5fc972d

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7531/)

@stevekuznetsov
Copy link
Contributor Author

#6065 again

openshift-bot pushed a commit that referenced this pull request Nov 30, 2015
@openshift-bot openshift-bot merged commit a004d9c into openshift:master Nov 30, 2015
@deads2k
Copy link
Contributor

deads2k commented Nov 30, 2015

@stevekuznetsov which one is next

@stevekuznetsov stevekuznetsov deleted the skuznets/ldap-testing branch January 8, 2016 15:23
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.

None yet

4 participants