-
Notifications
You must be signed in to change notification settings - Fork 81
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
Fix db:create rake tasks #145
Conversation
I'm also running into this issue when trying to upgrade to the latest 1.0 version of the gem. I can't issue a rake sequel:drop , rake sequel:create and have it just work. Now it is stuck with an error saying that the database needs to exist. I was able to get around these issues by adding test: false as a property to the database.yml. This reverts the connection behavior to sequel 4.x which will NOT attempt to connect to the database by default. |
@JonathanTron Could you prioritize merging this as an emergency patch? 1.0 of the gem effectively makes it impossible to use a new Rails app (no schema) at this time because of the attempt to connect prior to db rake tasks. I've helped document this issue along with @mofmn in this issue. |
@@ -68,7 +68,7 @@ class Railtie < Rails::Railtie | |||
end | |||
|
|||
initializer 'sequel.connect' do |app| | |||
::SequelRails.setup ::Rails.env unless app.config.sequel[:skip_connect] | |||
::SequelRails.setup ::Rails.env unless app.config.sequel[:skip_connect] or $0 =~ /rake/ |
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.
It might be good to turn this into a method at this point. The conditional is becoming obtuse.
How about extracting a should_connect?
method with these two conditions.
Railtie also has a rake
hook where you could simply set app.config.sequel[:skip_connect]
to true perhaps. I pored over ActiveRecord's railtie to figure out how they're avoiding this issue and couldn't find anything obvious to copy and reuse here.
@rolftimmermans You have duplicated commits in this branch. Could you rebase and merge them so this PR can be merge more easily? |
@rolftimmermans @olivierlacan thanks! |
@rolftimmermans @JonathanTron I've grabbed the gem from master and somehow this didn't fix the issue for me. I'll try to step through with pry to see what's happening in the railtie. |
Figured it out. @rolftimmermans's patch assumes |
We don't want to connect the database when it doesn't exist yet which should be the case during the rake/rails db:create task. We also can't assume db:create is run with the command rake db:create since Rails now proxies rails db:create into rake db:create This should solve unresolved problems with TalentBox#145 and fixes TalentBox#146 but I need to check tests to ensure that no regression occurs due the changes.
Correct, I ran into the same thing. I now have the equivalent of this code:
That seems to work fine. So presumably we can use something like this instead of this PR:
|
@rolftimmermans See #148. I think we can simply set I couldn't think of a reason why we'd have to avoid a connection attempt to the database than when we're trying to create it, and maybe drop it. I could clearly be forgetting something, but for me #148 fixes my |
When running
rake db:create
with a default sequel-rails installation (testing this with Rails 5.1.4), the following exception occurs:This happens because by default a connection is made to the configured database. This patch checks to see if we are running inside a Rake tasks and does not automatically connect if we do.
The Rake tasks themselves then call
db_for_current_env
if they need a connection to the database for the current environment. This only sets up a database connection in Rake tasks as needed.This solves the exception on
db:create
for us and others remain working.