search

mattpolito / paratrooper

Library for creating tasks that deploy to Heroku
MIT License
107 stars 19 forks source link

Fixes and Opinions #64

Closed ryanwood closed 10 years ago

ryanwood commented 10 years ago

First off, great project. Very well coded. This PR has a handful of different things in it. Each commit is modular and focused on a specific fix or change. I've added/update specs where appropriate. I'm happy to split this up into multiple PRs but I wanted you to review it first and let me know what you thought.

3 of the commits are bug fixes with specs to show failures & fixes.

The other 2 are backward incompatible changes but I think are more sensible defaults (and easily overridden). Here are my suggestions:

I'm happy to answer any questions you may have. This PR really comes out of requirements that I need to use this project. I hope they will help others. Let me know if you have questions or need commits in another fashion.

knwang commented 10 years ago

I don't agree with default being HEAD instead of master. Deployment should by default go from an integration branch, which typically is master. Deploying from a feature branch directly should be the exception not the norm.

ryanwood commented 10 years ago

In the case of deploying to production, I completely agree with you. Deploy a feature branch to a staging server is a very real use case for staging and isolated testing. Yes, it is an exception, but the current configuration doesn't support any way to do that. You'd have to alter your deploy script changing the branch every time which is not a great scenario. I'm trying to make that valid exception possible.

If you want the default to remain as master, there needs to be a way to set the reference point to HEAD in the config. I can tweak it to allow branch = 'HEAD' or branch = :head to override the default of master if that is preferred.

Let me know what you think.

knwang commented 10 years ago

Deploying from master being the default and supporting deploying from head could work.. though this is more likely a one time thing, it's probably not something you want to keep in deploy.rake. How do you expect to use this? I'd just do a manual heroku push for this myself.

ryanwood commented 10 years ago

@knwang Are you the current maintainer?

To answer your questions:

  1. I've used HerokuSan for years for deployment and deploying from HEAD has always been the default. I was looking for something with a little more flexibility in deploying to different environments in staging. With paratrooper, I can have multiple staging deploys with different rake tasks as opposed to one staging deploy. However, currently this is not possible without the ability to deploy from HEAD.
  2. A manual push won't work for us at MoonClerk as we have other tasks that always need to happen during a deploy (spinning down background workers, notifying our error tracker of deployments, etc.) We'd have to do all that manually if we just did a Heroku push. That defeats the purpose of having deployment scripts.
  3. MoonClerk is build on top of Stripe so we make heavy use of webhook processing. They only allow one staging/development environment so we (more than I'd like) need to push features that aren't yet ready for integration to a staging server to test the webhook interactions.

I don't expect that paratrooper should be aware of my need, or anyone else's specifically, but that it is a flexible enough deployment framework to be used in any situation. I completely understand convention over configuration but not convention instead of configuration. Smart defaults with flexibility should be the goal.

Since all the discussion in this PR is regarding one commit, should separate this PR into 3: the bug fixes, supporting HEAD, adding the force option? That way the bug fixes can be pulled in with the default changes. Thoughts?

mattpolito commented 10 years ago

Thanks for the PR, @ryanwood. I'm reviewing these changes and will have some input for you later on. Sorry I didn't see this sooner, I never get notifications from Github for some reason.

With that being said, it does appear that there are a few logical places the PR could be broken apart for easier consumption. ( the force option, master to head, and random fixes ). If you have some time, it would be helpful for these to be broken apart so I can merge some changes while we can discuss the others.

Thanks again for the PR!

ryanwood commented 10 years ago

@mattpolito Will do. I'll create 3 new PRs and reference the discussion from this one. I'll close this one when I've pushed the other 3. Thanks.

mattpolito commented 10 years ago

Awesome, thank you very much.

Matt Polito 904.534.0664

On Thu, Jul 24, 2014 at 10:33 AM, Ryan Wood notifications@github.com wrote:

@mattpolito https://github.com/mattpolito Will do. I'll create 3 new PRs and reference the discussion from this one. I'll close this one when I've pushed the other 3. Thanks.

— Reply to this email directly or view it on GitHub https://github.com/mattpolito/paratrooper/pull/64#issuecomment-50034618.

knwang commented 10 years ago

@ryanwood I'm not the maintainer :) @mattpolito is. I agree for your workflow, having an option to deploy from HEAD would be nice.

unrelated to paratrooper.. For testing Stripe webhook.. haven't you guys looked into a tunnel service like https://ngrok.com/? May be useful

ryanwood commented 10 years ago

@knwang Thanks for the suggestion. We have. The issue is with using Stripe Connect. They only allow one production and one development webhook endpoint. Our development webhook is pointed at our staging server. There is no way to get those hooks to a development machine without manually posting.

ryanwood commented 10 years ago

I've split this PR into 3 others. Closing.