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

Revert the cap deploy:symlink to deploy:create_symlink renaming #157

Closed
tvdeyen opened this Issue Feb 20, 2012 · 22 comments

Comments

Projects
None yet
7 participants
@tvdeyen

tvdeyen commented Feb 20, 2012

Please revert the renaming of the deploy:symlink task to deploy:create_symlink

EDIT: Removed the ranting :)

@pseidemann

This comment has been minimized.

Show comment
Hide comment
@pseidemann

pseidemann Feb 20, 2012

+1

this change broke the app from my client

+1

this change broke the app from my client

@robinboening

This comment has been minimized.

Show comment
Hide comment
@robinboening

robinboening Feb 20, 2012

For many of our client projects deployment with capistrano v2.10.0 means: Folders are not getting symlinked because of missing deploy:symlink where our scripts hook after.

+1 for reverting the renaming.

For many of our client projects deployment with capistrano v2.10.0 means: Folders are not getting symlinked because of missing deploy:symlink where our scripts hook after.

+1 for reverting the renaming.

@halfbyte

This comment has been minimized.

Show comment
Hide comment
@halfbyte

halfbyte Feb 20, 2012

Guys. Before ranting up and down on a change of the API (A change that refactors a misnomer in the old API, a welcome change in my view, let's be clear on that), let's look at the underlying issue here:

None of you guys know exactly what capistrano's policies for stable API's are. 2.10.0 is a minor release, but maybe it's @leehambley's policy to guarantee stable API on minor versions only?

Maybe (and I kind of assume that this is the case) @leehambley doesn't actually have policy on that and the change was unintentional or, at least, it wasn't thought through. Fine.

Let's assume that @leehambley wants to follow semver.org practices.

From what I've seen so far, this change would break the semver.org spec, if (and only if) it is agreed upon that hooking into deploy:symlink_task was public API. Do we? @leehambley?

(I for one always use, and I cannot tell you exactly why, deploy:update_code for my hooks)

But, please. Stop ranting. I know you're angry, because this change broke something you built. But it didn't only break because capistrano was changed, but also because you updated a critical part of your infrastructure without ensuring that your system can handle that update. (No, not having updated the CHANGELOG at releasetime doesn't really help, I agree on that, but that's a different Issue (#156 to be exact)

So cut the exclamation marks, raise the right questions and show some love and respect to your open source package maintainer. :)

Guys. Before ranting up and down on a change of the API (A change that refactors a misnomer in the old API, a welcome change in my view, let's be clear on that), let's look at the underlying issue here:

None of you guys know exactly what capistrano's policies for stable API's are. 2.10.0 is a minor release, but maybe it's @leehambley's policy to guarantee stable API on minor versions only?

Maybe (and I kind of assume that this is the case) @leehambley doesn't actually have policy on that and the change was unintentional or, at least, it wasn't thought through. Fine.

Let's assume that @leehambley wants to follow semver.org practices.

From what I've seen so far, this change would break the semver.org spec, if (and only if) it is agreed upon that hooking into deploy:symlink_task was public API. Do we? @leehambley?

(I for one always use, and I cannot tell you exactly why, deploy:update_code for my hooks)

But, please. Stop ranting. I know you're angry, because this change broke something you built. But it didn't only break because capistrano was changed, but also because you updated a critical part of your infrastructure without ensuring that your system can handle that update. (No, not having updated the CHANGELOG at releasetime doesn't really help, I agree on that, but that's a different Issue (#156 to be exact)

So cut the exclamation marks, raise the right questions and show some love and respect to your open source package maintainer. :)

@tvdeyen

This comment has been minimized.

Show comment
Hide comment
@tvdeyen

tvdeyen Feb 20, 2012

@halfbyte You are right. I edited my ticket and apologize by @leehambley 🐻

But could we please revert the change, or at least raise the version number to 3.0.0?
Thanks :)

tvdeyen commented Feb 20, 2012

@halfbyte You are right. I edited my ticket and apologize by @leehambley 🐻

But could we please revert the change, or at least raise the version number to 3.0.0?
Thanks :)

@tvdeyen

This comment has been minimized.

Show comment
Hide comment
@tvdeyen

tvdeyen Feb 20, 2012

But it didn't only break because capistrano was changed, but also because you updated a critical part of your infrastructure without ensuring that your system can handle that update.

@halfbyte Hmm, I have to disagree with that. I have ~> versioning in my Gemfiles and a simple bundle broke my deployment. With semver this would not have happened.

tvdeyen commented Feb 20, 2012

But it didn't only break because capistrano was changed, but also because you updated a critical part of your infrastructure without ensuring that your system can handle that update.

@halfbyte Hmm, I have to disagree with that. I have ~> versioning in my Gemfiles and a simple bundle broke my deployment. With semver this would not have happened.

@robinboening

This comment has been minimized.

Show comment
Hide comment
@robinboening

robinboening Feb 20, 2012

@halfbyte you´re right, ranting is not the right way. We love and respect people who share sourcecode with other!
Btw: Thanks for capistrano <3

But what if the update_code task would have been renamed? ;-) Would you say you did not ensure that your system can handle minor version updates of capistrano?

@halfbyte you´re right, ranting is not the right way. We love and respect people who share sourcecode with other!
Btw: Thanks for capistrano <3

But what if the update_code task would have been renamed? ;-) Would you say you did not ensure that your system can handle minor version updates of capistrano?

@gaffneyc

This comment has been minimized.

Show comment
Hide comment
@gaffneyc

gaffneyc Feb 20, 2012

I don't feel that renaming deploy:symlink to deploy:create_symlink adds much clarity to what's happening in the step. It also breaks six years of existing capistrano scripts.

A better approach may have been to update the code to use deploy:create_symlink and keep deploy:symlink that depended on deploy:create_symlink and gave a deprecation warning.

I don't feel that renaming deploy:symlink to deploy:create_symlink adds much clarity to what's happening in the step. It also breaks six years of existing capistrano scripts.

A better approach may have been to update the code to use deploy:create_symlink and keep deploy:symlink that depended on deploy:create_symlink and gave a deprecation warning.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Feb 20, 2012

Member

Please take your complaint to Rake, who introduced poisonous global methods with their DSL. This was changed in 2.9.0, was documented, and many people adapted to it. The name is clearer, and Capistrano will easily fail a deploy, without breaking anything in production if your symlink task misses.

As the change doesn't risk downtime, there's no easy way to make the transition, just deal with the pain now, and we'll be safer in the future.

As I made the change in 2.9.0, and didn't find a suitable way to un-define Rake's globals until 2.10.0, we're stuck with the change, it'd be worse to change the API at a whim every time I felt like it.

Member

leehambley commented Feb 20, 2012

Please take your complaint to Rake, who introduced poisonous global methods with their DSL. This was changed in 2.9.0, was documented, and many people adapted to it. The name is clearer, and Capistrano will easily fail a deploy, without breaking anything in production if your symlink task misses.

As the change doesn't risk downtime, there's no easy way to make the transition, just deal with the pain now, and we'll be safer in the future.

As I made the change in 2.9.0, and didn't find a suitable way to un-define Rake's globals until 2.10.0, we're stuck with the change, it'd be worse to change the API at a whim every time I felt like it.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Feb 20, 2012

Member

With semver this would not have happened.

Wrong, it's [architecture, feature, patch], a feature has changed. Not the architecture

Member

leehambley commented Feb 20, 2012

With semver this would not have happened.

Wrong, it's [architecture, feature, patch], a feature has changed. Not the architecture

@leehambley leehambley closed this Feb 20, 2012

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Feb 20, 2012

Member

For what it's worth, I do recommend locking the absolute hard version you require, many of you rely on things which have been reported as "bugs" on Capistrano, and I dare say not many of you test your deployments.

Member

leehambley commented Feb 20, 2012

For what it's worth, I do recommend locking the absolute hard version you require, many of you rely on things which have been reported as "bugs" on Capistrano, and I dare say not many of you test your deployments.

@halfbyte

This comment has been minimized.

Show comment
Hide comment
@halfbyte

halfbyte Feb 20, 2012

Lee, you probably already noticed that I tried to defend you here, but I have a minor quibble:

With semver this would not have happened.

Wrong, it's [architecture, feature, patch], a feature has changed. Not the architecture

Do you consider the above mentioned tasks part of the "public API" of capistrano? Because, if you do, please read the following lines from the semver spec:

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes. Patch version MUST be reset to 0 when minor version is incremented.

Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. It MAY include minor and patch level changes. Patch and minor version MUST be reset to 0 when major version is incremented.

Given that 1. the above mentioned tasks and hooking into them are public API and given that 2. the 2.9.0 update indeed broke that API, a Major version number increase does look like the only option.

This may be nitpicking, and I personally find a Major version bump for a change that was kind of forced upon you by 3rd party a bit harsh, but your [arch, feat, patch] description is not what semver tries to do.

That being said, +1 to everything you said (minus the bit about semver, obviously)

And also: Thanks a lot for doing this :)

Lee, you probably already noticed that I tried to defend you here, but I have a minor quibble:

With semver this would not have happened.

Wrong, it's [architecture, feature, patch], a feature has changed. Not the architecture

Do you consider the above mentioned tasks part of the "public API" of capistrano? Because, if you do, please read the following lines from the semver spec:

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes. Patch version MUST be reset to 0 when minor version is incremented.

Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. It MAY include minor and patch level changes. Patch and minor version MUST be reset to 0 when major version is incremented.

Given that 1. the above mentioned tasks and hooking into them are public API and given that 2. the 2.9.0 update indeed broke that API, a Major version number increase does look like the only option.

This may be nitpicking, and I personally find a Major version bump for a change that was kind of forced upon you by 3rd party a bit harsh, but your [arch, feat, patch] description is not what semver tries to do.

That being said, +1 to everything you said (minus the bit about semver, obviously)

And also: Thanks a lot for doing this :)

@tvdeyen

This comment has been minimized.

Show comment
Hide comment
@tvdeyen

tvdeyen Feb 20, 2012

I am not aware of the changes in rake and why they are related to this. But @leehambley seems to have obvious reasons to do the renaming.

But why did we not get any deprecation warnings? Even in 2.9.0. I used 2.9.0 for months now without any problems.

Ok. The child fell into the well (like we germans used to say).

Let's move on.

It's no big deal to adjust our deploy scripts. Even if we have hooks that are in gems and we have to look what version of capistrano is being used now..... but hey, that's open source.

tvdeyen commented Feb 20, 2012

I am not aware of the changes in rake and why they are related to this. But @leehambley seems to have obvious reasons to do the renaming.

But why did we not get any deprecation warnings? Even in 2.9.0. I used 2.9.0 for months now without any problems.

Ok. The child fell into the well (like we germans used to say).

Let's move on.

It's no big deal to adjust our deploy scripts. Even if we have hooks that are in gems and we have to look what version of capistrano is being used now..... but hey, that's open source.

@tvdeyen

This comment has been minimized.

Show comment
Hide comment
@tvdeyen

tvdeyen Feb 20, 2012

and Capistrano will easily fail a deploy, without breaking anything in production if your symlink task misses.

As the change doesn't risk downtime, there's no easy way to make the transition, just deal with the pain now, and we'll be safer in the future.

Sorry this is not true. I had downtime today, because of that. Capistrano did not failed because of the missing hook. It deployed without any error and it was my customer who reported me that images where missing on his page. So this is critical.

But hey I fixed it. Everything cool.

tvdeyen commented Feb 20, 2012

and Capistrano will easily fail a deploy, without breaking anything in production if your symlink task misses.

As the change doesn't risk downtime, there's no easy way to make the transition, just deal with the pain now, and we'll be safer in the future.

Sorry this is not true. I had downtime today, because of that. Capistrano did not failed because of the missing hook. It deployed without any error and it was my customer who reported me that images where missing on his page. So this is critical.

But hey I fixed it. Everything cool.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Feb 20, 2012

Member

@tvdeyen There's a lot of stuff around online about how to test this stuff (but it's not pretty, I admit it) - In all my testing it was a "safe" failure, that was the deploy aborted when the symlink task wasn't correctly hooked, but I can imagine why that might not happen with images/etc.

@halfbyte Typically, I wouldn't consider deploy:symlink to be part of the public API, the way it's built internally deploy:symlink is part of the update transaction, Capistrano does it's own linking of assets (shared resources, the REVISION file, etc) at deploy:finalize_update which is a safer place to hook. Typically one hooks things on deploy:symlink which are required to bring the server up again (database.yml is a prime example), for something like @tvdeyen 's images, I'd really recommend hooking into deploy:finalize_update.

A word on semver, @halfbyte I think you might be right, it's difficult to say though, who really has the luxury of private APIs in Ruby land.

And, a final word, @tvdeyen coffee is on me next time you are in the Hamburger Innenstadt. Open source is tricky sometimes!

2.11.0 will roll today, with a warning, and a backwards compatible :symlink task which will raise the appropriate warning, but continue to work for the time being. Now that I better understand the use-cases, and have clarified on the expected use of the APIs, I think that's the sensible solution.

Member

leehambley commented Feb 20, 2012

@tvdeyen There's a lot of stuff around online about how to test this stuff (but it's not pretty, I admit it) - In all my testing it was a "safe" failure, that was the deploy aborted when the symlink task wasn't correctly hooked, but I can imagine why that might not happen with images/etc.

@halfbyte Typically, I wouldn't consider deploy:symlink to be part of the public API, the way it's built internally deploy:symlink is part of the update transaction, Capistrano does it's own linking of assets (shared resources, the REVISION file, etc) at deploy:finalize_update which is a safer place to hook. Typically one hooks things on deploy:symlink which are required to bring the server up again (database.yml is a prime example), for something like @tvdeyen 's images, I'd really recommend hooking into deploy:finalize_update.

A word on semver, @halfbyte I think you might be right, it's difficult to say though, who really has the luxury of private APIs in Ruby land.

And, a final word, @tvdeyen coffee is on me next time you are in the Hamburger Innenstadt. Open source is tricky sometimes!

2.11.0 will roll today, with a warning, and a backwards compatible :symlink task which will raise the appropriate warning, but continue to work for the time being. Now that I better understand the use-cases, and have clarified on the expected use of the APIs, I think that's the sensible solution.

@tvdeyen

This comment has been minimized.

Show comment
Hide comment
@tvdeyen

tvdeyen Feb 20, 2012

Ok, thanks for the coffee. I really appreciate this.

And I will change my images folder hooks to after:finalize_update like this seems to be lot saver. Thanks for that advice.

But I still vote for an 3.0.0 release not 2.11.0.

tvdeyen commented Feb 20, 2012

Ok, thanks for the coffee. I really appreciate this.

And I will change my images folder hooks to after:finalize_update like this seems to be lot saver. Thanks for that advice.

But I still vote for an 3.0.0 release not 2.11.0.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Feb 20, 2012

Member

Compromise, 2.11.0 will hold the fix from 2.10.0 (as the API changed, but 2.11.0 will be compatible with 2.x, but will warn on using deploy:symlink).

For me 3.0 would have to be a significant re-write, which I have in mind, but alas no free time at the moment.

Member

leehambley commented Feb 20, 2012

Compromise, 2.11.0 will hold the fix from 2.10.0 (as the API changed, but 2.11.0 will be compatible with 2.x, but will warn on using deploy:symlink).

For me 3.0 would have to be a significant re-write, which I have in mind, but alas no free time at the moment.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Feb 20, 2012

Member

And I will pull 2.10.0 from rubygems

Member

leehambley commented Feb 20, 2012

And I will pull 2.10.0 from rubygems

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Feb 20, 2012

Member

Also, I was mistaken about 2.9.0, the change wasn't documented anywhere, that I planned on removing deploy:symlink for compatibility with Rake 0.9x, my sincere apologies for suggesting it was.

Member

leehambley commented Feb 20, 2012

Also, I was mistaken about 2.9.0, the change wasn't documented anywhere, that I planned on removing deploy:symlink for compatibility with Rake 0.9x, my sincere apologies for suggesting it was.

@tvdeyen

This comment has been minimized.

Show comment
Hide comment
@tvdeyen

tvdeyen Feb 20, 2012

Good solution I think.

And thanks for all your hard work. I couldn't say how much life would be a struggle without capistrano. ❤️

tvdeyen commented Feb 20, 2012

Good solution I think.

And thanks for all your hard work. I couldn't say how much life would be a struggle without capistrano. ❤️

@pseidemann

This comment has been minimized.

Show comment
Hide comment
@pseidemann

pseidemann Feb 21, 2012

wouldn't it be nice when before and after raise an exception when they get a undefined task?

wouldn't it be nice when before and after raise an exception when they get a undefined task?

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Feb 21, 2012

Member

@pseidemann, I think there's an issue with that solution, which is dynamically generated tasks that people hook sooner in the lifecycle (e.g. before they exist)

I'm not finished repairing the deploy:symlink issue yet, but I'll push something new today with more information, if you're not doing already, follow @capistranorb on twitter.

Member

leehambley commented Feb 21, 2012

@pseidemann, I think there's an issue with that solution, which is dynamically generated tasks that people hook sooner in the lifecycle (e.g. before they exist)

I'm not finished repairing the deploy:symlink issue yet, but I'll push something new today with more information, if you're not doing already, follow @capistranorb on twitter.

@joe1chen

This comment has been minimized.

Show comment
Hide comment
@joe1chen

joe1chen Feb 22, 2012

I was also bit by this change... took me a while to figure out what had happened and why my app stopped deploying. I was using capistrano 2.11.1 with custom overrides in order to deploy via git (like heroku). In my case, capistrano stopped calling my custom symlink function and invoked the default create_symlink method, which failed because of my setup (in a git-style deployment current directory is not removed and re-symlinked).

Anyways, it's not hard to fix once you know the issue and some sort of deprecation warning as others have pointed out would have been helpful.

I was also bit by this change... took me a while to figure out what had happened and why my app stopped deploying. I was using capistrano 2.11.1 with custom overrides in order to deploy via git (like heroku). In my case, capistrano stopped calling my custom symlink function and invoked the default create_symlink method, which failed because of my setup (in a git-style deployment current directory is not removed and re-symlinked).

Anyways, it's not hard to fix once you know the issue and some sort of deprecation warning as others have pointed out would have been helpful.

mattbrictson pushed a commit to mattbrictson/capistrano that referenced this issue Aug 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment