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

WIP - Trigger custom rake tasks at end of deploy process #330

Closed
wants to merge 1 commit into from

Conversation

schneems
Copy link
Contributor

@schneems schneems commented Jan 3, 2015

No description provided.

@hone
Copy link
Member

hone commented Jan 5, 2015

is this a new thing in Rails 4.2 or how does this differ from rake assets:precompile? I guess this is just a different rake task that will get run? I'm mostly worried about expanding the buildpack interface.

@schneems
Copy link
Contributor Author

schneems commented Jan 5, 2015

I would like a supported way to trigger custom actions (i.e. rake db:migrate) without having to do hacky stuff. Right now people hook into rake assets:precompile via Rake#enhance but it seems a bit wrong.

@geoffharcourt
Copy link

This need not be Rails- (or Rails version-) specific. Tim Pope's fork handles determining the tasks to run through an environment variable where custom tasks are declared.

@schneems
Copy link
Contributor Author

schneems commented Jan 6, 2015

I'm not a giant fan of the COMPILE_TASKS env var approach. It means that if you heroku create and push to a staging app it won't "just work" TM out of the box because the deployment logic is now tied to the config. We're already detecting rake task presence, so this adds minimal overhead.

@paulelliott
Copy link

A task I can optionally define in my app to perform the post deploy actions I need would be huge. I could actually deploy to Heroku with just a git push like advertised. Gems and blog posts abound addressing this with local rake tasks that wrap up the deployment process for this reason. It would be great to be able to ditch all that.

Huge 👍 from me.

@WattsInABox
Copy link

How can I help get something like this commit merged down?

@dholdren
Copy link

dholdren commented Mar 9, 2015

👍 here. I've just started using heroku again in a professional setting, and I'd love to have something like this. we were about to fork the buildpack ourselves, then found tpope's, but I'd rather use an officially supported buildpack

@schneems
Copy link
Contributor Author

Got a really good reason why this is a horrible idea to encourage. Wanted to share it to spread the logic. Bare with me, this only affects the really large apps, but when your app goes from being small to large (in data size) then the result can be catastrophic.

If you're using this (or a feature like it) to rake db:migrate you lose all control over your migration. This might not be a big deal if you have a small database, but as your database grows, accidentally doing something as seemingly simple as adding a non null default value to a new column could potentially seize up your entire website for hours, maybe days with no way to effectively back out of the migration.

Most people who want this feature (me included) probably won't run into this problem. However a few will use this feature and grow, continuing to use it until a junior dev adds a migration by accident in a bad git merge and locks up a company that does a few $MM of transactions an hour...for a few hours.

It's a scenario that while the risks of this happening are very small (you may never hit them), when this does occur it could result in news-worth downtime. Given the sheer number of customers and diversity of apps, it's guaranteed that even if this happens in a small percentage of apps, it will happen frequently enough to be a problem.

This doesn't mean you can't get something like this feature by using a custom buildpack or hacking a custom solution in your Rakefile:

task "assets:precompile" do
  Rake::Task["assets:precompile"].invoke
  # Custom code here
end

The difference may seem trivial to you, i.e. "why don't you add in the feature and let people opt to not use it instead of not adding in the feature and letting people hack around it?" By adding in the feature we're making doing the wrong thing easier (hours of downtime, remember?). If you choose to add in your own custom code, you're taking matters into your own hands and leaving our recommendations out of it. If things go south, don't say you weren't warned.

It doesn't mean I'm not still interested in solving this problem. It does mean, that this solution as is won't work wholesale across the platform. I'll continue to investigate with some of our internal teams that run extremely large databases to try to get some guidance and use cases. Once we figure out how they manage around this problem manually, we can start to work to automate around the problem.

tl;dr this feature rocks for small to medium sites, but when you're big enough for downtime to really hurt it will kick back at the worst possible time and cause serious pain.

@paulelliott
Copy link

I don't think this should automatically run migrations. You're right, that would be dangerous.

I don't think a hook where I can run some arbitrary code post deploy is dangerous though. Maybe I want to hit multiple webhooks (the heroku addon only allows one). In my case, I want to be able to clear the redis server that caches all my view partials. I have a custom task for that and the only way I can fire it is to manually do that every time I deploy or use something like paratrooper (which is what I currently do).

@dholdren
Copy link

The feature as defined in this PR merely provides a hook. It's opt-in, not opt-out as you stated: "let people opt to not use it".

It is up to the owner of the app to decide what runs in that hook when (and if) they define the task finish:deploy. (which I'd rather see named heroku:deploy:finish). It's as opt-in as hacking the assets:precompile task in my mind, but is much cleaner.

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.

None yet

6 participants