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

Implement Heroku Review Apps #214

Merged
merged 7 commits into from
Mar 3, 2016
Merged

Implement Heroku Review Apps #214

merged 7 commits into from
Mar 3, 2016

Conversation

dahlbyk
Copy link
Member

@dahlbyk dahlbyk commented Feb 26, 2016

Closes #183. As discussed, due to OAuth host constraints this isn't as trivial as it would be for a simple app.

  • Add app.json review app config, mostly generated by Heroku.

    I did manually add HEROKU_PARENT_APP_NAME, which I wish they had included in the generated file. It took me a long time to figure out that app.json only matters when a review app is first created - this second commit didn't take effect until I deleted the existing review app. Much time wasted here.

  • Override Warden::GitHub::Config.normalized_uri() to do two things:

    1. Update host with the parent app name instead of the current app name.
    2. Append an APP_NAME query string with the current app name, so we can redirect back to it.
  • Update LoginController Add middleware to check for APP_NAME so we can redirect to review apps from the parent app as necessary.

@dahlbyk dahlbyk deployed to huboard-rails-pr-214 February 26, 2016 05:54 Active
@dahlbyk
Copy link
Member Author

dahlbyk commented Feb 26, 2016

A good chunk of experimentation happened over in #201. Here's what it looks like so far:
firefox_2016-02-26_00-30-19

@dahlbyk dahlbyk deployed to huboard-rails-pr-214 February 27, 2016 17:43 Active
@dahlbyk dahlbyk deployed to huboard-rails-pr-214 February 27, 2016 18:02 Active
@dahlbyk
Copy link
Member Author

dahlbyk commented Feb 27, 2016

💥 https://huboard-rails-pr-214.herokuapp.com/ will now round-trip auth through https://huboard-rails.herokuapp.com/.
firefox_2016-02-27_13-10-35

Code is all in one file for hackability, but would welcome guidance on how best to structure it relative to the existing app. And general feedback, of course.

@Steven-Pingel
Copy link

👏

dahlbyk and others added 6 commits March 3, 2016 11:20
Heroku sets APP_NAME and PARENT_APP_NAME environment variables.

The latter needs to replace the former in the OAuth redirect hostname,
and the former needs to be appended as a query parameter to use on return.

Env:
HEROKU_APP_NAME = hb-pr-123
HEROKU_PARENT_APP_NAME = hb

Input: https://hb-pr-123.herokuapp.com/
Output: https://hb.herokuapp.com/?APP_NAME=hb-pr-123
@dahlbyk dahlbyk force-pushed the heroku-pr-deploy branch from 3b52cfa to a46cffd Compare March 3, 2016 17:26
@dahlbyk dahlbyk deployed to huboard-rails-pr-214 March 3, 2016 17:26 Active
@dahlbyk
Copy link
Member Author

dahlbyk commented Mar 3, 2016

For reference, when you auth from a PR build you should see the Warden Override in the logs (e.g. heroku -t -a huboard-rails-pr-214):

2016-03-03T17:48:24.910837+00:00 app[web.1]: Warden Override: huboard-rails-pr-214 => huboard-rails
2016-03-03T17:48:24.910902+00:00 app[web.1]: Warden Override: https://huboard-rails.herokuapp.com/login/public?APP_NAME=huboard-rails-pr-214

And then on the parent side (e.g. heroku -t -a huboard-rails) the app redirect is handled:

2016-03-03T17:48:24.864070+00:00 app[web.1]: AppRedirect: huboard-rails => huboard-rails-pr-214
2016-03-03T17:48:24.865453+00:00 app[web.1]: AppRedirect to http://huboard-rails-pr-214.herokuapp.com/login/public?code=d438whatever0342&state=7a88ce7e378cf5afwhatever56854fcdce9b5

Looking again at the logs, I'm going to suppress the AppRedirect logging if app_name is not set. We're seeing a lot of AppRedirect: huboard-rails => noise.

@dahlbyk dahlbyk deployed to huboard-rails-pr-214 March 3, 2016 17:55 Active
@dahlbyk
Copy link
Member Author

dahlbyk commented Mar 3, 2016

@discorick raised a good point offline - the app.json will need to be maintained as we add new environment variables. Also note that existing review apps need to be destroyed and recreated to if their app.json changes environment variable settings - the copy happens when the review app is created.

@discorick
Copy link
Member

@dahlbyk talked in length about where the code in the initializer should live (i.e in the bridges middleware stack), ultimately we decided to leave the Warden code where it is. This seems like the right course of action currently as we look into moving to OmniAuth #157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants