Skip to content

Added heroku integration in readme #222

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
mayukh18 wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Added heroku integration in readme #222

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
782bf36
added deploy to heroku button
mayukh18 Oct 21, 2017
225dd66
description to app.json was too long
mayukh18 Oct 21, 2017
8359ec7
add procfile
mayukh18 Oct 21, 2017
09099b2
add runtime.txt
mayukh18 Oct 21, 2017
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
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ For each type of obstacles there is a different action, and different points.

See [examples/README](examples/README) for explanation on how to write a driver module.

[![Deploy](https://www.herokucdn.com/deploy/button.svg)](https://heroku.com/deploy)


## Requirements

Expand Down
6 changes: 6 additions & 0 deletions app.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"name": "ROSE",
"description": "ROSE project car race game.",
"repository": "https://github.com/RedHat-Israel/ROSE",
"keywords": ["race", "game", "python"]

@nirs nirs Oct 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should also describe:

  • The server is started by running the rose-server script
  • Port 8880 should be open to receive web clients
  • Port 8888 should be open to receive rose-client(s)

See the updated issue for more details.

@cben cben Oct 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indeed, tried the button now from your fork https://github.com/mayukh18/ROSE/ => https://cben-rose-test.herokuapp.com, says "Application Error", heroku logs show:

2017-10-21T17:31:29.442324+00:00 heroku[router]: at=error code=H14 desc="No web processes running" method=GET path="/" host=cben-rose-test.herokuapp.com request_id=68b48dde-386d-4756-a4bd-a5901ea49fe5 fwd="141.226.169.57" dyno= connect= service= status=503 bytes= protocol=https

https://cben-rose-test.herokuapp.com:8880/ doesn't work either, connection refused.

  • I don't see anything in https://devcenter.heroku.com/articles/app-json-schema or elsewhere about specifying what port server internally listens on; we have to respect PORT env var that heroku provides.

  • On the outside, heroku router only listens on http[s] :80 and :443. That's fine for user interface, maybe will need to document that :8880 is not needed with heroku.

  • The nastier implication is you can't have 2nd outside port for rose-client. The client will have to connect on 80/443, and will need another mechanism for server to differentiate connection type (URL path? content-type? user-agent (meh)? custom header?).

@mayukh18 mayukh18 Oct 21, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes I too didn't find any documentation related to servers and ports in https://devcenter.heroku.com/articles/app-json-schema and related articles. Second thing which I know is true is that heroku pre-assigns $PORT. What we need is a Procfile

}

@nirs nirs Oct 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Newline missing here.