Cleanup and more #1
Conversation
app/models/landlord_record.rb
Outdated
| @@ -26,4 +28,8 @@ def fetch_db_connections | |||
|
|
|||
| connections | |||
| end | |||
|
|
|||
| # TODO: originally this should go in ApplicationRecord. By extracting this functionality (see tenant.rb:19) into this class to share across models of this gem, this doesn't work. | |||
| # why we need it? | |||
There was a problem hiding this comment.
question for you @nazamoresco
There was a problem hiding this comment.
This creates the connections pool for all the available tenants.
We have to fetch the connections from the database and do this at some point.
By doing it in the ApplicationRecord class evaluation I was sure that when a record that needed access to the database was accessed we will had created this connection pools.
Why this is failing?
I can't say for sure but I have a couple of ideas we can follow, the main one being this:
- We have commented out the
# fetch_db_connectionsinstruction, this will for sure make this fail. But maybe you try with this in and it still failed.
There was a problem hiding this comment.
gotcha. gonna try a different naming probably!
app/models/landlord_record.rb
Outdated
|
|
||
| # TODO: originally this should go in ApplicationRecord. By extracting this functionality (see tenant.rb:19) into this class to share across models of this gem, this doesn't work. | ||
| # why we need it? | ||
| # fetch_db_connections | ||
| end |
There was a problem hiding this comment.
the idea behind creating this class is that the other classes rely too much on ApplicationRecord. It's true that all apps should have an ApplicationRecord, but the reality is that it's not mandatory. By creating this intermediate class we can add stuff that both Tenant and TenantConnection use.
| unless Rails.env.test? | ||
| pid = File.read(".pumactl_pid").to_i | ||
| Process.kill("SIGUSR2", pid) | ||
| end |
There was a problem hiding this comment.
we should think about this thing.
There was a problem hiding this comment.
You mean the hot restart? 👍
There was a problem hiding this comment.
yeah, I mean, not everyone uses puma, not everyone's aware of the pidfile, etc. normally there'a a pidfile within tmp/pids. We should think about that mostly
There was a problem hiding this comment.
This is specifically design to work with Puma, I think is a good starting point.
It could be extended after to work with other web servers but I think just puma is ok for now.
Around the pidfile it could be configurable, we could assume it is in temp/pids and allow users to set it in a initialization file otherwise!
lib/landlord/tasks/landlord.rake
Outdated
| end | ||
|
|
||
| # Handles mismatches between env db and tenant db and returns if this should be run again | ||
| def check_env_matching_db_configuration!(env_config, tenancy_config) |
There was a problem hiding this comment.
we should review this thing shouldn't we?
app/models/landlord_record.rb
Outdated
| @@ -26,4 +28,8 @@ def fetch_db_connections | |||
|
|
|||
| connections | |||
| end | |||
|
|
|||
| # TODO: originally this should go in ApplicationRecord. By extracting this functionality (see tenant.rb:19) into this class to share across models of this gem, this doesn't work. | |||
| # why we need it? | |||
There was a problem hiding this comment.
This creates the connections pool for all the available tenants.
We have to fetch the connections from the database and do this at some point.
By doing it in the ApplicationRecord class evaluation I was sure that when a record that needed access to the database was accessed we will had created this connection pools.
Why this is failing?
I can't say for sure but I have a couple of ideas we can follow, the main one being this:
- We have commented out the
# fetch_db_connectionsinstruction, this will for sure make this fail. But maybe you try with this in and it still failed.
app/models/landlord_record.rb
Outdated
|
|
||
| # TODO: originally this should go in ApplicationRecord. By extracting this functionality (see tenant.rb:19) into this class to share across models of this gem, this doesn't work. | ||
| # why we need it? | ||
| # fetch_db_connections | ||
| end |
| unless Rails.env.test? | ||
| pid = File.read(".pumactl_pid").to_i | ||
| Process.kill("SIGUSR2", pid) | ||
| end |
There was a problem hiding this comment.
You mean the hot restart? 👍
|
|
||
| fetch_db_connections | ||
| STR | ||
| end |
There was a problem hiding this comment.
nicer installation :)
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
moved into a concern. it's automatically included in ApplicationRecord and also Tenant.
patriciomacadden
force-pushed
the
cleanup-and-more
branch
from
9d81702
to
f9a03ca
Compare
bundle gemto generate all of the blueprint code that bundle gem generates.