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

add ModifyDN() - moddn/modrdn #54

Closed
wants to merge 17 commits into from
Closed

Conversation

vetinari
Copy link
Contributor

@vetinari vetinari commented Feb 21, 2016

fixes #63
This implements the ModifyDNRequest from RFC 4511

@vetinari
Copy link
Contributor Author

vetinari commented Jun 8, 2016

@liggitt what about merging this one and #62? This PR is already running in our local version in production

moddn.go Outdated
return nil
}

// vim: ts=4 sw=4 noexpandtab nolist
Copy link
Contributor

Choose a reason for hiding this comment

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

drop this line

@liggitt
Copy link
Contributor

liggitt commented Jun 8, 2016

just a few comments, then LGTM, thanks

@jamesboswell
Copy link

+1 nice job @vetinari !

vetinari added a commit to vetinari/ldap that referenced this pull request Aug 4, 2016
* Parse() wraps Unmarshal() to parse a string
* Apply() sends an *LDIF to the given ldap.Client to modify
  the server
... changes for go-ldap#54 already
included by commented
Copy link

@jamesboswell jamesboswell left a comment

Choose a reason for hiding this comment

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

@vetinari great work. Some comments on examples for any newcomers to understand how the new functions work.

Is it feasible to have tests?

@liggitt v2 or v3? It would be nice to have modrdn functionality. I don't see this as breaking in anyways, so v2 point release?

moddn.go Outdated
// To move an object in the tree, set the "newSup" to the new parent entry DN. Use an
// empty string for just changing the object's RDN.
//
// For moving the object without renaming the "rdn" must be the first

Choose a reason for hiding this comment

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

Example for moving without renaming? Also a comma "without renaming, the"

// mdnReq := NewModifyDNRequest("uid=someone,dc=example,dc=org", "uid=newname", true, "")
// will setup the request to just rename uid=someone,dc=example,dc=org to
// uid=newname,dc=example,dc=org.
func NewModifyDNRequest(dn string, rdn string, delOld bool, newSup string) *ModifyDNRequest {

Choose a reason for hiding this comment

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

For future documentation clarity (godoc) it wouldn't hurt to have 3 short examples:

  1. Rename without move
  2. Rename with move
  3. Move only

Or just actual Example functions in a moddn_test.go file? (https://blog.golang.org/examples)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made an moddn_test.go with the 3 examples

@iavael
Copy link

iavael commented Nov 3, 2016

Hi. Is any progress expected in this PR? Looking forward for this feature.

@oldCoderException
Copy link

Awesome to see this. Good work! When will it be released? I'm struggling with RDN renaming (login change) right now.

@oldCoderException
Copy link

Perhaps somehow I'm viewing an older version or something, but on line 64 of moddn.go, shouldn't l.nextMessageID be the function call l.nextMessageID() ??
As it is I got a panic: Invalid type func() int64, expected {u|}int{64|32|16|8}
from gopkg.in/asn1-ber.v1/ber.go:491 +0x244
Changing to the function call resulted in success.
Again, thanks for this. Very needed function in LDAP! Excellent work.

@vetinari
Copy link
Contributor Author

@oldCoderException right, the () were missing. probably happened while porting my production tree to this one

@oldCoderException
Copy link

Don't mean to be a pain but any idea when this makes it into the baseline? We've been using it in production for a few months now.

Thanks!

@johnweldon
Copy link
Member

I think I'm ready to merge this @liggitt unless you have any concerns?

@liggitt
Copy link
Contributor

liggitt commented Sep 30, 2017

No concerns, though I haven't tried the function against a real server

@jamesboswell
Copy link

jamesboswell commented Jan 8, 2018

I recently tested this against production LDAP using my fork which cherry-picks this PR.

I needed to migrate many hundreds of users uid to a new login schema (pass-through to AD). It is a component of the DN in this environment. @vetinari moddn PR worked perfectly.

@vetinari FYI I had to drop your README modifications to this PR into my fork, you need to update it to make it merge ready

@johnweldon @liggitt any chance this can be merged at some point before it's 2 year anniversary?

edit: to link to moddn branch in my fork

@vb4life
Copy link

vb4life commented Feb 9, 2018

I would LOVE to have this feature merged, since I'm developing a web-based user management system using go, and renaming a user is quite common.

@liggitt
Copy link
Contributor

liggitt commented Feb 12, 2018

rebased as #149

@liggitt liggitt closed this Feb 12, 2018
@liggitt
Copy link
Contributor

liggitt commented Feb 12, 2018

merged in #149

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.

Missing ModDN / ModRDN functionality
7 participants