Skip to content

Multi database support#136

Merged
paulcsmith merged 4 commits intomasterfrom
pcs/9-multi-repo-support
Jul 9, 2019
Merged

Multi database support#136
paulcsmith merged 4 commits intomasterfrom
pcs/9-multi-repo-support

Conversation

@paulcsmith
Copy link
Member

@paulcsmith paulcsmith commented Jul 6, 2019 •
edited
Loading

Closes #9

@paulcsmith
Rename Repo -> Database
76b1a5a 
Because it makes more sense. Let's not over-abstract things with weird
terms :)
@paulcsmith paulcsmith force-pushed the pcs/9-multi-repo-support branch 2 times, most recently from e2897e7 to 4f88731 Compare July 9, 2019 03:45
@paulcsmith paulcsmith requested a review from jwoertink July 9, 2019 03:47
@paulcsmith
Copy link
Member Author

paulcsmith commented Jul 9, 2019

@jwoertink this now allows configuring multiple databases and allows configuring what database each model uses. Does this look like it would work for your needs?

paulcsmith
paulcsmith commented Jul 9, 2019
View reviewed changes
Copy link
Member Author

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs work, but hoping this shows what can be done

config/database.cr
database_name = "avram_dev"

Avram::Repo.configure do |settings|
class TestDatabase < Avram::Database
Copy link
Member Author

@paulcsmith paulcsmith Jul 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is how to create a new Database

config/database.cr
class TestDatabase < Avram::Database
end

TestDatabase.configure do |settings|
Copy link
Member Author

@paulcsmith paulcsmith Jul 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Configure databases

spec/support/base_model.cr
class BaseModel < Avram::Model
def self.database
TestDatabase
end
Copy link
Member Author

@paulcsmith paulcsmith Jul 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is how you set what database a model uses. Can be done on the abstract class or could be overridden per model

@paulcsmith paulcsmith force-pushed the pcs/9-multi-repo-support branch from 4f88731 to b2bf1ac Compare July 9, 2019 04:21
jwoertink
jwoertink reviewed Jul 9, 2019
View reviewed changes
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty amazing! The only thing I'm wondering is maybe there should be a spec that actually connects to another DB? It would mean setting up a secondary one, but it could have just a single table so we can verify that we can query each DB at the same time.

Aside from that, it all looks great. I can't wait to try this out.

@paulcsmith
Copy link
Member Author

paulcsmith commented Jul 9, 2019

Yeah I need to add some more specs and a few other things but wanted to make sure this would actually address the problem. Looks like it will so I'll finish it up so we can get you guys on Avram :D

@paulcsmith paulcsmith force-pushed the pcs/9-multi-repo-support branch from 88e97e7 to 9d5cf3e Compare July 9, 2019 18:46
@paulcsmith paulcsmith changed the title [WIP] Multi database support Multi database support Jul 9, 2019
@paulcsmith
Copy link
Member Author

paulcsmith commented Jul 9, 2019

Spec added. Ready for another review!

@paulcsmith paulcsmith mentioned this pull request Jul 9, 2019
Open
jwoertink
jwoertink reviewed Jul 9, 2019
View reviewed changes
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet! Super close. Just a few comments.

spec/multi_database_spec.cr
#
# If does not raise an error then that means it is using the good connection,
# which is not what we configured
expect_raises Avram::ConnectionError do
Copy link
Member

@jwoertink jwoertink Jul 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this raise an exception error message letting us know which one it wasn't able to connect to? Our app could have up to 4 PG connections technically (a bit absurd, I know), so if for some reason one of them didn't connection, it would be nice to see Unable to connect with DatabaseWithIncorrectSettings or something.

Copy link
Member Author

@paulcsmith paulcsmith Jul 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes great point!

@paulcsmith paulcsmith force-pushed the pcs/9-multi-repo-support branch from 88bc48d to f26c304 Compare July 9, 2019 21:05
@paulcsmith
Copy link
Member Author

paulcsmith commented Jul 9, 2019

This will require changes to the guides and also will need changes in Lucky CLI to generate a default database. I think we'll call it AppDatabase unless you've got other ideas

@paulcsmith paulcsmith force-pushed the pcs/9-multi-repo-support branch from f26c304 to 88e05c3 Compare July 9, 2019 21:09
@paulcsmith paulcsmith merged commit 4a38272 into master Jul 9, 2019
@paulcsmith paulcsmith deleted the pcs/9-multi-repo-support branch July 9, 2019 21:14
paulcsmith added a commit to luckyframework/lucky_cli that referenced this pull request Jul 9, 2019
@paulcsmith
Use new database classes
cbcd133 
Introduced in luckyframework/avram#136
@paulcsmith paulcsmith mentioned this pull request Jul 9, 2019
Merged
paulcsmith added a commit to luckyframework/lucky that referenced this pull request Jul 9, 2019
@paulcsmith
Use new database classes
c901b66 
Introduced in luckyframework/avram#136
@paulcsmith paulcsmith mentioned this pull request Jul 9, 2019
Merged
paulcsmith added a commit to luckyframework/lucky that referenced this pull request Jul 9, 2019
@paulcsmith
Use new database classes (#848)
0140eb2 
Introduced in luckyframework/avram#136
paulcsmith added a commit to luckyframework/lucky that referenced this pull request Jul 9, 2019
@paulcsmith
Use new database classes
8b4f798 
Introduced in luckyframework/avram#136
paulcsmith added a commit to luckyframework/lucky_cli that referenced this pull request Jul 9, 2019
@paulcsmith
Use new database classes
2719741 
Introduced in luckyframework/avram#136
paulcsmith added a commit to luckyframework/lucky_cli that referenced this pull request Jul 9, 2019
@paulcsmith
Use new database classes
bad8cc1 
Introduced in luckyframework/avram#136
paulcsmith added a commit to luckyframework/lucky_cli that referenced this pull request Jul 9, 2019
@paulcsmith
Use new database classes
1fd4d7d 
Introduced in luckyframework/avram#136
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jwoertink jwoertink jwoertink left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Multi-database support

2 participants

@paulcsmith @jwoertink