Use rackup in Procfile

[patcon]

Addresses #906

This also explicitly uses port 3000, which otherwise can vary between servers.
(foreman defaults PORT envvar to 5000; rack defaults to 9292 on its own, etc)

[patcon]

@monfresh in regards to #906 (comment):

Might you be willing to accept this? It changes very little doc-wise, but would seem to make dev a little closer to prod, and would allow folks using heroku to re-use Procfile you use for dev :)

[monfresh]

Thanks for the PR. It seems fine to me, but want to let my teammates take a look when they get back from vacation next week.

[pkarman]

@amoose you care to comment? I'm fine with this but don't really know the nuances of rails server vs rackup except what I read on http://stackoverflow.com/questions/9383358/what-are-the-differences-between-using-rails-server-and-rackup

[amoose]

Hello @patcon! Thank you for your recent contributions!

I'm not opposed to this change; if we include the same middlewares outlined here.

A nice-to-have would be a configurable port via ENV variable or argument, for when 3000 is in use.

[pkarman]

hey @patcon -- looks like a recent PR created a conflict for this change. If you'd be willing to rebase and resolve, I can merge.

[jessieay]

Oh darn, looks like there is still a merge conflict here. Care to resolve, @patcon ?

[patcon]

Oops, missed that msg before. Done and done!

[jessieay]

🌈

[monfresh]

I think this still needs the following lines added to config.ru, based on @amoose's comment

use Rails::Rack::Debugger
use Rack::ContentLength

[pkarman]

@patcon I'm not sure why the codeclimate/coverage hook is stuck on this. Could you try squashing your 2 commits and re-pushing to GH? Thanks.

[monfresh]

The CC coverage won't work for forks because it requires a secret token on Travis. We'll have to disable then reenable the check when merging external PRs.

[monfresh]

Please hold off on merging this. I'm running into a Sidekiq issue starting the app. Trying to figure out if it's related to this PR or not.

[monfresh]

Yep, it is. The addition of those 2 lines to config.ru are preventing the app from starting. Looking into it now.

[monfresh]

The culprit is use Rails::Rack::Debugger. It's saying uninitialized constant Rails::Rack::Debugger.

[monfresh]

I've never used the --debugger option when running rails server. Does anyone else use it? Anyone object to removing it for now so we can merge this, and then we can look into how to add it back if needed? @amoose?

[pkarman]

I have never used --debugger - ok with me to skip it.

[monfresh]

Problem solved. It looks like the documentation is super old unfortunately. The --debugger option has been ignored since Ruby 2.0, so we don't need it:
https://github.com/rails/rails/blob/4-2-stable/railties/lib/rails/commands/server.rb#L32-L37

@patcon Sorry for the confusion. Could you please remove the use Rails::Rack::Debugger line? Thanks!

[patcon]

Done, and no prob at all! In fact, I should have tested so it wasn't you who hit it ;)

[pkarman]

looks great @patcon thanks!

If you squash these 3 commits into 1 and re-push, I will merge.

[monfresh]

Thanks! Please squash your commits and we should be good to go.

[pkarman]

@patcon I went ahead and squashed as part of the GH merge. Thanks for your contribution!

[patcon]

haha i promise my next 2-line contribution will be quicker and cleaner :)

Is it a policy that you like squashed commits? I know sometimes that throws off code review on larger PRs. But if so, I can add something to the CONTRIBUTING.md. Lemme know

[pkarman]

@patcon yes, sorry, just released that the code style docs are all internal (private) right now, but do not need to be. Here's the relevant bit that might help you craft some language in a PR against CONTRIBUTING.md

You are our first external contributor to this particular repo (congrats!), and we hadn't yet written anything formal for that case.

How do I merge a PR?

Once a PR has passed all automated checks and has received at least
one approval, it is ready to be merged to master.

All PR commits should be squashed into a single commit before merging unless
there is a good reason not to (e.g. updating gems, want a different commit for
each gem update).

The author or reviewer may merge a PR as long as it is approved.

If the author has created several "fix" commits in response to reviewer
feedback, the reviewers may approve the PR and ask the reviewer to
"squash and merge" -- meaning: squash commits, force push, then merge the pull
request once the automated checks pass again.

Note: If you are squashing commits, only force push if you are the branch owner
and are not collaborating on your branch. Otherwise, you risk over-writing
another person's work.

The person merging the PR should:

• Use the "Rebase and
  merge" option on
  GitHub so that merge commits do not show up on master.
• Delete the PR branch.

The author of the PR should be explicit if non-normal flow is to be used (e.g.
not squashing, not deleting branch after merge)

[patcon]

Thanks! Am i missing why y'all couldn't just disable all options except "Allow squash merging" on the Settings page for the repo?

[monfresh]

We allow both squashing and rebasing depending on the scenario. The most common scenario is as follows:

1. Person 1 submits a PR with one or more commits
2. Another person reviews the PR, and asks for changes
3. Person 1 then makes the changes in new commits, to make it easier for the reviewer to see the changes
4. Once the PR is ready to be merged, Person 1 squashes the commits into 1. That can either be done on Person 1's computer via git rebase -i, or they can do in on the GitHub website using the "squash and merge" option

Another scenario is when we use bummr to update gems, as in #973, for example. In that case, we want a single commit for each gem update, but it's all part of the same PR. In that case, we don't want to squash, so we use the "Rebase and merge" option.