-
Notifications
You must be signed in to change notification settings - Fork 16
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
Integrate with make_osc & deploy on osm-database #541
Integrate with make_osc & deploy on osm-database #541
Conversation
5ddb5d6
to
3a6c0dd
Compare
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.
Found some minor things otherwise lgtm and good work to convert it into a Rake Task
lib/tasks/make_osc.rake
Outdated
require 'pg' | ||
require 'builder' | ||
|
||
$fields = [ 'shop', 'office', 'aerialway', 'aeroway', 'amenity', 'tourism', 'historic', 'sport', 'leisure', 'public_transport' ] |
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.
Is there a reason this has to be a global variable? I'm asking because of the $
.
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.
I don't know. I didn't write this code.
lib/tasks/make_osc.rake
Outdated
namespace :make_osc do | ||
desc 'Make OSC files.' | ||
task :full => :environment do | ||
$conn = PGconn.open(:dbname => 'osm', :user => 'osm', :password => 'osm', :host => 'osm-database') |
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.
Same as above? Does this one have to be global?
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.
I don't know. I didn't write this code.
lib/tasks/make_osc.rake
Outdated
end | ||
|
||
task :diff => :environment do | ||
$conn = PGconn.open(:dbname => 'osm', :user => 'osm', :password => 'osm', :host => 'osm-database') |
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.
Same as above with $conn
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.
I don't know. I didn't write this code.
Gemfile
Outdated
@@ -3,6 +3,7 @@ source 'https://rubygems.org' | |||
group :default do | |||
gem 'rails', '~> 4.1.0' | |||
gem 'mysql2', '~> 0.3.18' | |||
gem 'pg', '~> 0.18.4' |
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.
The pg
gem requires some native extensions to be build on installation. For that it requires the Postgres dev packages. By having it in the :default
group we require all devs to have Postgres installed on their machines when they want to work on wheelmap.
I'd propose to have a separate group
for this we then can exclude during installation via bundle install --without
.
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.
Hm, this seems reasonable. I'll work on this.
We discussed removing the |
2925593
to
b692d2a
Compare
@schultyy Those should be resolved. Am testing... |
Requested changes made and work. |
I would consider stopping this effort: What's the gain compared to the effort invested and the dependencies introduces into the main app. I'd rather see this in a small standalone script with a gemfile that's separate from the main wheelmap app. |
@Xylakant So a separate repo which we also deploy via capistrano or? |
I'm uncertain if cap is not an overkill in that case. This will not see many updates, installing that on the machine via chef should be perfectly fine. |
@Xylakant Well, the only dependency we introduce is |
The use of a group is somewhat limited, people expect I agree that it should be committed somewhere - it can even live in a "tools" or "osm-sync" subfolder of this repo, but I don't think the effort of folding this into the rake task has any positive ROI either in time or reduced complexity. |
So my reasoning for making it a rake task was that it was called by a rake task already, so it seemed easy to integrate that way. I suppose it wouldn't be much harder to drop this in a subfolder and give it it's own gemfile then modify capistrano to have the dependencies installed when it is deployed. |
as I said - I don't think that installing it via CAP has any positive ROI either, but that's a minor point. I'd really go and move it to a dedicated repo and treat it like we treat any other dependency: We assume that it's available at the point where we install the app. |
Ok. |
Okay, this should be good to go. |
f97ad60
to
a6ef178
Compare
b794f12
to
5b2d883
Compare
Rebased on top of |
a6ef178
to
d1e9709
Compare
178af96
to
c369343
Compare
c369343
to
ca7d305
Compare
Rebased. |
There was an
make_osc.rb
andstring_hstore.rb
file on the deployed instances which was not in any of our repos. These are currently required for syncing between PostgreSQL and MySQL.This PR converts configures things to appropriately talk to it.