For Review: Feature #1833 patch

[Jeffrey Hulten]

All,

It is my first patch, so be kind. ;)  Please let me know what you think.  This is in regards to http://projects.reductivelabs.com/issues/show/1833

Thanks,

Jeff

diff --git a/install.rb b/install.rb

index c99d7da..b0803a5 100755

--- a/install.rb

+++ b/install.rb

@@ -92,9 +92,11 @@ def do_libs(libs, strip = 'lib/')

   libs.each do |lf|

     olf = File.join(InstallOptions.site_dir, lf.gsub(/#{strip}/, ''))

     op = File.dirname(olf)

-    File.makedirs(op, true)

-    File.chmod(0755, op)

-    File.install(lf, olf, 0755, true)

+    if File.exists?(op)

+      File.makedirs(op, true)

+      File.chmod(0755, op)

+    end

+    install_libfile(lf, olf)

   end

 end

 

@@ -200,6 +202,12 @@ def prepare_installation

     opts.on('--mandir[=OPTIONAL]', 'Installation directory for man pages', 'overrides Config::CONFIG["mandir"]') do |mandir|

       InstallOptions.mandir = mandir

     end

+    opts.on('--confdir[=OPTIONAL]', 'Configuration directory', 'Default to /etc/puppet') do |confdir|

+      InstallOptions.confdir = confdir

+    end

+    opts.on('--vardir[=OPTIONAL]', 'Data directory', 'Default to /var/puppet') do |vardir|

+      InstallOptions.vardir = vardir

+    end

     opts.on('--quick', 'Performs a quick installation. Only the', 'installation is done.') do |quick|

       InstallOptions.rdoc   = false

       InstallOptions.ri     = false

@@ -245,7 +253,19 @@ def prepare_installation

   else

     sbindir = Config::CONFIG['sbindir']

   end

-  

+

+  if not InstallOptions.vardir.nil?

+    vardir = InstallOptions.vardir

+  else

+    vardir = '/var/puppet'

+  end

+

+  if not InstallOptions.confdir.nil?

+    confdir = InstallOptions.confdir

+  else

+    confdir = '/etc/puppet'

+  end

+

   if not InstallOptions.sitelibdir.nil?

     sitelibdir = InstallOptions.sitelibdir

   else

@@ -296,6 +316,8 @@ def prepare_installation

   InstallOptions.site_dir = sitelibdir

   InstallOptions.bin_dir  = bindir

   InstallOptions.sbin_dir = sbindir

+  InstallOptions.var_dir = vardir

+  InstallOptions.conf_dir = confdir

   InstallOptions.lib_dir  = libdir

   InstallOptions.man_dir  = mandir

 end

@@ -434,6 +456,34 @@ def install_binfile(from, op_file, target)

   File.unlink(tmp_file)

 end

 

+##

+# Install file(s) from ./lib to InstallOptions.site_dir.

+def install_libfile(from, op_file)

+  tmp_dir = nil

+  InstallOptions.tmp_dirs.each do |t|

+    if File.directory?(t) and File.writable?(t)

+      tmp_dir = t

+      break

+    end

+  end

+

+  fail "Cannot find a temporary directory" unless tmp_dir

+  tmp_file = File.join(tmp_dir, '_tmp')

+

+  File.open(from) do |ip|

+    File.open(tmp_file, "w") do |op|

+      contents = ip.readlines

+      contents = contents.join()

+      contents = contents.gsub(/\/var\/puppet/, InstallOptions.var_dir)

+      contents = contents.gsub(/\/etc\/puppet/, InstallOptions.conf_dir)

+      op.write

+    end

+  end

+  FileUtils.install(tmp_file, File.join(InstallOptions.site_dir, op_file), :mode => 0644, :verbose => true)

+  File.unlink(tmp_file)

+end

+

+

 CMD_WRAPPER = <<-EOS

 @echo off

 if "%OS%"=="Windows_NT" goto WinNT

[Jeffrey Hulten]

I may have spoken too soon. It would help if it ran properly...  Let me know what you think of the approach and I will clean up the implementation.

Jeff

[Jeffrey Hulten]

All,

Premature patch emailing...  Must be a function of old age.  This one passes the 'does it run' test.

Thanks,

Jeff

diff --git a/install.rb b/install.rb

old mode 100755

new mode 100644

index c99d7da..f19aa94

--- a/install.rb

+++ b/install.rb

@@ -92,9 +92,11 @@ def do_libs(libs, strip = 'lib/')

   libs.each do |lf|

     olf = File.join(InstallOptions.site_dir, lf.gsub(/#{strip}/, ''))

     op = File.dirname(olf)

-    File.makedirs(op, true)

-    File.chmod(0755, op)

-    File.install(lf, olf, 0755, true)

+    if not File.exists?(op)

+      op.write(contents)

+    end

+  end

+  FileUtils.install(tmp_file, op_file, :mode => 0644, :verbose => true)

+  File.unlink(tmp_file)

+end

+

+

 CMD_WRAPPER = <<-EOS

 @echo off

 if "%OS%"=="Windows_NT" goto WinNT

[Luke Kanies]

This would be a bit cleaner like this:

tmp_file = Tempfile.new("puppet-config")

File.open(from) do |ip|
tmp_file.open("w") do |tmp| # I think this is how it works

ip.readlines.each do |line|
tmp.print line.gsub("/var/puppet/",
InstallOptions.var_dir).gsub("/etc/puppet/", InstallOptions.conf_dir)
end
end
end

It's slightly uglier having both gsubs on one line, but otherwise you
have to use gsub! (gsub, w/o the bang, returns the new value, rather
than modifying the original value). And there's no real reason to use
regexes, with their leaning toothpicks, in this case.

Otherwise, the patch seems fine, although the install.rb file is such
a piece of nastiness it's hard to say. :/

> + end
> + end
> + FileUtils.install(tmp_file, op_file, :mode => 0644, :verbose =>
> true)
> + File.unlink(tmp_file)
> +end
> +
> +
> CMD_WRAPPER = <<-EOS
> @echo off
> if "%OS%"=="Windows_NT" goto WinNT

--
An ounce of action is worth a ton of theory. --Friedrich Engels
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

[Jeffrey Hulten]

Okay. Is the correct next step to submit a pull request via GitHub?

I will also follow up with some cleanup to install.rb (I saw some areas that could be clearer and some of the ugliness you saw was copied from install_binfile).

Thanks,

Jeff

[Luke Kanies]

If the code is indicated as accepted on the list, then just change the
ticket to 'ready for checkin', or (more likely) 'ready for testing'.

--
The surest sign that intelligent life exists elsewhere in the universe
is that it has never tried to contact us.
--Calvin and Hobbes (Bill Watterson)