-
Notifications
You must be signed in to change notification settings - Fork 200
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
feat(tenants): initial tenants module #932
feat(tenants): initial tenants module #932
Conversation
7de4de4
to
167e67a
Compare
167e67a
to
8a84a6e
Compare
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
8a84a6e
to
a1613b1
Compare
Codecov Report
@@ Coverage Diff @@
## 0.3.0-pre #932 +/- ##
=============================================
+ Coverage 86.70% 86.91% +0.21%
=============================================
Files 518 531 +13
Lines 12603 12854 +251
Branches 2137 2181 +44
=============================================
+ Hits 10927 11172 +245
- Misses 1581 1587 +6
Partials 95 95
Continue to review full report at Codecov.
|
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.
Nice work! Looks good to me. I left two comments, but those are minor things.
Additionally, maybe it's nice to add methods for creating and getting multiple tenants. E.g. createTenants([{...}, {...}, {...}, {...}])
. Maybe that is too difficult regarding sessions management tho. Idk, just a thought.
We could. Do you think it will be an often used use case to create a lot of tenants at the same time? I'd like to keep the API minimal, so if you have a good use case for it, I'd be happy to add it. |
So I can't think of a good use case for creating multiple tenants atm, but I can imagine wanting to execute some kind of action in batch for multiple tenants at the same time (e.g. creating a connection invite). In this case a |
Ok, I've made some changes to these method in a next PR. Let me look at it in that branch and see If I can come up with nice API |
Merging as only failing test is the one from #923 |
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Initial version of the tenants module. This is not fully finished yet and ready for prime time, but it contains most of the things needed for multi-tenancy. It doesn't introduce any breaking changes (those were addressed in the PRs this PR depends on).
Dependant on:
Until the modularization is fully here the API for registering module is not really nice, but I see that as a separate task that we can address after multi-tenancy has been merged. This is how you use it:
I would like to already merge so we can keep PRs reasonable (this is already getting quite large).
Basically the only thing that's left to do is correctly managing the tenant lifecycle and managing tenant sessions (see https://hackmd.io/vGLVlxLvQR6jsEEjzNcL8g?view#Tenant-Lifecycle). Currently, wallets are being opened when needed and will not be closed afterwards. That means over time the number of open wallets keeps increasing. Correctly opening wallets and avoiding race conditions if two sessions would like to open the same wallet is handled (using
async-mutex
library). This does need some improvements in not waiting indefinitely.Concrete what's still todo:
getTenantAgent
with no proper way to dispose of the agent (session count won't be reduced)I'm quite happy with how it turned out and how multi-tenancy is an optional addon without having the code in the core package.