Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

capistrano: use :release_path, not :current_release #1264

Closed
wants to merge 1 commit into from

Conversation

sunaku
Copy link
Contributor

@sunaku sunaku commented Jun 21, 2011

This commit fixes the following generated command where a bogus
releases/ls directory is assumed due to the use of :current_release.

failed: "sh -c 'cd MY_DEPLOY_DIR/releases/ls && bundle install --gemfile MY_DEPLOY_DIR/releases/ls/Gemfile --path MY_DEPLOY_DIR/shared/bundle --deployment --quiet --without development test'" on MY_HOST_NAME

I am using capistrano (2.6.0) and bundler (1.0.15) under ruby 1.9.2p180.

This commit fixes the following generated command where a bogus
`releases/ls` directory is assumed due to the use of :current_path.

failed: "sh -c 'cd MY_DEPLOY_DIR/releases/ls && bundle install --gemfile MY_DEPLOY_DIR/releases/ls/Gemfile --path MY_DEPLOY_DIR/shared/bundle --deployment --quiet --without development test'" on MY_HOST_NAME

I am using capistrano (2.6.0) and bundler (1.0.15) under ruby 1.9.2p180.
@indirect
Copy link
Member

Thanks for the patch! Because Capistrano's docs say that :current_path is the correct variable to use, we are going to stick with that. Back when I was writing and testing bundler/capistrano, :release_path is not always the desired directory. If you need :current_path to work differently, please send a pull request to Capistrano.

Alternatively, you could just overwrite the Bundler cap task yourself. You already wrote the code! :)

@indirect indirect closed this Jun 21, 2011
@sunaku
Copy link
Contributor Author

sunaku commented Jun 22, 2011

Alright, I filed this as an issue on capistrano.

@ashchan
Copy link

ashchan commented Nov 25, 2011

@indirect, the problem here is not current_path, but current_release. Capistrano assigns current_release with the last file(directory) name in the releases folder. So if you have a folder created (by mistake in most cases) which happens to be the last one entry via ls -x, that folder would become current_release.

On deployment, most other tasks (create links, compile assets, etc) use release_path while bundler uses current_release. The result is the new deployed folder doesn't have a proper .bundle folder, thus the app would fail to start.

@sunaku
Copy link
Contributor Author

sunaku commented Jan 12, 2012

@indirect would you reconsider this patch in light of @ashchan's explanation? Thanks.

@indirect
Copy link
Member

Last time I checked, Capistrano's maintainers said the correct variable to use was current_path. I am reluctant to go against their suggestion, since I want Bundler to keep working with future versions of Capistrano.

@sunaku
Copy link
Contributor Author

sunaku commented Jan 12, 2012

Fair enough. I'll report back if I experience any problems without this patch, in which case I'll try ask the bundler devs to clarify which variable they recommend to use in this case. Cheers.

@skwp
Copy link

skwp commented Feb 27, 2012

+1 on the patch

release_path is the most reliable variable because it points to exactly where the release is being put. the current_release you're using is causing problems for us because we have other folders in the release dir (I know, not very 'capistrano'-y, but it's the nature of the system we're dealing with). Since current_release appears to use magic (i.e. ls -x) to figure out which release is current, I don't know why it's even used at all in this case. release_path is specifically the path we want, I think.

@indirect
Copy link
Member

@skwp please let us know if Capistrano's maintainers start suggesting release_path over current_path. Until then, I've already explained why Bundler's built in recipe is the way it is.

If you need something else, please copy the recipe and change the single line you need to change. The entirety of the capistrano integration is only a few lines of code anyway.

@skwp
Copy link

skwp commented Feb 28, 2012

As this plugin's maintainer you have absolute rights to merge or not merge a patch :) of course, it is easy for me to copy the recipe. I'm just wondering where you got the idea that they suggest using current_release instead of release_path (btw in your reply, I believe you confused 'current_path' with 'current_release' - they are not the same thing). I'd also like to know for my own education why release_path is a bad thing to use - it is after all exactly the path to the current deploy.

@skwp
Copy link

skwp commented Feb 28, 2012

The original patch author also confused it in his commit message. If you look at his patch, he changed 'current_release' to 'release_path' and in the comments he wrote 'current_path'. This may be adding more than the needed level of confusion to this already confusing topic :)

@indirect
Copy link
Member

Heh. My bad, I did in fact mean current_release.

Like I said earlier in this ticket, if there is somewhere in the Cap docs (or it has otherwise been stated by someone on Cap-core) that recommends release_path over current_release, please let me know so that we can change it. Thanks.

@skwp
Copy link

skwp commented Feb 28, 2012

@indirect here's some stuff i found from a few mins of googling. it's not definitive, but it may be worth a look

in this thread Jamis explains:

release_path is recomputed every time Capistrano is invoked, and is only valid for a deploy that is IN PROGRESS
current_release is always the most recent release in the releases directory.

http://groups.google.com/group/capistrano/browse_thread/thread/4caa32e22b7dc3e7/13f822a59bceb8a1?lnk=gst&q=jamis+release_path+current_release#13f822a59bceb8a1

in this thread, jamis explains that current_release changes throughout the capistrano deploy process, since it's filesystem based. before deploy it points to the latest current release, and after the new release is there. so I don't know why you would use it, since you have to know when you're calling it to ensure it's not the 'wrong' thing
http://groups.google.com/group/capistrano/browse_thread/thread/6a82d5303bc0b0e6/c8d966bb1c9e30b0?lnk=gst&q=current_release#c8d966bb1c9e30b0

some more resources..

engine yard - uses release_path, does not mention current_release
http://docs.engineyard.com/use-deploy-hooks-with-engine-yard-cloud.html

some stats that may be useful..

"capistrano current_release" google search 6,910 results
"capistrano release_path" results 8,870 results
capistrano mailing list. current_release - 140 hits.
capistrano mailing list. release_path - 304 hits

If your capistrano bundler plugin is meant for use during deploys, I would think release_path is the most consistent way to access the deploy currently in progress rather than relying on filesystem

@indirect
Copy link
Member

Firstly, we of course know exactly when we are calling it, because we insert it into the deploy ourselves: after "deploy:update_code", "bundle:install". Since we have control over when the bundle process runs, and it's at a time when current_release is known to be good, that's not a concern. Secondarily, the deploy task that this patch changes is run by both Vlad and Capistrano, so any changes need to work in both deploy systems.

This script is supposed to be the "easy version". If you use Cap in a default way, this just works. If you don't use Cap in a default way, all bets are off, and unfortunately I don't have time to provide support for situations like that. Since there is no clear indication that either one is "right", and the current version is known to work in both Cap and Vlad, I think we should leave things the way they are for right now.

@shevaun
Copy link

shevaun commented Apr 23, 2012

+1 for your research @skwp, I agree that release_path is the most consistent way to access the current deployment.

I've run into this issue also (where current_release is being set to the wrong directory on initial deployments) and fixed it for our apps by overriding current_release to be release_path in deploy.rb:

set :current_release, release_path

@skwp
Copy link

skwp commented Apr 23, 2012

@shevaun hah, what a clever hack, i didn't think of that :)

@dontangg
Copy link

I mostly agree with this pull request. I think that release_path or latest_release should be used. I recently ran into this bug and a Google search led me here. I am not sure why current_path was pointing to the wrong directory for me since I am definitely using Capistrano in a default way. It has worked for me just fine until today.

I have searched the Capistrano documentation and I don't see anywhere that they suggest using current_path over release_path or vice versa. However, I'd like to suggest that their code is documentation enough to change it. They use release_path to put the code in the right place (see here). That means that the most reliable way to find the code is by using release_path.

I say that with one caveat. If we want to be able to run the bundle:install task outside of a deploy, then we should actually use latest_release. Documentation for that is also found in their code in a comment. latest_release basically checks to see if we are deploying to a time-stamped directory. If we are, then it uses release_path. Otherwise, it uses current_release. This is probably why you (@indirect) were told to use current_path. If you run the task without doing a deploy, release_path will point to a folder that doesn't exist.

As far as Vlad is concerned, it has the same logic for each of these variables as Capistrano does (see here). current_release is based on ls -x. release_path is always the directory with a new timestamp where it puts the code (see here). Finally, latest_release is either release_path or current_release based on whether or not we are doing a timestamped deploy.

To sum it all up, I think the current_path is a clear loser. release_path is a step up since this task is most often run during a deployment, but latest_release rules them all.

@indirect
Copy link
Member

I'm willing to try this change, but worried about breaking backwards compatibility with deploy scripts that expect bundle:install to use current_path. It sounds like latest_release is a potential replacement that would (hopefully) not interrupt any current uses of the bundle:install task. Anyone want to give that a try?

@dontangg
Copy link

fwiw, I'm using my own bundle:install task now that is using latest_release and it's working great for me both during deployment and on its own.

@leehambley
Copy link

@indirect As I am "Capistrano's Maintainers" - I don't recall ever having had that conversation with you. I've only now been made aware of this farse because of a pull request at Capistrano. Please sort your house out, the Bundler integration with Capistrano is a constant source of support queries that I have to deal with, because it's poorly documented, and poorly implemented.

@indirect
Copy link
Member

@leehambley I apologize for not consulting with you directly. I wasn't aware that you are the only maintainer of capistrano, and frankly have no idea how I could have found that out, since it isn't documented anywhere that I was able to find when I looked.

In another discussion of bundler/capistrano, a long time ago, someone with a sizable number of commits to capistrano suggested using current_path. Since I don't know cap well, I didn't want to change things in a way that could potentially fix one thing while breaking something else.

You spent some time talking about how this "farse" means I should sort my house out, but you didn't mention what the correct variable to use is. Is it latest_release? As you can see directly above your comment, I agreed to change to latest_release based on some excellent investigation by @dontangg, and we are in the process of making that change.

As for "poorly documented, and poorly implemented", please let me know (either in a public ticket or privately to my personal email) any way that we can possibly improve it. I would love to get some feedback containing actual suggestions from someone who knows capistrano well.

Thanks.

@leehambley
Copy link

it isn't documented anywhere that I was able to find when I looked.

You could have approached our community via our mailing list and/or Github (my name is all over our commit history.

I don't know exactly what the Capistrano Bundler integration is doing, and to be honest, a lot of the problems that come from new installations come from cap deploy:cold; a task which hasn't made sense since the days of Mongrel and script/spin, now there's not really a notion of a cold deploy, unless someone is running without monit/etc controlling their unicorns. I'd guess at saying that the more common deploy host now is Passenger mod_rails anyway, where deploy:cold is obsolete.

To offer something useful, in order to bundle gems for the current deployment, I would personally recommend using release_path (that is, the current, in-progress release), and hooking at before 'deploy:finalize_update', &block.

As for "poorly documented, and poorly implemented"

I'm picking specifically on the small, un-link-to-able note about the cap-bundler integration page on the Bundler docs, I would recommend that this be a real page, or at least have an anchor so that I can refer people to that heading without issuing a general RTFM for the Bundler docs, which is the best I can do now. As for implementation, I'm referring to this issue.

Anyway, @indirect I'm sorry for my over-reaction - we're all friends here on :octocat: and I don't want to call you out on a simple mistake, and I should have offered some decent constructive input instead of picking a fight over one misappropriation of my role as the sole maintainer.

It's annoyed me for a long time that neither Carl nor Yehuda approached me to discuss ways of integrating Bundler more tightly with Capistrano, and this just piqued my pre-existing frustration.

@dontangg
Copy link

I think that it still makes sense to use latest_release to allow for the bundle:install task to be run outside of a deployment. During a deployment latest_release == release_path. Outside of a deployment, latest_release == current_release. (Here is the link to the explanation of latest_release again). Do you agree with this @leehambley?

@leehambley
Copy link

I'm not sure why you would bundle (or use Cap for any ad-hoc commands) outside of a deployment, that's generally a bad practice but @dontangg is right. (Apart from using Cap for general administration being an anti-pattern)

@indirect
Copy link
Member

Thanks for the suggestions. We currently hook into after 'deploy:finalize_update'. I would be glad to change it to before. I'm guessing is so that potential failures happen at a time that is easier to catch?

I'm also making the change to latest_release instead of current_release. @dontangg et. al., would you be willing to try Bundler from master and see if that fixes things for you?

@leehambley, I apologize for the lack of communication with you. I made the decision to accept Capistrano integration into Bundler back when Carl and Yehuda were still working on it, and I've since taken over as primary maintainer (along with Terence). It seemed like a good idea at the time to provide some easy integration between the two, and we tried to add copious documentation to the capistrano task itself.

I completely understand the frustration of not being able to link to the right place in the Bundler docs, and I will fix that on the documentation site as soon as possible. If you have any other thoughts or ideas about integration between Bundler and Capistrano, I would truly love to hear them. As I said before, I don't use cap much myself, so input from someone who is knowledgable about it would be greatly appreciated. Thanks!

@dontangg
Copy link

Works great for me! Thanks!

@dipnlik
Copy link

dipnlik commented May 24, 2012

Just installed bundler from master and my original problem is gone, thanks @indirect :)

@indirect
Copy link
Member

Awesome, thanks everyone! These changes will be part of the Bundler 1.2 release, and will be available from Rubygems as soon as we push the 1.2.0.pre.1 gem.

drogus pushed a commit to drogus/bundler that referenced this pull request May 26, 2012
switch the bundler hook to before "deploy:finalize_update", so that finalize_update will not run if `bundle install` fails. (thanks @leehambley)

change from `current_release` to `latest_release`, because both cap and vlad provide more robust directory-finding code behind `latest_release`. (thanks @dontangg)

closes rubygems#1264
thetamind pushed a commit to thetamind/bundler that referenced this pull request May 29, 2012
switch the bundler hook to before "deploy:finalize_update", so that finalize_update will not run if `bundle install` fails. (thanks @leehambley)

change from `current_release` to `latest_release`, because both cap and vlad provide more robust directory-finding code behind `latest_release`. (thanks @dontangg)

closes rubygems#1264
@cgriego
Copy link
Contributor

cgriego commented Jul 18, 2012

@indirect Good call on going with latest_release. Best option. I got pretty worked up based on the pull request diff which only shows release_path. I've only just become aware of this issue due to the RC release. I used to watch Bundler closely for changes related to deployment, but deployment stabilized and the activity become too much to sift through it all. I wish GitHub would let me watch individual files… anywho. If questions like this come up, please feel free to @cgriego me.

@leehambley Bundler's capistrano tasks are actually some of the most well documented 3rd party tasks in the Capistrano ecosystem, rivaling the documentation provided for the core deploy tasks. Also I cannot believe that I'm hearing you call ad-hoc commands or using Capistrano's task system an antipattern. This is contradicted right in the gem description ("Capistrano is a utility and framework for executing commands in parallel on multiple remote machines, via SSH.") and how many, including myself, use the project you now maintain. The tool is designed for this and this is how I use it. Preventing this type of use is like like having a rake task that only worked if triggered by the default task.

In fact, I use the bundle:install tasks outside of deployment with my own custom tasks. For example, I have a custom bundle:reinstall task which uses bundle:install for cases where upstream headers have changed (say a MySQL upgrade) or I perform a major rubygems upgrade. Capistrano's deploy:migrate is an example of a task that is in the core Capistrano deploy recipe that is setup to work during and outside of a deploy. Whenever is an example of a gem which includes a Capistrano recipe whose tasks are designed to be used in isolation and during a deploy.

@sunaku Why would you have a releases/ls directory and why would you expect anything in Capistrano to work while that directory exists? The directories in the releases path is is what Capistrano uses to calculate the releases variable which is in turn used to calculate the current_release, previous_release, latest_revision, previous_revision, and latest_release. If you do have a reason to change how Capistrano interprets the releases, override the releases variable. That was never a problem with Bundler's Capistrano recipe.

@skwp Did you seriously call ls magic?

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.

None yet

9 participants