Merge branch 'kpaulisse-parallel-tmpdir-cleans' into kpaulisse-mega-merge

kpaulisse · kpaulisse · commit 6bd73dde8f36 · 2017-10-15T18:28:31.000-05:00
diff --git a/README.md b/README.md
@@ -53,6 +53,7 @@ The example above reflects the changes in the Puppet catalog from switching an u
 - [Requirements](/doc/requirements.md)
 - [Limitations](/doc/limitations.md)
 - [List of all command line options](/doc/optionsref.md)
+- [Environment variables](/doc/advanced-environment-variables.md)
 
 ### Project
 
diff --git a/doc/advanced-environment-variables.md b/doc/advanced-environment-variables.md
@@ -0,0 +1,70 @@
+# Environment variables
+
+The following environment variables have special meaning to octocatalog-diff:
+
+### `OCTOCATALOG_DIFF_CONFIG_FILE`
+
+This environment variable is used to locate the configuration file for the CLI. The use of configuration files is described generally in:
+
+- [Configuration](/doc/configuration.md)
+
+### `OCTOCATALOG_DIFF_CUSTOM_VERSION`
+
+When set, the `octocatalog-diff` CLI will display this as the version number within debugging, instead of the version number in the package. This is most useful if you want to use or include a git SHA in the version number.
+
+```
+$ export OCTOCATALOG_DIFF_CUSTOM_VERSION="@$(git rev-parse HEAD)"
+$ octocatalog-diff -d ...
+D, [2017-10-12T08:57:46.454738 #35205] DEBUG -- : Running octocatalog-diff @504d7f3c91267e5193beb103caae5d4d8cebfee3 with ruby 2.3.1
+...
+```
+
+### `OCTOCATALOG_DIFF_DEVELOPER_PATH`
+
+When set, instead of loading libraries from the system or bundler location, libraries will be loaded from the specified value of this environment variable. This is used internally for development as we point users to unreleased code for debugging or testing.
+
+```
+$ export OCTOCATALOG_DIFF_DEVELOPER_PATH=$HOME/git-checkouts/octocatalog-diff
+$ octocatalog-diff ...
+```
+
+### `OCTOCATALOG_DIFF_TEMPDIR`
+
+When set:
+
+- `octocatalog-diff` will create all of its temporary directories within the specified directory.
+- `octocatalog-diff` will not attempt to remove any temporary directories it creates.
+
+This is useful in the following situations:
+
+- You are calling `octocatalog-diff` from within another program which is highly parallelized, and `at_exit` handlers are difficult to implement. Instead of figuring that all out (if it can even be figured out), you create a temporary directory before the parallelized logic, and remove it afterwards.
+
+- You wish to debug intermediate output. For example, you may be instructed to set this variable and send some of the output to the project maintainers if you request assistance.
+
+This variable is used internally for the parallelized logic for catalog compilation, but the value set from the environment will override any internal usage.
+
+### `OCTOCATALOG_DIFF_VERSION`
+
+This variable is used when building the gem, to override the default version. This is used for internal testing of `octocatalog-diff` before public releases. This variable is not useful outside the build context.
+
+### `PUPPETDB_HOST`
+
+This variable specifies the fully qualified domain name or IP address of the PuppetDB server.
+
+Note: If `PUPPETDB_URL` is specified, then `PUPPETDB_HOST` is not consulted.
+
+### `PUPPETDB_PORT`
+
+This variable specifies the port number of the PuppetDB server.
+
+Note: If `PUPPETDB_URL` is specified, then `PUPPETDB_PORT` is not consulted.
+
+### `PUPPETDB_URL`
+
+This variable specifies the URL to the PuppetDB server.
+
+Example: `https://puppetdb.example.net:8081`
+
+### `PUPPET_FACT_DIR`
+
+This variable specifies the directory path where puppet fact files are stored. (Fact files must be named `<fqdn>.yaml` where `<fqdn>` is specified when running `octocatalog-diff`.)
diff --git a/lib/octocatalog-diff/catalog-util/builddir.rb b/lib/octocatalog-diff/catalog-util/builddir.rb
@@ -4,6 +4,7 @@
 
 require_relative '../facts'
 require_relative 'enc'
+require_relative '../util/util'
 
 module OctocatalogDiff
   module CatalogUtil
@@ -38,9 +39,7 @@ class BuildDir
       # @param options [Hash] Options for class; see above description
       def initialize(options = {}, logger = nil)
         @options = options.dup
-        @tempdir = Dir.mktmpdir('ocd-builddir-')
-        at_exit { FileUtils.rm_rf(@tempdir) if File.directory?(@tempdir) }
-
+        @tempdir = OctocatalogDiff::Util::Util.temp_dir('ocd-builddir-')
         @factdir = nil
         @enc = nil
         @fact_file = nil
diff --git a/lib/octocatalog-diff/catalog/computed.rb b/lib/octocatalog-diff/catalog/computed.rb
@@ -11,6 +11,7 @@
 require_relative '../catalog-util/facts'
 require_relative '../util/puppetversion'
 require_relative '../util/scriptrunner'
+require_relative '../util/util'
 
 module OctocatalogDiff
   class Catalog
@@ -69,23 +70,6 @@ def convert_file_resources(dry_run = false)
         OctocatalogDiff::CatalogUtil::FileResources.convert_file_resources(self, environment)
       end
 
-      private
-
-      # Private method: Clean up a checkout directory, if it exists
-      def cleanup_checkout_dir(checkout_dir, logger)
-        return unless File.directory?(checkout_dir)
-        logger.debug("Cleaning up temporary directory #{checkout_dir}")
-        # Sometimes this seems to break when handling the recursive removal when running under
-        # a parallel environment. Trap and ignore the errors here if we don't care about them.
-        begin
-          FileUtils.remove_entry_secure checkout_dir
-          # :nocov:
-        rescue Errno::ENOTEMPTY, Errno::ENOENT => exc
-          logger.debug "cleanup_checkout_dir(#{checkout_dir}) logged #{exc.class} - this can be ignored"
-          # :nocov:
-        end
-      end
-
       # Private method: Bootstrap a directory
       def bootstrap(logger)
         return if @builddir
@@ -99,9 +83,7 @@ def bootstrap(logger)
           tmphash[:basedir] = @options[:bootstrapped_dir]
         elsif @options[:branch] == '.'
           if @options[:bootstrap_current]
-            tmphash[:basedir] =  Dir.mktmpdir('ocd-bootstrap-basedir-')
-            at_exit { cleanup_checkout_dir(tmphash[:basedir], logger) }
-
+            tmphash[:basedir] = OctocatalogDiff::Util::Util.temp_dir('ocd-bootstrap-basedir-')
             FileUtils.cp_r File.join(@options[:basedir], '.'), tmphash[:basedir]
 
             o = @options.reject { |k, _v| k == :branch }.merge(path: tmphash[:basedir])
@@ -110,10 +92,8 @@ def bootstrap(logger)
             tmphash[:basedir] = @options[:basedir]
           end
         else
-          checkout_dir = Dir.mktmpdir('ocd-bootstrap-checkout-')
-          at_exit { cleanup_checkout_dir(checkout_dir, logger) }
-          tmphash[:basedir] = checkout_dir
-          OctocatalogDiff::CatalogUtil::Bootstrap.bootstrap_directory(@options.merge(path: checkout_dir), logger)
+          tmphash[:basedir] = OctocatalogDiff::Util::Util.temp_dir('ocd-bootstrap-checkout-')
+          OctocatalogDiff::CatalogUtil::Bootstrap.bootstrap_directory(@options.merge(path: tmphash[:basedir]), logger)
         end
 
         # Create and populate the temporary directory
diff --git a/lib/octocatalog-diff/util/parallel.rb b/lib/octocatalog-diff/util/parallel.rb
@@ -6,6 +6,7 @@
 # this instead executes the tasks serially, but provides the same API as the parallel tasks.
 
 require 'stringio'
+require_relative 'util'
 
 module OctocatalogDiff
   module Util
@@ -107,13 +108,14 @@ def self.run_tasks(task_array, logger = nil, parallelized = true, raise_exceptio
       # @return [Exception] First exception encountered by a child process; returns nil if no exceptions encountered.
       def self.run_tasks_parallel(result, task_array, logger)
         pidmap = {}
-        ipc_tempdir = Dir.mktmpdir('ocd-ipc-')
+        ipc_tempdir = OctocatalogDiff::Util::Util.temp_dir('ocd-ipc-')
 
         # Child process forking
         task_array.each_with_index do |task, index|
           # simplecov doesn't see this because it's forked
           # :nocov:
           this_pid = fork do
+            ENV['OCTOCATALOG_DIFF_TEMPDIR'] ||= ipc_tempdir
             task_result = execute_task(task, logger)
             File.open(File.join(ipc_tempdir, "#{Process.pid}.dat"), 'w') { |f| f.write Marshal.dump(task_result) }
             Kernel.exit! 0 # Kernel.exit! avoids at_exit from parents being triggered by children exiting
@@ -157,17 +159,6 @@ def self.run_tasks_parallel(result, task_array, logger)
             # If the process doesn't exist, that's fine.
           end
         end
-
-        retries = 0
-        while File.directory?(ipc_tempdir) && retries < 10
-          retries += 1
-          begin
-            FileUtils.remove_entry_secure ipc_tempdir
-          rescue Errno::ENOTEMPTY, Errno::ENOENT # rubocop:disable Lint/HandleExceptions
-            # Errno::ENOTEMPTY will trigger a retry because the directory exists
-            # Errno::ENOENT will break the loop because the directory won't exist next time it's checked
-          end
-        end
       end
 
       # Utility method! Not intended to be called from outside this class.
diff --git a/lib/octocatalog-diff/util/scriptrunner.rb b/lib/octocatalog-diff/util/scriptrunner.rb
@@ -6,6 +6,8 @@
 require 'open3'
 require 'shellwords'
 
+require_relative 'util'
+
 module OctocatalogDiff
   module Util
     # This is a utility class to execute a built-in script.
@@ -91,14 +93,7 @@ def log(priority, message, logger = @logger)
       # @return [String] Path to tempfile containing script
       def temp_script(script)
         raise Errno::ENOENT, "Script '#{script}' not found" unless File.file?(script)
-        temp_dir = Dir.mktmpdir('ocd-scriptrunner-')
-        at_exit do
-          begin
-            FileUtils.remove_entry_secure temp_dir
-          rescue Errno::ENOENT # rubocop:disable Lint/HandleExceptions
-            # OK if the directory doesn't exist since we're trying to remove it anyway
-          end
-        end
+        temp_dir = OctocatalogDiff::Util::Util.temp_dir('ocd-scriptrunner')
         temp_file = File.join(temp_dir, File.basename(script))
         File.open(temp_file, 'w') { |f| f.write(File.read(script)) }
         FileUtils.chmod 0o755, temp_file
diff --git a/lib/octocatalog-diff/util/util.rb b/lib/octocatalog-diff/util/util.rb
@@ -2,6 +2,8 @@
 
 # Handy methods that are not tied to one particular class
 
+require 'fileutils'
+
 module OctocatalogDiff
   module Util
     # Helper class to construct catalogs, performing all necessary steps such as
@@ -43,6 +45,51 @@ def self.deep_dup(object)
           safe_dup(object)
         end
       end
+
+      # Utility Method!
+      # This creates a temporary directory. If the base directory is specified, then we
+      # do not remove the temporary directory at exit, because we assume that something
+      # else will remove the base directory.
+      #
+      # prefix  - A String with the prefix for the temporary directory
+      # basedir - A String with the directory in which to make the tempdir
+      #
+      # Returns the full path to the temporary directory.
+      def self.temp_dir(prefix = 'ocd-', basedir = ENV['OCTOCATALOG_DIFF_TEMPDIR'])
+        # If the base directory is specified, make sure it exists, and then create the
+        # temporary directory within it.
+        if basedir
+          unless File.directory?(basedir)
+            raise Errno::ENOENT, "temp_dir: Base dir #{basedir.inspect} does not exist!"
+          end
+          return Dir.mktmpdir(prefix, basedir)
+        end
+
+        # If the base directory was not specified, then create a temporary directory, and
+        # send the `at_exit` to clean it up at the conclusion.
+        the_dir = Dir.mktmpdir(prefix)
+        at_exit { remove_temp_dir(the_dir) }
+        the_dir
+      end
+
+      # Utility method!
+      # Remove a directory recursively that has been used as a temporary directory. This
+      # should be called within an `at_exit` handler, and is only intended to be called via the
+      # `temp_dir` method above.
+      #
+      # dir - A String with the directory to remove.
+      def self.remove_temp_dir(dir)
+        retries = 0
+        while File.directory?(dir) && retries < 10
+          retries += 1
+          begin
+            FileUtils.remove_entry_secure(dir)
+          rescue Errno::ENOTEMPTY, Errno::ENOENT # rubocop:disable Lint/HandleExceptions
+            # Errno::ENOTEMPTY will trigger a retry because the directory exists
+            # Errno::ENOENT will break the loop because the directory won't exist next time it's checked
+          end
+        end
+      end
     end
   end
 end
diff --git a/spec/octocatalog-diff/tests/catalog/computed_spec.rb b/spec/octocatalog-diff/tests/catalog/computed_spec.rb
@@ -292,30 +292,6 @@
     end
   end
 
-  describe '#cleanup_checkout_dir' do
-    it 'should remove a directory if one exists' do
-      opts = { node: 'foo', branch: 'bar', bootstrapped_dir: OctocatalogDiff::Spec.fixture_path('null') }
-      obj = OctocatalogDiff::Catalog::Computed.new(opts)
-      logger, _logger_str = OctocatalogDiff::Spec.setup_logger
-      begin
-        dir = Dir.mktmpdir
-        expect(File.directory?(dir)).to eq(true)
-        obj.send(:cleanup_checkout_dir, dir, logger)
-        expect(File.directory?(dir)).to eq(false)
-      ensure
-        OctocatalogDiff::Spec.clean_up_tmpdir(dir)
-      end
-    end
-
-    it 'should not error if a directory does not exist' do
-      opts = { node: 'foo', branch: 'bar', bootstrapped_dir: OctocatalogDiff::Spec.fixture_path('null') }
-      obj = OctocatalogDiff::Catalog::Computed.new(opts)
-      logger, _logger_str = OctocatalogDiff::Spec.setup_logger
-      dir = OctocatalogDiff::Spec.fixture_path('null')
-      expect { obj.send(:cleanup_checkout_dir, dir, logger) }.not_to raise_error
-    end
-  end
-
   describe '#assert_that_puppet_environment_directory_exists' do
     before(:each) do
       allow(File).to receive(:"directory?").with('/tmp/assert/environments/yup').and_return(true)
diff --git a/spec/octocatalog-diff/tests/util/parallel_spec.rb b/spec/octocatalog-diff/tests/util/parallel_spec.rb
@@ -5,14 +5,14 @@
 require 'logger'
 require 'parallel'
 
-# rubocop:disable Style/GlobalVars
 describe OctocatalogDiff::Util::Parallel do
   before(:each) do
-    $octocatalog_diff_util_parallel_spec_tempdir = Dir.mktmpdir
+    ENV['OCTOCATALOG_DIFF_TEMPDIR'] = Dir.mktmpdir
   end
 
   after(:each) do
-    OctocatalogDiff::Spec.clean_up_tmpdir($octocatalog_diff_util_parallel_spec_tempdir)
+    OctocatalogDiff::Spec.clean_up_tmpdir(ENV['OCTOCATALOG_DIFF_TEMPDIR'])
+    ENV.delete('OCTOCATALOG_DIFF_TEMPDIR')
   end
 
   context 'with parallel processing' do
@@ -50,13 +50,13 @@ def two(arg, _logger = nil)
     it 'should handle a task that fails after other successes' do
       class Foo
         def one(arg, _logger = nil)
-          File.open(File.join($octocatalog_diff_util_parallel_spec_tempdir, 'one'), 'w') { |f| f.write '' }
+          File.open(File.join(ENV['OCTOCATALOG_DIFF_TEMPDIR'], 'one'), 'w') { |f| f.write '' }
           'one ' + arg
         end
 
         def two(_arg, _logger = nil)
           100.times do
-            break if File.file?(File.join($octocatalog_diff_util_parallel_spec_tempdir, 'one'))
+            break if File.file?(File.join(ENV['OCTOCATALOG_DIFF_TEMPDIR'], 'one'))
             sleep 0.1
           end
           # Sometimes the system will still handle the second process if it's near-simultaneous
@@ -90,7 +90,7 @@ def two(_arg, _logger = nil)
       class Foo
         def one(arg, _logger = nil)
           sleep 10
-          File.open(File.join($octocatalog_diff_util_parallel_spec_tempdir, 'one'), 'w') { |f| f.write '' }
+          File.open(File.join(ENV['OCTOCATALOG_DIFF_TEMPDIR'], 'one'), 'w') { |f| f.write '' }
           'one ' + arg
         end
 
@@ -119,7 +119,7 @@ def two(_arg, _logger = nil)
       expect(two_result.exception).to be_a_kind_of(RuntimeError)
       expect(two_result.exception.message).to eq('Two failed')
 
-      expect(File.file?(File.join($octocatalog_diff_util_parallel_spec_tempdir, 'one'))).to eq(false)
+      expect(File.file?(File.join(ENV['OCTOCATALOG_DIFF_TEMPDIR'], 'one'))).to eq(false)
     end
 
     it 'should log debug messages' do
@@ -480,4 +480,3 @@ def validate(arg, _logger = nil, _extra_args = {})
     end
   end
 end
-# rubocop:enable Style/GlobalVars
diff --git a/spec/octocatalog-diff/tests/util/util_spec.rb b/spec/octocatalog-diff/tests/util/util_spec.rb
