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

Setup app for heroku deploy #56

Closed
wants to merge 2 commits into from

Conversation

benreyn
Copy link
Contributor

@benreyn benreyn commented May 9, 2018

  • Add postinstall script to package.json to ensure webpack assets are built for
    heroku deploy
  • Add Procfile for heroku to read the main web process
  • Add process.env.PORT to listen call

Ref #15

* Add postinstall script to package.json to ensure webpack assets are built for
  heroku deploy
* Add Procfile for heroku to read the main web process
* Add process.env.PORT to listen call
@benreyn
Copy link
Contributor Author

benreyn commented May 9, 2018

@mickael-kerjean I can circle back and document this and add a Deploy to Heroku button to the README if you accept this contribution.

@dmarto
Copy link
Contributor

dmarto commented May 9, 2018

Just a little note.

Here, you should probably also update the message with the proper port.

@mickael-kerjean
Copy link
Owner

mickael-kerjean commented May 9, 2018

Thank you for your contribution, this is a great idea.

document this and add a Deploy to Heroku button to the README

Yes

Here, you should probably also update the message with the proper port.

I don't see anything wrong about this but:

  • if we go that road, it would make sense to make the binding address configurable as well
  • I would prefer the configuration to appear from the config_server with a default that make sense, something like:
   ...
    info: {
        host: "application_url",
        port: process.env.PORT || 8334,
        bind: process.env.BIND || "127.0.0.1",
        usage_stats: true
    },
    ...
`

@mickael-kerjean
Copy link
Owner

mickael-kerjean commented May 10, 2018

if there's support for heroku, it would either need:

  • a way for someone to enter some sort of configuration somewhere inside heroku (thinking about the dropbox api key and the google drive api key)
  • to configure the application from the config_client.js so that the unconfigured backend doesn't appear for the end user (considering they would be broken)

@benreyn
Copy link
Contributor Author

benreyn commented May 10, 2018

@mickael-kerjean I have this branch deployed to heroku now. On Heroku ENV variables can be added either with the command line, or in the heroku web UI. In general I think all of the required ENV variables need documenting (for any deployment, not just Heroku)

if we go that road, it would make sense to make the binding address configurable as well

👍 Sounds good.

I might be able to pick this back up tomorrow, but it might be the weekend before I get to it.

@mickael-kerjean
Copy link
Owner

On Heroku ENV variables can be added either with the command line, or in the heroku web UI.

Great then

In general I think all of the required ENV variables need documenting (for any deployment, not just Heroku)

That's why I've put them in the root of the repo in the server_config and client_config files thinking that would be enough but if you're implying it, I guess it's not good enough.

I might be able to pick this back up tomorrow, but it might be the weekend before I get to it.

There's no hurry at all, deploying on heroku is a nice addition, not a fix for an awfull security issue. Anyway, whatever times it takes, know that your contribution is really appreciated :)

@mickael-kerjean
Copy link
Owner

I made up a change in the config so that heroku get proper default for the secret_key (random string). it's not great for the end user as it get you disconnect at every restart but that's the price to pay for lazyness

Dumb question: your heroku instance is behind SSL right? Is it the default behavior? If not, it can be a pretty nasty security issue

@benreyn
Copy link
Contributor Author

benreyn commented May 12, 2018

Dumb question: your heroku instance is behind SSL right? Is it the default behavior? If not, it can be a pretty nasty security issue

All paid dynos from heroku have SSL out of the box. Free Dynos do not though, which is what a user would get from clicking the "Deploy to Heroku" button. Perhaps a warning / some documentation in the README under a Heroku Section?

@mickael-kerjean
Copy link
Owner

Perhaps a warning / some documentation in the README under a Heroku Section?

I agree

@mickael-kerjean
Copy link
Owner

A whole lot has change in this repo since this PR was created. If there's no activity here at all, I will close this somewhere around next week

@benreyn
Copy link
Contributor Author

benreyn commented Sep 24, 2018

Fine to go ahead and close up, I really lost the time to work on this.

@benreyn benreyn closed this Sep 24, 2018
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.

3 participants