Group finalize_update shell commands into one command #282

Merged
merged 1 commit into from Oct 1, 2012

Conversation

Projects
None yet
3 participants
@ndbroadbent
Contributor

ndbroadbent commented Sep 29, 2012

Individual runs had an overhead of ~500ms, so this saves me about 4 seconds per deploy.

@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Sep 30, 2012

Contributor

@ndbroadbent Looks good! Would you mind hardening the shell calls with the Shellwords#shellescape mixin and using -- to separate optional arguments from positional ones? I think that this PR should be the point at which we turn over a new leaf with regards to shell coding policy.

Contributor

carsomyr commented Sep 30, 2012

@ndbroadbent Looks good! Would you mind hardening the shell calls with the Shellwords#shellescape mixin and using -- to separate optional arguments from positional ones? I think that this PR should be the point at which we turn over a new leaf with regards to shell coding policy.

@ndbroadbent

This comment has been minimized.

Show comment Hide comment
@ndbroadbent

ndbroadbent Sep 30, 2012

Contributor

Ok, how about that? I think that seems like a good policy.

Contributor

ndbroadbent commented Sep 30, 2012

Ok, how about that? I think that seems like a good policy.

@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Sep 30, 2012

Contributor

@ndbroadbent Great work, and getting there! Some more nitpicks:

  1. Don't shell escape the directory string so soon. I know it's meant for conciseness; however, escaping too soon might throw off file and path processing routines (unlikely, but best be on the safe side).
  2. Shell escape latest_release.
  3. chmod and touch both take -- separators. Strange but true. There can be one after the -R.

Thanks for your contribution, and expect to hear from me on another thread.

Contributor

carsomyr commented Sep 30, 2012

@ndbroadbent Great work, and getting there! Some more nitpicks:

  1. Don't shell escape the directory string so soon. I know it's meant for conciseness; however, escaping too soon might throw off file and path processing routines (unlikely, but best be on the safe side).
  2. Shell escape latest_release.
  3. chmod and touch both take -- separators. Strange but true. There can be one after the -R.

Thanks for your contribution, and expect to hear from me on another thread.

@ndbroadbent

This comment has been minimized.

Show comment Hide comment
@ndbroadbent

ndbroadbent Sep 30, 2012

Contributor

Ah, I see, I was only escaping things that the user can configure in their deploy.rb, I didn't think it was important to escape things latest_release, and thus I didn't think the chmod command needed a -- either. Also, I don't think touch needs a --, since we are 100% in control of the stamp variable. But ok, I'll make all those changes :)

Contributor

ndbroadbent commented Sep 30, 2012

Ah, I see, I was only escaping things that the user can configure in their deploy.rb, I didn't think it was important to escape things latest_release, and thus I didn't think the chmod command needed a -- either. Also, I don't think touch needs a --, since we are 100% in control of the stamp variable. But ok, I'll make all those changes :)

@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Sep 30, 2012

Contributor

@ndbroadbent I don't know how enforceable this policy will be in the future -- just trying the all-out strategy for now. Thanks for bearing with me.

Contributor

carsomyr commented Sep 30, 2012

@ndbroadbent I don't know how enforceable this policy will be in the future -- just trying the all-out strategy for now. Thanks for bearing with me.

@ndbroadbent

This comment has been minimized.

Show comment Hide comment
@ndbroadbent

ndbroadbent Sep 30, 2012

Contributor

Sure, no problem! Ok, have made those changes

Contributor

ndbroadbent commented Sep 30, 2012

Sure, no problem! Ok, have made those changes

@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Sep 30, 2012

Contributor

@ndbroadbent Do you think it would be better to move the require "shellwords" into the deploy.rb file?

Contributor

carsomyr commented Sep 30, 2012

@ndbroadbent Do you think it would be better to move the require "shellwords" into the deploy.rb file?

Group finalize_update shell commands into one command, harden shell c…
…alls with #shellescape, and separate arguments with --
@ndbroadbent

This comment has been minimized.

Show comment Hide comment
@ndbroadbent

ndbroadbent Sep 30, 2012

Contributor

Right, that does make more sense! The intention was to go for a 'global' require, but yeah, deploy.rb will also be loaded every time. Pushed

Contributor

ndbroadbent commented Sep 30, 2012

Right, that does make more sense! The intention was to go for a 'global' require, but yeah, deploy.rb will also be loaded every time. Pushed

carsomyr added a commit that referenced this pull request Oct 1, 2012

Merge pull request #282 from ndbroadbent/group_finalize_update_commands
Group finalize_update shell commands into one command

@carsomyr carsomyr merged commit 483d253 into capistrano:master Oct 1, 2012

1 check passed

default The Travis build passed
Details
@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Oct 1, 2012

Contributor

Merged, thanks!

Contributor

carsomyr commented Oct 1, 2012

Merged, thanks!

@felixbuenemann

This comment has been minimized.

Show comment Hide comment
@felixbuenemann

felixbuenemann Feb 16, 2013

Contributor

This actually breaks deployments, if your release path is relative to the users home (e.g. set :deploy_to, "~/releases"), because ~ gets escaped to \\~. It is also incomplete, because e.g. shared_path is not escaped.

Found this issue to be covered in #300.

Contributor

felixbuenemann commented on 483d253 Feb 16, 2013

This actually breaks deployments, if your release path is relative to the users home (e.g. set :deploy_to, "~/releases"), because ~ gets escaped to \\~. It is also incomplete, because e.g. shared_path is not escaped.

Found this issue to be covered in #300.

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