rax-maas / dreadnot

deploy without dread
Apache License 2.0
630 stars 61 forks source link

Implement LDAP authentication, remove node_modules from source control #66

Closed martintajur closed 8 years ago

martintajur commented 9 years ago
ksheedlo commented 9 years ago

@martintajur Thanks for the PR!

As you might be able to tell, this repo isn't maintained very closely at the moment. But as luck would have it, I'm also interested in running dreadnot under a recent Node.js, and know where the original creator/maintainers are and can make them pay attention. So I want to help you at least get some parts of this merged.

(dreadnot surely does work under node 0.10.x)

I sure hope so. In fact, I'm working on tests to prove it. The problem is, I can't stake my team's (and others') production deployments on this assurance when the code base has no tests and explicitly specifies itself not to work with node > 0.6. I don't need 100% coverage, but I do need the interesting bits. I'm not asking or expecting you to help out with that specifically, but it would be possible once I get the initial test harness set up. Just sayin'.

node_modules are removed from source control

Normally I don't like to put them in source control either, but there's a reason for doing it this way. I suggest treating them as an existing convention and not trying to make sweeping changes in one PR. Leave the node_modules as they are; and if you want to add new dependencies in your PR just go ahead and vendor them in there with the other ones.

Implentation of LDAP auth

Nonetheless, this is impressive. It's not something we've done before, and I'll have to talk to some Rackers to find out if this feature is going to be a fit for our project, but any kind of federated login scares me. Even with a module. Sheesh.

The only other minor nitpick I have is that this PR has merge commits cluttering up the history, and it would be really great if you could squash them away.

martintajur commented 9 years ago

All comments are valid. Thank you for the explanations. I will update the PR to only have the LDAP auth functionality be in it, along with the abtraction of different authentication providers' logic. Stay tuned.