Skip to content

Heroku compatibility (WIP) #33

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.

Sign up for GitHub

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

Open
wrs wants to merge 4 commits into
base: master
Choose a base branch
from
+30 −8
Open

Heroku compatibility (WIP) #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
947ebc5
Use Puma web server
wrs Jan 26, 2013
e51b018
Specify JRuby version in Gemfile
wrs Jan 26, 2013
ee3a49a
Switch to https Git URLs in Gemfile
wrs Jan 26, 2013
cfa30e6
Respond to OPTIONS /api/1.0/notify
wrs Jan 18, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
source :rubygems
ruby '1.9.3', :engine => 'jruby', :engine_version => '1.7.2'
Copy link
Contributor

@RISCfuture RISCfuture Jan 30, 2013

Choose a reason for hiding this comment

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

This constrains everyone to using JRuby, correct? Right now Squash is confirmed to work on CRuby 1.9.3, and JRuby 1.7.2 with the --1.9 flag. I'd like to preserve compatibility with both of these platforms and allow for the possibility of others in the future.

Copy link
Author

@wrs wrs Jan 30, 2013

Choose a reason for hiding this comment

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

Yeah, I wasn't sure what to do about this. This seems to be how you tell Heroku to use the JRuby buildpack. Perhaps this should be a setup.rb question? Or perhaps there's another way to specify it and there should be a README.Heroku with an explanation.

Copy link
Contributor

@RISCfuture RISCfuture Jan 30, 2013

Choose a reason for hiding this comment

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

Does Heroku need to use the JRuby buildpack? Obviously Squash's threaded model is more ideal under JRuby, but we could still open it to user choice.

Copy link
Author

@wrs wrs Jan 31, 2013

Choose a reason for hiding this comment

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

Well, it wasn't clear to me whether it was tested with the MRI. If that works, then sure.

I'd like to somehow have an "optimized" Heroku config that's easy to set up, because I suspect a lot of people will want to use Heroku. The only actual requirement to make it work may well be the git -> https change in Gemfile. But there are these other choices that may have a "right" non-default answer if you're not an expert. Maybe it should all be encapsulated in setup.rb.

Copy link
Contributor

@RISCfuture RISCfuture Jan 31, 2013

Choose a reason for hiding this comment

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

Yes. The git -> http change can be global; I see no harm in that. If you can determine a minimal set of changes necessary to make Heroku deployment as painless as possible, we can wrap them up into setup.rb.


# SERVER
gem 'puma'
Copy link
Contributor

@RISCfuture RISCfuture Jan 30, 2013

Choose a reason for hiding this comment

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

Why are we specifying that everyone use Puma?

Copy link
Author

@wrs wrs Jan 30, 2013

Choose a reason for hiding this comment

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

It's from the Heroku JRuby instructions. (I know pretty much nothing about JRuby web server choices.) The default presumably works…will try taking this out.

Copy link
Contributor

@RISCfuture RISCfuture Jan 30, 2013

Choose a reason for hiding this comment

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

I'd rather not force everyone in the world to use Puma for the sake of the Heroku users :) If it can be done as part of the Heroku deploy process, that would be ideal; second-best option would be to ask the user if he intends to use Heroku as part of setup.rb, and add Puma to the Gemfile if he answers yes.

gem 'jruby-openssl', platform: :jruby
Copy link
Contributor

@RISCfuture RISCfuture Jan 30, 2013

Choose a reason for hiding this comment

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

The latest version of JRuby includes the OpenSSL library by default. Including the gem only gets you a bunch of "Already initialized constant" warnings.


# FRAMEWORK
gem 'rails', git: 'git://github.com/rails/rails.git', branch: '3-2-stable'
gem 'rails', git: 'https://github.com/rails/rails.git', branch: '3-2-stable'
# We need to use this branch of Rails because it includes fixes for ActiveRecord
# and concurrency that we need for our thread-spawning background job paradigm
# to work
Expand All @@ -11,14 +16,14 @@ gem 'rack-cors', require: 'rack/cors'
# MODELS
gem 'pg', platform: :mri
gem 'activerecord-jdbcpostgresql-adapter', platform: :jruby
gem 'has_metadata_column', git: 'git://github.com/RISCfuture/has_metadata_column.git'
gem 'has_metadata_column', git: 'https://github.com/RISCfuture/has_metadata_column.git'
gem 'slugalicious'
gem 'email_validation'
gem 'url_validation'
gem 'json_serialize'
gem 'validates_timeliness'
gem 'find_or_create_on_scopes', '>= 1.2.1'
gem 'composite_primary_keys', git: 'git://github.com/RISCfuture/composite_primary_keys.git'
gem 'composite_primary_keys', git: 'https://github.com/RISCfuture/composite_primary_keys.git'
gem 'activerecord-postgresql-cursors'

# VIEWS
Expand All @@ -28,7 +33,7 @@ gem 'kramdown'

# UTILITIES
gem 'json'
gem 'git', git: 'git://github.com/RISCfuture/ruby-git.git'
gem 'git', git: 'https://github.com/RISCfuture/ruby-git.git'
gem 'user-agent'

# AUTH
Expand Down
17 changes: 13 additions & 4 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,26 @@
GIT
remote: git://github.com/RISCfuture/composite_primary_keys.git
remote: https://github.com/RISCfuture/composite_primary_keys.git
revision: 4f624f3a431b5ad3fa2d80e58b118896c356dffe
specs:
composite_primary_keys (5.0.12)
activerecord (~> 3.2.0, >= 3.2.9)

GIT
remote: git://github.com/RISCfuture/has_metadata_column.git
remote: https://github.com/RISCfuture/has_metadata_column.git
revision: 2aebf034ab97365cfbf56073c24c3a28fe2dc4ae
specs:
has_metadata_column (1.0.3)
boolean
rails (>= 3.0)

GIT
remote: git://github.com/RISCfuture/ruby-git.git
remote: https://github.com/RISCfuture/ruby-git.git
revision: 473d8ecc1fd49cbfcbabff0061484319d4bac9fa
specs:
git (1.2.5)

GIT
remote: git://github.com/rails/rails.git
remote: https://github.com/rails/rails.git
revision: a3aca81b21f793e8869440e9e84ead80c2479e3d
branch: 3-2-stable
specs:
Expand Down Expand Up @@ -78,6 +78,7 @@ GEM
addressable (2.3.2)
arel (3.0.2)
boolean (1.0.1)
bouncy-castle-java (1.5.0146.1)
builder (3.0.4)
coffee-rails (3.2.2)
coffee-script (>= 2.2.0)
Expand Down Expand Up @@ -124,6 +125,8 @@ GEM
jquery-rails (2.1.4)
railties (>= 3.0, < 5.0)
thor (>= 0.14, < 2.0)
jruby-openssl (0.8.2)
bouncy-castle-java (>= 1.5.0146.1)
json (1.7.6)
json (1.7.6-java)
json-schema (1.1.1)
Expand Down Expand Up @@ -151,6 +154,10 @@ GEM
pg (0.14.1)
plist (3.1.0)
polyglot (0.3.3)
puma (1.6.3)
rack (~> 1.2)
puma (1.6.3-java)
rack (~> 1.2)
rack (1.4.4)
rack-cache (1.2)
rack (>= 0.4)
Expand Down Expand Up @@ -253,13 +260,15 @@ DEPENDENCIES
has_metadata_column!
jira-ruby
jquery-rails
jruby-openssl
json
json_serialize
kramdown
less-rails
libv8 (~> 3.11.8)
net-ldap
pg
puma
rack-cors
rails!
redcarpet
Expand Down
1 change: 1 addition & 0 deletions Procfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
web: bundle exec rails server puma -p $PORT -e $RACK_ENV
7 changes: 7 additions & 0 deletions config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,5 +67,12 @@
get 'search/suggestions' => 'search#suggestions'
get 'search' => 'search#search'

match 'api/1.0/notify', :constraints => {:method => 'OPTIONS'},
Copy link
Contributor

@RISCfuture RISCfuture Jan 30, 2013

Choose a reason for hiding this comment

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

Project standard is to use Ruby 1.9 hash syntax.

:to => lambda { |env| [200, {
'Access-Control-Allow-Origin' => '*',
'Access-Control-Request-Method' => '*',
Copy link
Contributor

@RISCfuture RISCfuture Jan 30, 2013

Choose a reason for hiding this comment

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

The /api/1.0/notify route is POST-only; you can constrain that here as well.

Copy link
Author

@wrs wrs Jan 30, 2013

Choose a reason for hiding this comment

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

Not sure I understand — this is the response for OPTIONS, not POST.

Copy link
Contributor

@RISCfuture RISCfuture Jan 30, 2013

Choose a reason for hiding this comment

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

Unless I'm mistaken, an OPTIONS request is used to determine who can make a request to that same route, what headers they can use, what methods they can use, etc. So, when a client makes an OPTIONS request to /api/1.0/notify, the response should basically say "any host can request this resource; they may supply only the Content-Type header, and they may only use the POST method."

Copy link
Author

@wrs wrs Jan 30, 2013

Choose a reason for hiding this comment

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

Oh, sorry, I thought you meant the route constraints — yes, Access-Control-Request-Method should be POST.

Copy link
Author

@wrs wrs Jan 30, 2013

Choose a reason for hiding this comment

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

And of course this doesn't belong in this pull request, as it has nothing to do with Heroku! :)

Copy link
Contributor

@RISCfuture RISCfuture Jan 30, 2013

Choose a reason for hiding this comment

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

Oh, I figured for some reason Heroku needed this =/

Copy link
Author

@wrs wrs Jan 31, 2013

Choose a reason for hiding this comment

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

Chrome dev channel didn't want to send the notification without this. I haven't tried it in other browsers to see if it makes a difference, but my reading of CORS indicates that they all will require it eventually.

Copy link
Contributor

@RISCfuture RISCfuture Jan 31, 2013

Choose a reason for hiding this comment

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

OK. I've got rack-cors in the Gemfile already; I use it in https://github.com/SquareSquash/web/blob/master/config/environments/development.rb#L54 -- perhaps you can use that?

'Access-Control-Allow-Headers' => 'Content-Type'
}, []] }

root to: 'projects#index'
end