Skip to content
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 rbenv/rvm and capistrano-db-tasks #99

Merged
merged 1 commit into from
Nov 29, 2016

Conversation

elthariel
Copy link
Contributor

As mentionned here: capistrano/sshkit#153 (comment) and capistrano/sshkit#24 (comment) by one of Capistrano's maintainer, the run_locally feature is not a very good idea and shouldn't be used.

This PR removes the use of it in Initializing Database::Local, which is broken when used in conjunction with capistrano-rvm/rbenv (it'll use the sshkit configs for the rails command which are set up to use the remote rbenv/rvm config)

@elthariel
Copy link
Contributor Author

Oops, this PR includes the commit from my other PR. I can remove it if for any reason you can't / doesn't want to merge the previous PR.

config_content = dirty_config_content.match(/#{DBCONFIG_BEGIN_FLAG}(.*?)#{DBCONFIG_END_FLAG}/m)[1]
@config = YAML.load(config_content).inject({}) { |h, (k, v)| h[k.to_s] = v; h }
puts "Loading local database config"
@config = YAML.load(ERB.new(File.read(File.join('config', 'database.yml'))).result)[fetch(:local_rails_env).to_s]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's bad idea to use config/database.yml instead of ActiveRecord::Base.connection.instance_variable_get(:@config). Check PR #54 #70 #46.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum. Great point.

@elthariel
Copy link
Contributor Author

I've reverted to using ActiveRecord::Base.connection.instance_variable_get(:@config), but using Open3 instead of the broken run_locally

@elthariel
Copy link
Contributor Author

ping @numbata

end
# Remove all warnings, errors and artefacts produced by bunlder, rails and other useful tools
config_content = dirty_config_content.match(/#{DBCONFIG_BEGIN_FLAG}(.*?)#{DBCONFIG_END_FLAG}/m)[1]
puts "Loading local database config"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@cap.info ?

@@ -6,8 +6,11 @@ def remote_to_local(cap)
server = servers.detect { |s| s.roles.include?(:app) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, remove commit from other PR.


command = "#{Dir.pwd}/bin/rails runner \"puts '#{DBCONFIG_BEGIN_FLAG}' + ActiveRecord::Base.connection.instance_variable_get(:@config).to_yaml + '#{DBCONFIG_END_FLAG}'\""
stdout, status = Open3.capture2(command)
raise "Error running command (status=#{status}): #{cmd}" if status != 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, fix variable name: #{cmd} should be #{command}.

@numbata
Copy link
Collaborator

numbata commented Nov 23, 2016

ping @elthariel

@numbata
Copy link
Collaborator

numbata commented Nov 28, 2016

@elthariel is it actual for you? I want to push new version with this fix.

@elthariel
Copy link
Contributor Author

@numbata Sorry for the delay.

I fixed what your comments sugested and re-tested in the project I use it, we're good to go

@numbata
Copy link
Collaborator

numbata commented Nov 29, 2016

@elthariel thanks a lot! 👍

@numbata numbata merged commit 5cd7193 into sgruhier:master Nov 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants