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

#683 Deployment #1048

Merged
merged 43 commits into from
Jan 4, 2015
Merged

#683 Deployment #1048

merged 43 commits into from
Jan 4, 2015

Conversation

orenyk
Copy link
Contributor

@orenyk orenyk commented Nov 25, 2014

Resolves #683, still needs a bit of work

@orenyk
Copy link
Contributor Author

orenyk commented Dec 2, 2014

stupid AppConfigsController failing tests... I really need to debug those

@orenyk orenyk changed the title 683 Deployment #683 Deployment Dec 29, 2014
@orenyk
Copy link
Contributor Author

orenyk commented Dec 29, 2014

I think this is ready to go, other than intermittently failing test :-P. Please review to make sure that all relevant configuration parameters have been abstracted out and that everything is pretty well documented (including the wiki page and README).

@orenyk
Copy link
Contributor Author

orenyk commented Dec 29, 2014

Stupid intermittent test failures (see #1059). It's mostly the rake test :-.

set :deploy_to, "#{ENV['DEPLOY_DIR']}"

# Set Rails environment
# set, :rails_env, 'production'
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be commented?

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 believe it's the default, we could probably just take it out. Note that this PR probably doesn't have a finalized Capistrano setup since I have no way to test it, it's really just for the isolation of configuration parameters in environment variables and the setup of secrets.yml and .env + documentation.

@squidgetx
Copy link
Contributor

Wow this looks awesome. The new config page and the code read very well. I left a few minor inline comments. I'm definitely not an authority on deployment but as far as I can tell this looks ready.

}

# with these disabled, the server must be connected to the yale network for
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove/modify this comment to be meaningful to non-yale clients

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, done.

@orenyk
Copy link
Contributor Author

orenyk commented Jan 4, 2015

I generalized the comment in setup_mail.rb but left deploy.rb alone since I don't know exactly how to proceed w/ the Capistrano stuff. Should we just take it all out / cherry pick those commits into a new branch and only leave the configuration stuff? Honestly, having the Capistrano files in the repository don't hurt (as we know since we had old ones hanging around for years), and the work necessary to separate out the commits would be a pain, so I'm inclined to leave them and rely on the #1074 to get them polished up.

@squidgetx
Copy link
Contributor

Cherry picking sounds like a pain. Merge this and we'll polish in #1074

orenyk added 3 commits January 4, 2015 00:20
Conflicts:
	app/models/user.rb
	config/environments/production.rb
	config/initializers/devise.rb
	config/initializers/setup_mail.rb
@orenyk
Copy link
Contributor Author

orenyk commented Jan 4, 2015

passing all but our troublesome tests (see #1059), merging!

orenyk added a commit that referenced this pull request Jan 4, 2015
@orenyk orenyk merged commit 45ab7db into master Jan 4, 2015
@mnquintana mnquintana deleted the 683_deployment branch February 20, 2015 13:48
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.

Document and set up generalized deployment
2 participants