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

Fixed link to heroku 1 click deploy #3530

Merged
merged 3 commits into from
Sep 2, 2019

Conversation

lexoyo
Copy link
Contributor

@lexoyo lexoyo commented May 4, 2019

The link to heroku now takes the file app.json into account so that the developer can see a nice UI to provide values to required env vars

This pull request makes the following changes:

  • In the readme, the link leads to 1 click deploy

Testing checklist:

  • link to 1 click deploy

  • I certify that I ran my checklist

Fixes #3529 .

Ping @ushahidi/platform

The link to heroku now takes the file app.json into account so that the developer can see a nice UI to provide values to required env vars
@ushbot
Copy link
Collaborator

ushbot commented May 4, 2019

@rjmackay It looks like @lexoyo just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@coveralls
Copy link

coveralls commented May 4, 2019

Coverage Status

Coverage remained the same at 20.526% when pulling 3b0e0c1 on lexoyo:3529-link-to-heroku-1-click-deploy into 8b5ec78 on ushahidi:develop.

@@ -19,7 +19,7 @@ Ushahidi 3
[![Sponsors on Open Collective](https://opencollective.com/platform/sponsors/badge.svg)](#sponsors)


[![Deploy](https://www.herokucdn.com/deploy/button.png)](https://heroku.com/deploy)
[![Deploy](https://www.herokucdn.com/deploy/button.png)](https://heroku.com/deploy?template=https://github.com/ushahidi/platform/tree/master)
Copy link
Contributor

Choose a reason for hiding this comment

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

@tuxpiper should this point to master or develop? I think master but just checking in case you disagree
do you see any issues with this approach (aside from the fact that we need to "hardcode" the branch in)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks
@lexoyo for submitting this 💯
Question: do you think we should explicitly call out all the ENV params in the app.json ?

https://stackoverflow.com/questions/44026881/deploy-to-heroku-button-with-env-variables
https://devcenter.heroku.com/articles/heroku-button#creating-the-app-json-file
was just checking this out and we could define the vars from https://github.com/ushahidi/platform/blob/master/.env.example in app.json too

cc @tuxpiper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usually it is the master branch because the use case is "As a developer I want to host an instance on heroku to show my boss that it is easy to self host this tool"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could define the vars from https://github.com/ushahidi/platform/blob/master/.env.example in app.json too

Yes sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe for that we should merge this and start an issue to discuss what we want in a second step?

my PR is just a fix, the app.json was already there

Copy link
Contributor

@rowasc rowasc left a comment

Choose a reason for hiding this comment

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

@tuxpiper I will merge this tomorrow , unless you disagree ? last call :)

@tuxpiper
Copy link
Member

@lexoyo how is it working for you? Are you in a heroku paid plan or in the free tier? Last time I tried, the free heroku tier would not complete the database migrations, because they were too big.

@lexoyo
Copy link
Contributor Author

lexoyo commented May 30, 2019

Hi @tuxpiper
Sorry for the late answer
I was only trying to fix the link in a first step

Since then I had the chance to test heroku and I believe the problem is with the conexion to the DB. Heroku provides an env var CLEARDB_DATABASE_URL with the DB URL but it looks like the migrate script does not use it.

Do you have a pointer where to change that? Where is the .env used by the migrate script in the context of heroku?

@tuxpiper
Copy link
Member

@lexoyo I remember taking a stab at this some time ago, pls check this branch diff: https://github.com/ushahidi/platform/compare/3305-fix-heroku-deployment

The issue I ended up running into was that the free database tier in heroku sets up a rate limit on the number of transactions and that was quickly exceeded by phinx

@rowasc rowasc merged commit 79c5380 into ushahidi:develop Sep 2, 2019
@rowasc
Copy link
Contributor

rowasc commented Sep 12, 2019

@all-contributors please add @lexoyo for doc

@allcontributors
Copy link
Contributor

@rowasc

I've put up a pull request to add @lexoyo! 🎉

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.

the link to heroku 1 click deploy is not right
5 participants