-
Notifications
You must be signed in to change notification settings - Fork 232
SQLite3, MySQL, Postgres Support #94
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
Conversation
I would really appreciate to see that in the official release! Maybe mention @benyanke 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
@Fusion hey, thx for all the work. I am trying to get some PRs merged and tag a release asap. Your PR adds ~1k lines of code. While they look good I'd like to defer merging them after the next release. Also I plan to refactor the code into reusable packages: see #100 for how that will look like. Can I ask you to split your PR into a dedicated one for every backend? Then we can get them in one by one. I admit I personally don't like the cgo dependency, we can reevaluate that after mysql and postgres are merged. That way we can see when travis starts crying ;-) To be honest: I don't want to maintain sql backends, because it seems to me that a proper LDAP server makes more sense in those scenarios. Now, don't let that stop you. I see the maintainer role as getting out releases on a regular base and reviewing some core PRs. For the time being core seems to be the config and ldap backends. So, I plan to release with bugs, as long as the config and ldap backends work correctly. I am aware of the favor of other people for this. Just keep the 'lightweight' in mind. I think we should try to gradually build a better tool. Let me know if that works for you! |
Sure, I get your philosophy. |
How close are we to getting this into the main version? Lack of mysql support is the reason I didn't want to deploy this yet because I want to integrate everything with mysql. |
@hongkongkiwi I recently got around to trying to modularize the code, as previously discussed. Unfortunately, the main code has been heavily refactored since I created this pull request. I think I am now going to have to study the latest version of GLauth's code and rewrite the my SQL code. I am trying to figure out which is the least painful path, forwarding and merging wise. |
@butonic New pull request: #133 @hongkongkiwi Hopefully this brings you some joy :) |
I am extremely joyous! Thank you!! |
UPDATE: Well, I added support for MySQL and Postgres. Hopefully this make needing CGO a bit more palatable!
Hi,
Apologies for the quality as I am not a Go programmer (yet) and some aspects of Go are surprising to me.
Anyway, this is a SQLite backend, with the limitations described in the header I added to the top of 'sqlitebackend.go`
I did not tag it as 'WIP' because I would like a review, but feel free to consider it as such because:
a) it's not that great
b) you may have a few suggestions for me
c) you may not agree with file naming or content, etc.
Now, this is my second attempt because I did not realize that, by requiring the SQLite library, I was creating a CGO dependency. This means that this additional feature is adding a lot of weight to the build process: Travis takes more than 10 minutes to build and you will see in the Makefile that we have to drag lots of dependencies in to build various targets. This means, too, that these targets are not directly buildable on their own platform: building for Darwin works better when running on Linux(!) -- this could be avoided by creating cross-compile targets vs native targets.
And, yes, we have to juggle the cross-compilation packages because they are not compatible, especially the 'multilib' ones which mutually uninstall; and that is why we have to add package management to the Makefile itself.
Regards