(PDK-1113) Use PDK configuration instead of AnswerFile class

Previously the PDK had two ways to retrieve the same configuration information.
One via the PDK::Config classes and the other the AnswerFile class.  This should
only be one.  This commit removes the AnswerClass (and associated features) and
instead uses PDK::Config for setting management

* Removes the PDK::AnswerFile class, and instead has a single helper method for
  the default file location.  This may eventually be moved into the PDK::Config
  namespace as well

* Removes the hidden `--answer-file` command line option and instead uses an
  environment variable PDK_ANSWER_FILE.  This feature is not supported and is
  only used to acceptance testing.  Note that we moved to an environment
  variable as there was a subtle race condition where the CLI reads from
  PDK::Config, but the CLI also moves the user answer file location. By using
  an environment variable this race condition can no longer occur.

* Move setting reads from PDK.answers to PDK.config.pdk_setting(....)

* Move setting writes from PDK.answers.update! to
  PDK.config.user['module_defaults']['....'] =

* Modify tests for the setting read/write changes

* Added a Mock configuration RSpec Context to stop any test information
  affecting actual real setting files.  Previously module answer files were
  being modified

lib/pdk.rb

@@ -30,35 +30,22 @@ module Test
     autoload :Unit, 'pdk/tests/unit'
   end
 
-  # Singleton accessor to the current answer file being used by the PDK.
-  #
-  # @return [PDK::AnswerFile] The AnswerFile instance currently being used by
-  #   the PDK.
-  def self.answers
-    @answer_file ||= PDK::AnswerFile.new
-  end
-
-  # Specify the path to a custom answer file that the PDK should use.
-  #
-  # @param path [String] A path on disk to the file where the PDK should store
-  #   answers to interactive questions.
-  def self.answer_file=(path)
-    @answer_file = PDK::AnswerFile.new(path)
-  end
-
   def self.logger
     @logger ||= PDK::Logger.new
   end
 
   def self.config
-    @config ||= PDK::Config.new
+    return @config unless @config.nil?
+    options = {}
+    options['user.module_defaults.path'] = PDK::Util::Env['PDK_ANSWER_FILE'] unless PDK::Util::Env['PDK_ANSWER_FILE'].nil?
+    @config = PDK::Config.new(options)
   end
 
   def self.analytics
     @analytics ||= PDK::Analytics.build_client(
       logger:        PDK.logger,
-      disabled:      PDK::Util::Env['PDK_DISABLE_ANALYTICS'] || PDK.config.user['analytics']['disabled'],
-      user_id:       PDK.config.user['analytics']['user-id'],
+      disabled:      PDK::Util::Env['PDK_DISABLE_ANALYTICS'] || PDK.config.pdk_setting('analytics', 'disabled'),
+      user_id:       PDK.config.pdk_setting('analytics', 'user-id'),
       app_id:        "UA-139917834-#{PDK::Util.development_mode? ? '2' : '1'}",
       client:        :google_analytics,
       app_name:      'pdk',

lib/pdk/answer_file.rb

@@ -1,103 +1,12 @@
 require 'pdk'
-autoload :JSON, 'json'
 
 module PDK
   class AnswerFile
-    attr_reader :answers
-    attr_reader :answer_file_path
-
-    # Initialises the AnswerFile object, which stores the responses to certain
-    # interactive questions.
-    #
-    # @param answer_file_path [String, nil] The path on disk to the file where
-    #   the answers will be stored and read from. If not specified (or `nil`),
-    #   the default path will be used (see #default_answer_file_path).
-    #
-    # @raise (see #read_from_disk)
-    def initialize(answer_file_path = nil)
-      @answer_file_path = answer_file_path || default_answer_file_path
-      @answers = read_from_disk
-    end
-
-    # Retrieve the stored answer to a question.
-    #
-    # @param question [String] The question name/identifying string.
-    #
-    # @return [Object] The answer to the question, or `nil` if no answer found.
-    def [](question)
-      answers[question]
-    end
-
-    # Update the stored answers in memory and then save them to disk.
-    #
-    # @param new_answers [Hash{String => Object}] The new questions and answers
-    #   to be merged into the existing answers.
-    #
-    # @raise [PDK::CLI::FatalError] if the new answers are not provided as
-    #   a Hash.
-    # @raise (see #save_to_disk)
-    def update!(new_answers = {})
-      unless new_answers.is_a?(Hash)
-        raise PDK::CLI::FatalError, _('Answer file can be updated only with a Hash')
-      end
-
-      answers.merge!(new_answers)
-
-      save_to_disk
-    end
-
-    private
-
     # Determine the default path to the answer file.
     #
     # @return [String] The path on disk to the default answer file.
-    def default_answer_file_path
-      File.join(PDK::Util.cachedir, 'answers.json')
-    end
-
-    # Read existing answers into memory from the answer file on disk.
-    #
-    # @raise [PDK::CLI::FatalError] If the answer file exists but can not be
-    #   read.
-    #
-    # @return [Hash{String => Object}] The existing questions and answers.
-    def read_from_disk
-      return {} if !PDK::Util::Filesystem.file?(answer_file_path) || PDK::Util::Filesystem.zero?(answer_file_path)
-
-      unless PDK::Util::Filesystem.readable?(answer_file_path)
-        raise PDK::CLI::FatalError, _("Unable to open '%{file}' for reading") % {
-          file: answer_file_path,
-        }
-      end
-
-      answers = JSON.parse(PDK::Util::Filesystem.read_file(answer_file_path))
-      if answers.is_a?(Hash)
-        answers
-      else
-        PDK.logger.warn _("Answer file '%{path}' did not contain a valid set of answers, recreating it") % {
-          path: answer_file_path,
-        }
-        {}
-      end
-    rescue JSON::JSONError
-      PDK.logger.warn _("Answer file '%{path}' did not contain valid JSON, recreating it") % {
-        path: answer_file_path,
-      }
-      {}
-    end
-
-    # Save the in memory answer set to the answer file on disk.
-    #
-    # @raise [PDK::CLI::FatalError] if the answer file can not be written to.
-    def save_to_disk
-      PDK::Util::Filesystem.mkdir_p(File.dirname(answer_file_path))
-
-      PDK::Util::Filesystem.write_file(answer_file_path, JSON.pretty_generate(answers))
-    rescue SystemCallError, IOError => e
-      raise PDK::CLI::FatalError, _("Unable to write '%{file}': %{msg}") % {
-        file: answer_file_path,
-        msg:  e.message,
-      }
+    def self.default_answer_file_path
+      PDK::Util::Filesystem.expand_path(File.join(PDK::Util.cachedir, 'answers.json'))
     end
   end
 end

lib/pdk/cli.rb

@@ -150,11 +150,6 @@ def self.puppet_dev_option(dsl)
     flag :d, :debug, _('Enable debug output.') do |_, _|
       PDK.logger.enable_debug_output
     end
-
-    option nil, 'answer-file', _('Path to an answer file.'), argument: :required, hidden: true do |value|
-      require 'pdk/answer_file'
-      PDK.answer_file = value
-    end
   end
 
   require 'pdk/cli/bundle'

lib/pdk/cli/convert.rb

@@ -31,7 +31,7 @@ module PDK::CLI
         raise PDK::CLI::ExitWithError, _('You can not specify --template-url and --default-template.') if opts[:'template-url']
 
         opts[:'template-url'] = PDK::Util::TemplateURI.default_template_addressable_uri.to_s
-        PDK.answers.update!('template-url' => nil)
+        PDK.config.user['module_defaults']['template-url'] = nil
       end
 
       PDK::CLI::Util.validate_template_opts(opts)

lib/pdk/config.rb

@@ -11,6 +11,24 @@ class Config
     autoload :Validator, 'pdk/config/validator'
     autoload :YAML, 'pdk/config/yaml'
 
+    # Create a new instance of the PDK Configuration
+    # @param options [Hash[String => String]] Optional hash to override configuration options
+    # @option options [String] 'system.path'                 Path to the system PDK configuration file
+    # @option options [String] 'system.module_defaults.path' Path to the system module answers PDK configuration file
+    # @option options [String] 'user.path'                   Path to the user PDK configuration file
+    # @option options [String] 'user.module_defaults.path'   Path to the user module answers PDK configuration file
+    # @option options [String] 'user.analytics.path'         Path to the user analytics PDK configuration file
+    def initialize(options = nil)
+      options = {} if options.nil?
+      @config_options = {
+        'system.path'                 => PDK::Config.system_config_path,
+        'system.module_defaults.path' => PDK::Config.system_answers_path,
+        'user.path'                   => PDK::Config.user_config_path,
+        'user.module_defaults.path'   => PDK::AnswerFile.default_answer_file_path,
+        'user.analytics.path'         => PDK::Config.analytics_config_path,
+      }.merge(options)
+    end
+
     # The user configuration settings.
     # @deprecated This method is only provided as a courtesy until the `pdk set config` CLI and associated changes in this class, are completed.
     #             Any read-only operations should be using `.get` or `.get_within_scopes`
@@ -23,24 +41,26 @@ def user
     # @return [PDK::Config::Namespace]
     # @api private
     def system_config
-      @system ||= PDK::Config::JSON.new('system', file: PDK::Config.system_config_path) do
-        mount :module_defaults, PDK::Config::JSON.new(file: PDK::Config.system_answers_path)
+      local_options = @config_options
+      @system ||= PDK::Config::JSON.new('system', file: local_options['system.path']) do
+        mount :module_defaults, PDK::Config::JSON.new(file: local_options['system.module_defaults.path'])
       end
     end
 
     # The user level configuration settings.
     # @return [PDK::Config::Namespace]
     # @api private
     def user_config
-      @user ||= PDK::Config::JSON.new('user', file: PDK::Config.user_config_path) do
-        mount :module_defaults, PDK::Config::JSON.new(file: PDK.answers.answer_file_path)
+      local_options = @config_options
+      @user ||= PDK::Config::JSON.new('user', file: local_options['user.path']) do
+        mount :module_defaults, PDK::Config::JSON.new(file: local_options['user.module_defaults.path'])
 
         # Due to the json-schema gem having issues with Windows based paths, and only supporting Draft 05 (or less) do
         # not use JSON validation yet.  Once PDK drops support for EOL rubies, we will be able to use the json_schemer gem
         # Which has much more modern support
         # Reference - https://github.com/puppetlabs/pdk/pull/777
         # Reference - https://tickets.puppetlabs.com/browse/PDK-1526
-        mount :analytics, PDK::Config::YAML.new(file: PDK::Config.analytics_config_path, persistent_defaults: true) do
+        mount :analytics, PDK::Config::YAML.new(file: local_options['user.analytics.path'], persistent_defaults: true) do
           setting :disabled do
             validate PDK::Config::Validator.boolean
             default_to { PDK::Config.bolt_analytics_config.fetch('disabled', true) }

lib/pdk/config/namespace.rb

@@ -34,6 +34,7 @@ def initialize(name = nil, file: nil, parent: nil, persistent_defaults: false, &
         @persistent_defaults = persistent_defaults
         @mounts = {}
         @loaded_from_file = false
+        @read_only = false
 
         instance_eval(&block) if block_given?
       end
@@ -206,6 +207,14 @@ def include_in_parent?
         child_namespace? && file.nil?
       end
 
+      # Disables the namespace, and child namespaces, from writing changes to disk.
+      # Typically this is only needed for unit testing.
+      # @api private
+      def read_only!
+        @read_only = true
+        @mounts.each { |_, child| child.read_only! }
+      end
+
       private
 
       # Determines whether a setting name should be resolved using the filter
@@ -303,7 +312,7 @@ def load_data(filename)
       #
       # @return [nil]
       def save_data
-        return if file.nil?
+        return if file.nil? || @read_only
 
         PDK::Util::Filesystem.mkdir_p(File.dirname(file))
 

lib/pdk/generate/module.rb

@@ -82,11 +82,11 @@ def self.invoke(opts = {})
           # If the user specifies our default template url via the command
           # line, remove the saved template-url answer so that the template_uri
           # resolution can find new default URLs in the future.
-          PDK.answers.update!('template-url' => nil) if opts.key?(:'template-url')
+          PDK.config.user['module_defaults']['template-url'] = nil if opts.key?(:'template-url')
         else
           # Save the template-url answers if the module was generated using a
           # template/reference other than ours.
-          PDK.answers.update!('template-url' => template_uri.metadata_format)
+          PDK.config.user['module_defaults']['template-url'] = template_uri.metadata_format
         end
 
         begin
@@ -135,13 +135,13 @@ def self.prepare_metadata(opts = {})
         require 'pdk/answer_file'
         require 'pdk/module/metadata'
 
-        opts[:username] = (opts[:username] || PDK.answers['forge_username'] || username_from_login).downcase
+        opts[:username] = (opts[:username] || PDK.config.pdk_setting('module_defaults', 'forge_username') || username_from_login).downcase
 
         defaults = PDK::Module::Metadata::DEFAULTS.dup
 
         defaults['name'] = "#{opts[:username]}-#{opts[:module_name]}" unless opts[:module_name].nil?
-        defaults['author'] = PDK.answers['author'] unless PDK.answers['author'].nil?
-        defaults['license'] = PDK.answers['license'] unless PDK.answers['license'].nil?
+        defaults['author'] = PDK.config.pdk_setting('module_defaults', 'author') unless PDK.config.pdk_setting('module_defaults', 'author').nil?
+        defaults['license'] = PDK.config.pdk_setting('module_defaults', 'license') unless PDK.config.pdk_setting('module_defaults', 'license').nil?
         defaults['license'] = opts[:license] if opts.key?(:license)
 
         metadata = PDK::Module::Metadata.new(defaults)
@@ -342,13 +342,9 @@ def self.module_interview(metadata, opts = {})
         end
 
         require 'pdk/answer_file'
-        PDK.answers.update!(
-          {
-            'forge_username' => opts[:username],
-            'author'         => answers['author'],
-            'license'        => answers['license'],
-          }.delete_if { |_key, value| value.nil? },
-        )
+        PDK.config.user['module_defaults']['forge_username'] = opts[:username] unless opts[:username].nil?
+        PDK.config.user['module_defaults']['author'] = answers['author'] unless answers['author'].nil?
+        PDK.config.user['module_defaults']['license'] = answers['license'] unless answers['license'].nil?
       end
     end
   end

lib/pdk/util/template_uri.rb

@@ -218,10 +218,11 @@ def self.templates(opts)
                        else
                          nil
                        end
-        answers_uri = if [PACKAGED_TEMPLATE_KEYWORD, DEPRECATED_TEMPLATE_URL].include?(PDK.answers['template-url'])
+        default_template_url = PDK.config.pdk_setting('module_defaults', 'template-url')
+        answers_uri = if [PACKAGED_TEMPLATE_KEYWORD, DEPRECATED_TEMPLATE_URL].include?(default_template_url)
                         Addressable::URI.parse(default_template_uri)
-                      elsif PDK.answers['template-url']
-                        new(uri_safe(PDK.answers['template-url'])).uri
+                      elsif default_template_url
+                        new(uri_safe(default_template_url)).uri
                       else
                         nil
                       end

spec/acceptance/convert_spec.rb

@@ -156,17 +156,28 @@
   context 'when converting to the default template' do
     include_context 'in a new module', 'non_default_template'
 
-    answer_file = File.join('..', 'non_default_template_answers.json')
+    before(:each) do
+      @old_answer_file = ENV['PDK_ANSWER_FILE']
+      ENV['PDK_ANSWER_FILE'] = File.expand_path(File.join('..', 'non_default_template_answers.json'))
+    end
+
+    after(:each) do
+      ENV['PDK_ANSWER_FILE'] = @old_answer_file # rubocop:disable RSpec/InstanceVariable This is fine.
+    end
 
-    describe command("pdk convert --default-template --force --answer-file #{answer_file}") do
+    describe command('pdk convert --default-template --force') do
       its(:exit_status) { is_expected.to eq(0) }
 
+      # Note - The following file(...) tests can not be run in isolation and require the above
+      # `its(:exit_status)` to have run first.
       describe file('metadata.json') do
         its(:content_as_json) { is_expected.to include('template-url' => "#{template_repo}#master") }
       end
 
-      describe file(answer_file) do
-        its(:content_as_json) { is_expected.to include('template-url' => nil) }
+      describe file(File.join('..', 'non_default_template_answers.json')) do
+        it 'has a nil template-url' do
+          expect(subject.content_as_json['template-url']).to be_nil # rubocop:disable RSpec/NamedSubject We can't use named subjects here
+        end
       end
     end
   end

spec/acceptance/support/in_a_new_module.rb

@@ -8,10 +8,10 @@
     argv = [
       'pdk', 'new', 'module', name,
       '--skip-interview',
-      '--template-url', template,
-      '--answer-file', File.join(Dir.pwd, "#{name}_answers.json")
+      '--template-url', template
     ]
-    output, status = Open3.capture2e(*argv)
+    env = { 'PDK_ANSWER_FILE' => File.join(Dir.pwd, "#{name}_answers.json") }
+    output, status = Open3.capture2e(env, *argv)
 
     raise "Failed to create test module:\n#{output}" unless status.success?
 

spec/acceptance/template_ref_spec.rb

@@ -2,7 +2,9 @@
 
 describe 'Specifying a template-ref' do
   after(:all) do
-    Dir.chdir('..')
+    # We may or may not be in the foo module directory.  If we are, go back one directory.
+    # This can happen if you only run a subset of tests in this file.
+    Dir.chdir('..') if Dir.pwd.end_with?('foo')
     FileUtils.rm_rf('foo')
     FileUtils.rm('foo_answers.json')
   end
@@ -12,10 +14,18 @@
       'pdk', 'new', 'module', 'foo',
       '--skip-interview',
       '--template-url', 'https://github.com/puppetlabs/pdk-templates',
-      '--template-ref', '1.7.0',
-      '--answer-file', 'foo_answers.json'
+      '--template-ref', '1.7.0'
     ]
 
+    before(:each) do
+      @old_answer_file = ENV['PDK_ANSWER_FILE']
+      ENV['PDK_ANSWER_FILE'] = 'foo_answers.json'
+    end
+
+    after(:each) do
+      ENV['PDK_ANSWER_FILE'] = @old_answer_file # rubocop:disable RSpec/InstanceVariable This is fine.
+    end
+
     describe command(create_cmd.join(' ')) do
       its(:exit_status) { is_expected.to eq(0) }
       its(:stderr) { is_expected.to match(%r{creating new module: foo}i) }