Skip to content
This repository was archived by the owner on Jul 24, 2020. It is now read-only.

#275 Heroku #1103

Merged
merged 1 commit into from
Feb 17, 2015
Merged

#275 Heroku #1103

merged 1 commit into from
Feb 17, 2015

Conversation

orenyk
Copy link
Contributor

@orenyk orenyk commented Jan 15, 2015

Resolves #275 (and #1104), needs to be squashed but appears to be fully functional :-).

Things I have not yet tested:

  • LDAP
  • CAS

@@ -31,7 +31,7 @@

# Disable Rails's static asset server
# In production, Apache or nginx will already do this
config.serve_static_assets = false
config.serve_static_assets = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realized that we might not want to do this directly, since in many deployment situations you won't want this to be true. We could define an environment variable for this (e.g. RES_HEROKU) and use that to determine the setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, I'm going to do something like this and update app.json (and the documentation).

@orenyk
Copy link
Contributor Author

orenyk commented Jan 15, 2015

Please also look over the changes to the custom buildpack (i.e. my commits) as well, thanks!

@squidgetx
Copy link
Contributor

this all looks excellent - I just deployed a test instance to heroku in about 5 minutes which is pretty solid. However, I think we need to add a little more in terms of setup/seed scripting:

  • we don't want our dev seed script running (probably); have this as an option and default off
  • it's unclear how to log in to the app; the seed script just creates the account with username [email protected] and password passw0rd but if you don't know that (which you don't because heroku doesn't show you the seed script output) then you've got a deployed with no admin access.

IMO we should just require the user to run rake app:setup themselves with minimal mode off via the heroku console after they deploy

@orenyk
Copy link
Contributor Author

orenyk commented Jan 26, 2015

In terms of the login credentials, I noted them in the wiki page but you're right, if you're just coming from the repo it won't be clear (even though you'll likely have to visit the wiki page to deal with the configuration stuff).

I'm fine to default to no seeding / requiring app:setup, we'll just have to update our documentation accordingly. I just figured that since you were clicking a "Deploy to Heroku" button to get your instance started, you'd probably start fresh if you wanted to deploy for real and this would be more oriented towards people who just want to mess around, hence, rake db:seed running by default.

@orenyk orenyk mentioned this pull request Feb 1, 2015
@orenyk
Copy link
Contributor Author

orenyk commented Feb 1, 2015

Note to self, once #1115 is merged in, update the lib/tasks/scheduler.rake to include the new rake task to send e-mails for missed reservations.

@orenyk
Copy link
Contributor Author

orenyk commented Feb 16, 2015

I made the above change; I'd like to hold off on merging this in until #1146 and #1116 are merged in just to offset any merge conflicts.

@orenyk
Copy link
Contributor Author

orenyk commented Feb 16, 2015

@squidgetx: are you good with leaving rake db:seed running by default with the Heroku button and allowing for proper setup using rake app:setup via the command line if you want to deploy a local instance? I don't think we should assume that someone clicking the Heroku button will necessarily be comfortable on the command line (e.g. we're providing users with the ability to spin up demo instances), so I'd rather avoid it for Heroku button users. I'll make a note at the end of the wiki page that explains how to clear the database if you want to start fresh and I think that should cover all of our use cases. Does that make sense?

Let me know how this all looks (now that I addressed my comment) and if I should squash commits. Thanks!

@squidgetx
Copy link
Contributor

Sounds good to me; squash + merge!

old_blackouts.each do |b|
Rails.logger.info "Deleting old blackout:\n #{b.inspect}"
b.destroy
if AppConfig.blank? || AppConfig.first.blackout_exp_time.blank?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why this was changed... I must have screwed up my merge. I'll go back to the cleaner form.

@orenyk
Copy link
Contributor Author

orenyk commented Feb 16, 2015

On second thought, I want to test both CAS and LDAP, here we go!

@orenyk
Copy link
Contributor Author

orenyk commented Feb 16, 2015

Ok, CAS works perfectly :-D

@mnquintana
Copy link
Contributor

:shipit: :shipit: :shipit:

@orenyk
Copy link
Contributor Author

orenyk commented Feb 16, 2015

LDAP isn't working right now :-(. Not sure if it's Heroku or just master :-P

@orenyk
Copy link
Contributor Author

orenyk commented Feb 16, 2015

BTW, merged in the #1116 changes and it looks really nice 😄

@orenyk
Copy link
Contributor Author

orenyk commented Feb 16, 2015

Yale LDAP lookup isn't working, and I don't know of another service I can use to test it. Anyone have any ideas?

@squidgetx
Copy link
Contributor

You mean LDAP isn't working on heroku-deployed instances? Seems to make sense right, since LDAP just doesn't work anyway when not on a Yale server

@orenyk
Copy link
Contributor Author

orenyk commented Feb 16, 2015

So that's not quite true. You can't do NetID-based lookups from outside Yale, but you should be able to do e-mail based lookup (according to @dgoerger). I'm switching off CAS and seeing if that helps.

EDIT Woot! It works 😆

@orenyk
Copy link
Contributor Author

orenyk commented Feb 16, 2015

Ok, I think this is all good. Can someone please do a quick final review and merge? Thanks!

@dgoerger
Copy link
Contributor

Yup, you can look up by email like this from a linux terminal:

ldapsearch -LLL -x -h directory.yale.edu -b o=yale.edu mail=$EMAIL_ADDRESS

Changing the login field for CAS-auth from NetID to email address probably makes sense anyway, given non-CAS auth uses email, right?

@orenyk
Copy link
Contributor Author

orenyk commented Feb 17, 2015

We're (probably I'm) going to try and make CAS and non-CAS auth live happily together (see #1106), hopefully in v5.2.0. We could just switch all LDAP lookups to e-mail (since I believe we enforce uniqueness on e-mail in both cases), I'll add a note there.

@orenyk orenyk force-pushed the 275_heroku branch 2 times, most recently from 5bb8617 to d1c7e7e Compare February 17, 2015 03:11
@orenyk
Copy link
Contributor Author

orenyk commented Feb 17, 2015

Ok, resolved a merge commit due to #1110, ready for final review / merge.

@squidgetx
Copy link
Contributor

Did you mean to commit the bin directory?

@orenyk
Copy link
Contributor Author

orenyk commented Feb 17, 2015

Yup, see here

@squidgetx
Copy link
Contributor

Okay. I left one super minor inline comment, otherwise this is ready for merge

@orenyk
Copy link
Contributor Author

orenyk commented Feb 17, 2015

Thanks! Issue resolved, just re-squashing and then I'll merge 😄

added pg gem to Gemfile

made log level server configurable

fixed bin/rails error

updated production environment to fix broken assets (hopefully)

trying a different fix for broken assets

more production.rb tweaks

added rails_12factor gem for Heroku

trying to fix EISDIR error

made party_foul conditional on config

updated Gemfile.lock

rubocop fixes (not being checked anyway)

undoing conditional

now party foul should hopefully not screw up?

fix issues with non-existant app configs

moved ffaker out of development

removed redundant conditional

moved progressbar out of development

check for git directory before calling git commands

setting up scheduler for cron jobs

fixing rake typo

fixed broken rake task?

trying to fix broken dates issue

another date fix

added unicorn / Procfile (as suggested by Heroku)

initial app.json for Heroku button

added Heroku button to README

moved rack timeout initializer to example

removed extra space

updated app.json with better instructions

fixed app.json optional env params

reset cart.rb

whoops forgot postgresql

reworked how we handle e-mail authentication configs

updated app.json description

shortened description

removed staging environment, cleaned up Gemfile

fixed link to correct file

added missed reservations email task to scheduler.rake

fixed incorrect merge

made static assets serving configurable via ENV

updated .env.example to include USE_LDAP

updated .env.example with required parameters

ignored .env file

improved SERVE_STATIC logic
@squidgetx
Copy link
Contributor

cool

orenyk added a commit that referenced this pull request Feb 17, 2015
@orenyk orenyk merged commit 0642a6c into master Feb 17, 2015
@orenyk orenyk mentioned this pull request Feb 17, 2015
6 tasks
@orenyk orenyk deleted the 275_heroku branch February 20, 2015 17:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

META: Heroku
4 participants