-
-
Notifications
You must be signed in to change notification settings - Fork 225
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
Modularize Managers #57
Conversation
I think that the coverage decrease isn't a true decrease, but an artifact of splitting up the code. However, I'll work on that. There are definitely some important cases not being covered (now or before). |
Thank you for your work on this. As you already noticed, this is a quite complex PR with a lot of code changes that will introduce some major breaks in the API. There are some really good ideas, I need some time to review this and decide what to do next. |
What's the reasoning of introducing packages like policies and access? It is best practice to have as few packages as possible in Go so you need less import statements and fewer exports. |
What is the purpose of moving the different managers to subpackages? go get is still going to fetch all of the dependencies, right? The only way to avoid this is to move them to a completely different package |
@arekkas Thanks for any time you have to take a look at what I've done. (Certainly anything that gets merged upstream make it easier to maintain my fork.) I started off with the goal of minimizing dependencies. Yes, For example, if you were to just use Redis, you could avoid having to require As far as separating out
P.S. I'm currently writing tests for MySQL that use the |
You can see in the glide manifest file documentation (http://glide.readthedocs.io/en/latest/glide.yaml/) the description of subpackages: As implied, unused subpackages (and therefore their dependencies) won't get downloaded on |
I see, I didn't know that the core team is working on something that mimics the behaviour of glide. In that case, moving the managers to their own packages makes more sense. In general, I don't have a problem with fetching a few extra dependencies in my code. From quickly looking over your branch, here are my take aways:
In general, I want to support SQL/memory officially and all other adapters like rethink or redis as a community effort. One way to go along with fewer breaks could be to replace sqlx with the standard One more thing as a takeaway, because ladon used to have a similar structure you are suggesting, but was unified later on to be more usable:
|
Thanks for all the feedback! Okay, so first big thing: Yeah, I think I can pretty easily put access and policy back into the ladon package. Looking back at it now, I'm pretty sure the import loop problem really only stemmed from Speaking of integration tests, I can start changing out sqlx for plain sql. Does this mean you'd also like to not depend on sql-migrate? Agreed on logging. Returning errors is always the best decision for a library. I haven't thought about this much yet, but what if we were to add one more method to the So I'm not sure what you're referring to with the connection pools. I haven't introduced any new pools. Also, going back to the top of your comment, in regards to fetching dependencies, you're absolutely right that grabbing a few extra is relatively fast. But here's something to consider: because packages are hosted in a distributed manner and git history is mutable, just having a lock file (such as glide provides) isn't really enough to guarantee you'll always be able to build. Someone could easily take their project down or screw up a few commits (maybe squashing them). That's why my company and some others I know clone all dependencies and host them privately. Glide even makes it easy to use them:
But now you can see how dependencies suddenly become more of a hassle. So anyway, I'll make these changes (might not get to them until the weekend) and hopefully they'll reduce the complexity of this PR a bunch. Then let me know what you think, and hopefully it's closer to something you'd like to integrate. :) At the very least, it should make it easier to create an |
Yes, please use ladon_test if possible - that makes a lot of sense.
No, sql-migrate is very important. I also want to walk back on the statement. Let's keep sqlx and try moving the sql manager to a different package (not sure about the naming though, I don't like
I don't think that's neccessary - let's keep this simple and remove logrus!
Yes, this is true. With Go there's always the trade-off between lock files and commiting deps for guaranteed reproducible builds. It happened to me once that someone deleted a library from github, so the lock file didn't help. In that sense keeping deps to a minimum is good practice. What has to be understood though is that there is a very serious trade-off between moving forward with fewer dependencies and breaking compatibility - which moving the managers to a new location is. Ladon is seeing a very steady increase amount of clones (currently 300 uniques per week) and while not all of those are going to use ladon in a production system, it will break stuff.
Let's consider this (I only briefly checked the code, so it's possible that I overlooked something):
We now have two connetion (pools), one encapsulated in NewManager and one that is used by our other code. Instead we should do something like this:
Now we have one connection pool that we need to take care of, like closing it, handling errors, listening on events, ... Does that make it clearer? |
Before you start implementing something, please lay out your plan in writing (bullet points are enough) here so I have something that I can think about. Otherwise we'll probably end up with another discussion and a lot of code being done, that maybe needs changes again :) |
Got it on the sql-migrate. Got it on removing logrus. Got it on the minimizing breaking changes. I understand now what you mean by connections. You're correct that this does make them harder to manage. I hadn't thought of that. Currently there is no way to tell from the errors a manager can return if that means the backend is down (and whether/how long to wait for it to reconnect). Therefore you're going to need the database connection to ping to diagnose errors. So looking back at all my changes (and some yet uncommitted), a lot of lessons. Most of those lessons are that my main goal can be achieved with very few, localized breaking changes. This PR should only be about those. Many of the other breaks were caused by trying to generalize manager instantiation. Now, this would be very cool if the goal were to create a ladon as a CLI instead of library (in fact I'm very nearly there in uncommitted code), however that was really accidental, as that's not a goal for either of us. Here's where I think I'd like to go next on this PR:
Then as a separate PRs:
And separate issues/discussions:
|
I'd just like to mention that I would very much appreciate not having to vendor redis/rethink/logrus etc, when I don't really need either of those. I also think that minimizing direct dependencies is a security win as well. I wouldn't mind breaking changes to get there and could also offer some limited help in testing. |
There are not a lot of reflective people on GitHub. Kudos for that :)
I'd love to have a ladon-cli package, or alternative
Sounds good
I like the way this is currently handled. How about having an
Sounds very good!
Ladon rethink or rethink in integration package? I merged the new rethink repo location in the common/integration package recently. |
Just checking in (no pressure) if you're still up to this? I'll release a larger feature soon ( #58 ) and thought of including the new manager setup too and just do one breaking release instead of two. |
Thanks for checking in. Yeah, I caught a cold, which slowed me down a bit. How immediately are you looking to release? This new plan shouldn't take very long to implement. Let me know when you want it this week and I'll do my best to have it ready. Also, thank you for taking the time to help me contribute. I really appreciate it. I've seen so many different opinions from maintainers on how they like to receive contributions - from PRs only (code shows dedication and issues are too big of a hassle) to issues first (PRs are too aggressive and maintainers feel bad rejecting them if they don't fit the direction they want to take the project). So thanks for rolling with my crazy first draft. I'll do those 6 steps as laid out, except with integration tests as a subpackage, as quickly as possible. I'll push commits as I go so you can see the progress. |
Don't worry, I'll ping you 3-4 days prior to release |
Oh and get well soon! :) |
I went ahead and implemented this in #58 |
This pull request was originally meant to just modularize the manager system so that you could import ladon without also having to depend on rethink/redis/sqlx drivers if you weren't using them. (My company is considering building an internal system based on Ladon and we would like this feature.) However, it got somewhat more complex. It's certainly possible that this giant PR can be simplified into a few smaller ones, so consider it a work in progress and let me know which ideas/changes you like.
Big things this PR does:
manager
subpackagemanager.DefaultManagers
so you can invoke them as if they were drivers (after underscore-importing them).access
subpackagepolicy
subpackage (this could be moved intoaccess
)log
subpackage to make logging pluggableIt currently breaks pre-Go 1.8 compatibility due to usingFIXEDsqlx.ConnectContext
, but this can be fixed.ory/common/integration
to get around some issues.