search

AdRoll / hologram

Easy, painless AWS credentials on developer laptops.
Apache License 2.0
803 stars 42 forks source link

Add persistent LDAP layer #60

Closed copumpkin closed 8 years ago

copumpkin commented 9 years ago

Before this, Hologram dialed LDAP once and if that connection died, it would silently fail. This commit adds a new type that implements the same interface but pings behind the scenes and reconnects if needed.

Fixes #53.

copumpkin commented 9 years ago

Ping @frangarciam, @zylad, or anyone else who's watching. Is there something I can do to make merging this easier?

gaylatea commented 9 years ago

I think both Fran and Dominik have left AdRoll, as did I in February. I sadly no longer have commit access to this repository and cannot do much to help here.

I've been a bit out of sorts lately, and haven't kept up with the pace of Hologram development; I've forked this repo to https://github.com/sanctuarygroup/hologram so that development can continue in that light and I can at least help there.

Can you please replicate this Pull Request there, and I'll provide commentary?

copumpkin commented 9 years ago

Aha, I hadn't realized that the project had moved! I'll move the PR over there this evening, thanks! There's another PR here that should probably be notified, and the issue tracker is disabled in the new location.

gaylatea commented 9 years ago

Nice catch, thank you. I've added the Issue Tracker there for you.

walterking commented 9 years ago

We're still supporting this at adroll.

@copumpkin why ping every time instead of reconnect on failure?

copumpkin commented 9 years ago

@walterking no real reason, and your way sounds better, thanks. I'll revise the PR to do that instead. Any other comments while I'm at it?

walterking commented 9 years ago

that was the only thing that stood out to me

copumpkin commented 8 years ago

@walterking sorry for the long delay, but I think I've addressed the point you made in this latest commit. Basically switched the logic from "unconditional ping -> potentially reconnect -> run operation" to "run operation -> potentially reconnect -> potentially retry operation". Tests still pass and it simplified things a bit (like all the Ping stuff is now gone).

walterking commented 8 years ago

The code looks good to me. Just want to build/test to verify first

adriandoolittle commented 8 years ago

Looks good, thanks. The build & tests passed.