Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Copy stratery refactoring #185

Merged
merged 26 commits into from

3 participants

@despo

refactoring copy strategy to make more readable

despo added some commits
@despo despo extract refresh local cache code to its own method 3d30635
@despo despo move local cache creation to its own method 6285849
@despo despo extract rollback changes to its own method d03da71
@despo despo remove duplication 6e17f9f
@despo despo move copy of cache to server to its own method 8586954
@despo despo extract code to copy_repository_to_server 7897b46
@despo despo extract to remove_excluded_files method b534177
@despo despo extract to create_revision_file 7ed118a
@despo despo extract code to compress_repository 9e28169
@despo despo remove space 9c83fce
@despo despo rename rollback_changes to raise_command_failed
and refactored cleanup to new rollback_changes method
8a12de6
@despo despo extract code to copy_repository_to_local_cache bc310a7
@despo despo rename method to be more meaningful 5ba5da3
@despo despo remove spaces fe76b89
@despo despo separate copying cache to more readable methods c789f99
@despo despo extract to methods
to make copy cache process more readable
1c1d3c3
@despo despo remove spaces 2c60bd8
@despo despo rename filename to name
so there is no confusion with the filename method
d20acaf
@despo despo add logging before build script is executed b8a9b18
@despo despo extracted logging and system command execution
to method
6f12a73
@despo despo remove duplication 2386734
@despo despo rename build method to more meaningful name 41af69c
@despo despo extract copy and copy cache strategies to…
 their own methods

change file processing to use File.ftype method and change tests to reflect that
5752213
@despo despo ensure filetype matches method name 290e281
@despo despo changing if else to ternary statement..
 so it reads better
35ab129
@leehambley
Owner

@despo what provision is made so that people relying on monkey patching the existing copy strategy can continue to use their code?

The list has spoken a few times about refactoring this, but decided that actually, we need to maintain a stable API.

@despo

@leehambley I wasn't aware of the discussion in the list and I refactored in an attempt to understand what goes on in the copy strategy.

I have renamed run_build_script back to build so that it doesn't impact anyone who has overridden the method.
As I have only extracted the deploy method to smaller private methods, I'm not sure how this would impact any monkey patching. Then again, I can't be sure...

Keeping hold of code just for the sake of not breaking existing build scripts doesn't make a lot of sense. Can't the version be bumped and a warning or update be issued instead? The code is a lot more readable now and if anyone decides to update, they should easily be able to apply any changes. (Plus anyone wanting to use the existing copy strategy should now be able to extend it easier without having to patch deploy/build or copy cache)

@dfreudenberger

+1 on this - just tried to read through the existing code which isn't really easy

@despo

Any suggestions on how to improve this code, so the request can make it in, are more than welcome.

@leehambley
Owner
@leehambley leehambley merged commit c8139bf into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 16, 2012
  1. @despo
  2. @despo
  3. @despo
  4. @despo

    remove duplication

    despo authored
  5. @despo
  6. @despo
  7. @despo
  8. @despo

    extract to create_revision_file

    despo authored
  9. @despo
  10. @despo

    remove space

    despo authored
  11. @despo

    rename rollback_changes to raise_command_failed

    despo authored
    and refactored cleanup to new rollback_changes method
  12. @despo
  13. @despo
  14. @despo

    remove spaces

    despo authored
Commits on Mar 18, 2012
  1. @despo
  2. @despo

    extract to methods

    despo authored
    to make copy cache process more readable
  3. @despo

    remove spaces

    despo authored
  4. @despo

    rename filename to name

    despo authored
    so there is no confusion with the filename method
  5. @despo
  6. @despo

    extracted logging and system command execution

    despo authored
    to method
  7. @despo

    remove duplication

    despo authored
  8. @despo
  9. @despo

    extract copy and copy cache strategies to…

    despo authored
     their own methods
    
    change file processing to use File.ftype method and change tests to reflect that
  10. @despo
  11. @despo

    changing if else to ternary statement..

    despo authored
     so it reads better
Commits on Mar 19, 2012
  1. @despo

    rename run_build_script to build

    despo authored
This page is out of date. Refresh to see the latest.
View
6 lib/capistrano/recipes/deploy/strategy/base.rb
@@ -34,7 +34,7 @@ def check!
d.remote.writable(configuration[:releases_path]).or("You do not have permissions to write to `#{configuration[:releases_path]}'.")
end
end
-
+
protected
# This is to allow helper methods like "run" and "put" to be more
@@ -52,7 +52,7 @@ def system(*args)
cmd = args.join(' ')
result = nil
if RUBY_PLATFORM =~ /win32/
- cmd = cmd.split(/\s+/).collect {|w| w.match(/^[\w+]+:\/\//) ? w : w.gsub('/', '\\') }.join(' ') # Split command by spaces, change / by \\ unless element is a some+thing://
+ cmd = cmd.split(/\s+/).collect {|w| w.match(/^[\w+]+:\/\//) ? w : w.gsub('/', '\\') }.join(' ') # Split command by spaces, change / by \\ unless element is a some+thing://
cmd.gsub!(/^cd /,'cd /D ') # Replace cd with cd /D
cmd.gsub!(/&& cd /,'&& cd /D ') # Replace cd with cd /D
logger.trace "executing locally: #{cmd}"
@@ -65,7 +65,7 @@ def system(*args)
result = super
end
end
-
+
logger.trace "command finished in #{(elapsed * 1000).round}ms"
result
end
View
223 lib/capistrano/recipes/deploy/strategy/copy.rb
@@ -54,83 +54,19 @@ class Copy < Base
# servers, and uncompresses it on each of them into the deployment
# directory.
def deploy!
- if copy_cache
- if File.exists?(copy_cache)
- logger.debug "refreshing local cache to revision #{revision} at #{copy_cache}"
- system(source.sync(revision, copy_cache))
- else
- logger.debug "preparing local cache at #{copy_cache}"
- system(source.checkout(revision, copy_cache))
- end
-
- # Check the return code of last system command and rollback if not 0
- unless $? == 0
- raise Capistrano::Error, "shell command failed with return code #{$?}"
- end
-
- build(copy_cache)
-
- FileUtils.mkdir_p(destination)
-
- logger.debug "copying cache to deployment staging area #{destination}"
- Dir.chdir(copy_cache) do
- queue = Dir.glob("*", File::FNM_DOTMATCH)
- while queue.any?
- item = queue.shift
- name = File.basename(item)
-
- next if name == "." || name == ".."
- next if copy_exclude.any? { |pattern| File.fnmatch(pattern, item) }
-
- if File.symlink?(item)
- FileUtils.ln_s(File.readlink(item), File.join(destination, item))
- elsif File.directory?(item)
- queue += Dir.glob("#{item}/*", File::FNM_DOTMATCH)
- FileUtils.mkdir(File.join(destination, item))
- else
- FileUtils.ln(item, File.join(destination, item))
- end
- end
- end
- else
- logger.debug "getting (via #{copy_strategy}) revision #{revision} to #{destination}"
- system(command)
-
- build(destination)
-
- if copy_exclude.any?
- logger.debug "processing exclusions..."
-
- copy_exclude.each do |pattern|
- delete_list = Dir.glob(File.join(destination, pattern), File::FNM_DOTMATCH)
- # avoid the /.. trap that deletes the parent directories
- delete_list.delete_if { |dir| dir =~ /\/\.\.$/ }
- FileUtils.rm_rf(delete_list.compact)
- end
- end
- end
-
- File.open(File.join(destination, "REVISION"), "w") { |f| f.puts(revision) }
-
- logger.trace "compressing #{destination} to #{filename}"
- Dir.chdir(copy_dir) { system(compress(File.basename(destination), File.basename(filename)).join(" ")) }
+ copy_cache ? run_copy_cache_strategy : run_copy_strategy
+ create_revision_file
+ compress_repository
distribute!
ensure
- FileUtils.rm filename rescue nil
- FileUtils.rm_rf destination rescue nil
+ rollback_changes
end
- def build(directory)
- return unless configuration[:build_script]
-
- Dir.chdir(directory) do
- self.system(configuration[:build_script])
- # Check the return code of last system command and rollback if not 0
- unless $? == 0
- raise Capistrano::Error, "shell command failed with return code #{$?}"
- end
- end
+ def build directory
+ execute "running build script on #{directory}" do
+ Dir.chdir(directory) { system(build_script) }
+ end if build_script
end
def check!
@@ -153,6 +89,143 @@ def copy_cache
private
+ def run_copy_cache_strategy
+ copy_repository_to_local_cache
+ build copy_cache
+ copy_cache_to_staging_area
+ end
+
+ def run_copy_strategy
+ copy_repository_to_server
+ build destination
+ remove_excluded_files if copy_exclude.any?
+ end
+
+ def execute description, &block
+ logger.debug description
+ handle_system_errors &block
+ end
+
+ def handle_system_errors &block
+ block.call
+ raise_command_failed if last_command_failed?
+ end
+
+ def refresh_local_cache
+ execute "refreshing local cache to revision #{revision} at #{copy_cache}" do
+ system(source.sync(revision, copy_cache))
+ end
+ end
+
+ def create_local_cache
+ execute "preparing local cache at #{copy_cache}" do
+ system(source.checkout(revision, copy_cache))
+ end
+ end
+
+ def raise_command_failed
+ raise Capistrano::Error, "shell command failed with return code #{$?}"
+ end
+
+ def last_command_failed?
+ $? != 0
+ end
+
+ def copy_cache_to_staging_area
+ execute "copying cache to deployment staging area #{destination}" do
+ create_destination
+ Dir.chdir(copy_cache) { copy_files(queue_files) }
+ end
+ end
+
+ def create_destination
+ FileUtils.mkdir_p(destination)
+ end
+
+ def copy_files files
+ files.each { |name| process_file(name) }
+ end
+
+ def process_file name
+ send "copy_#{filetype(name)}", name
+ end
+
+ def filetype name
+ filetype = File.ftype name
+ filetype = "file" unless ["link", "directory"].include? filetype
+ filetype
+ end
+
+ def copy_link name
+ FileUtils.ln_s(File.readlink(name), File.join(destination, name))
+ end
+
+ def copy_directory name
+ FileUtils.mkdir(File.join(destination, name))
+ copy_files(queue_files(name))
+ end
+
+ def copy_file name
+ FileUtils.ln(name, File.join(destination, name))
+ end
+
+ def queue_files directory=nil
+ Dir.glob(pattern_for(directory), File::FNM_DOTMATCH).reject! { |file| excluded_files_contain? file }
+ end
+
+ def pattern_for directory
+ !directory.nil? ? "#{directory}/*" : "*"
+ end
+
+ def excluded_files_contain? file
+ copy_exclude.any? { |p| File.fnmatch(p, file) } or [ ".", ".."].include? File.basename(file)
+ end
+
+ def copy_repository_to_server
+ execute "getting (via #{copy_strategy}) revision #{revision} to #{destination}" do
+ copy_repository_via_strategy
+ end
+ end
+
+ def copy_repository_via_strategy
+ system(command)
+ end
+
+ def remove_excluded_files
+ logger.debug "processing exclusions..."
+
+ copy_exclude.each do |pattern|
+ delete_list = Dir.glob(File.join(destination, pattern), File::FNM_DOTMATCH)
+ # avoid the /.. trap that deletes the parent directories
+ delete_list.delete_if { |dir| dir =~ /\/\.\.$/ }
+ FileUtils.rm_rf(delete_list.compact)
+ end
+ end
+
+ def create_revision_file
+ File.open(File.join(destination, "REVISION"), "w") { |f| f.puts(revision) }
+ end
+
+ def compress_repository
+ execute "Compressing #{destination} to #{filename}" do
+ Dir.chdir(copy_dir) { system(compress(File.basename(destination), File.basename(filename)).join(" ")) }
+ end
+ end
+
+ def rollback_changes
+ FileUtils.rm filename rescue nil
+ FileUtils.rm_rf destination rescue nil
+ end
+
+ def copy_repository_to_local_cache
+ return refresh_local_cache if File.exists?(copy_cache)
+ create_local_cache
+ end
+
+ def build_script
+ configuration[:build_script]
+ end
+
# Specify patterns to exclude from the copy. This is only valid
# when using a local cache.
def copy_exclude
@@ -237,10 +310,14 @@ def decompress(file)
compression.decompress_command + [file]
end
+ def decompress_remote_file
+ run "cd #{configuration[:releases_path]} && #{decompress(remote_filename).join(" ")} && rm #{remote_filename}"
+ end
+
# Distributes the file to the remote servers
def distribute!
upload(filename, remote_filename)
- run "cd #{configuration[:releases_path]} && #{decompress(remote_filename).join(" ")} && rm #{remote_filename}"
+ decompress_remote_file
end
end
View
6 test/deploy/strategy/copy_test.rb
@@ -306,15 +306,15 @@ def test_with_build_script_should_run_script
def prepare_directory_tree!(cache, exclude=false)
Dir.expects(:glob).with("*", File::FNM_DOTMATCH).returns([".", "..", "app", "foo.txt"])
- File.expects(:directory?).with("app").returns(true)
+ File.expects(:ftype).with("app").returns("directory")
FileUtils.expects(:mkdir).with("/temp/dir/1234567890/app")
- File.expects(:directory?).with("foo.txt").returns(false)
+ File.expects(:ftype).with("foo.txt").returns("file")
FileUtils.expects(:ln).with("foo.txt", "/temp/dir/1234567890/foo.txt")
Dir.expects(:glob).with("app/*", File::FNM_DOTMATCH).returns(["app/.", "app/..", "app/bar.txt"])
unless exclude
- File.expects(:directory?).with("app/bar.txt").returns(false)
+ File.expects(:ftype).with("app/bar.txt").returns("file")
FileUtils.expects(:ln).with("app/bar.txt", "/temp/dir/1234567890/app/bar.txt")
end
end
Something went wrong with that request. Please try again.