Merge pull request #480 from scotje/842_its_aliveeeee

(PDK-842) Wire puppet-version and pe-version options into subcommands

lib/pdk/cli.rb

@@ -51,13 +51,8 @@ def self.full_interview_option(dsl)
   end
 
   def self.puppet_version_options(dsl)
-    dsl.option nil, 'puppet-version', _('Puppet version to run tests or validations against.'), argument: :required do |value|
-      PDK::Util::PuppetVersion.find_gem_for(value)
-    end
-
-    dsl.option nil, 'pe-version', _('Puppet Enterprise version to run tests or validations against.'), argument: :required do |value|
-      PDK::Util::PuppetVersion.from_pe_version(value)
-    end
+    dsl.option nil, 'puppet-version', _('Puppet version to run tests or validations against.'), argument: :required
+    dsl.option nil, 'pe-version', _('Puppet Enterprise version to run tests or validations against.'), argument: :required
   end
 
   @base_cmd = Cri::Command.define do

lib/pdk/cli/bundle.rb

@@ -18,6 +18,13 @@ module PDK::CLI
         message: _('`pdk bundle` can only be run from inside a valid module directory.'),
       )
 
+      PDK::CLI::Util.validate_puppet_version_opts({})
+
+      # Ensure that the bundled gems are up to date and correct Ruby is activated before running commend.
+      puppet_env = PDK::CLI::Util.puppet_from_opts_or_env({})
+      PDK::Util::RubyVersion.use(puppet_env[:ruby_version])
+      PDK::Util::Bundler.ensure_bundle!(puppet_env[:gemset])
+
       command = PDK::CLI::Exec::Command.new(PDK::CLI::Exec.bundle_bin, *args).tap do |c|
         c.context = :module
       end

lib/pdk/cli/exec.rb

@@ -42,17 +42,24 @@ def self.bundle_bin
 
       def self.try_vendored_bin(vendored_bin_path, fallback)
         unless PDK::Util.package_install?
-          PDK.logger.debug(_("PDK package installation not found. Trying '%{fallback}' from the system PATH instead.") % { fallback: fallback })
+          PDK.logger.debug(_("PDK package installation not found. Trying '%{fallback}' from the system PATH instead.") % {
+            fallback: fallback,
+          })
           return fallback
         end
 
-        if File.exist?(File.join(PDK::Util.pdk_package_basedir, vendored_bin_path))
-          PDK.logger.debug(_("Using '%{vendored_bin_path}' from PDK package.") % { vendored_bin_path: vendored_bin_path })
-          File.join(PDK::Util.pdk_package_basedir, vendored_bin_path)
-        else
-          PDK.logger.debug(_("Could not find '%{vendored_bin_path}' in PDK package. Trying '%{fallback}' from the system PATH instead.") % { fallback: fallback, vendored_bin_path: vendored_bin_path })
-          fallback
+        vendored_bin_full_path = File.join(PDK::Util.pdk_package_basedir, vendored_bin_path)
+
+        unless File.exist?(vendored_bin_full_path)
+          PDK.logger.debug(_("Could not find '%{vendored_bin}' in PDK package. Trying '%{fallback}' from the system PATH instead.") % {
+            fallback: fallback,
+            vendored_bin: vendored_bin_full_path,
+          })
+          return fallback
         end
+
+        PDK.logger.debug(_("Using '%{vendored_bin}' from PDK package.") % { vendored_bin: vendored_bin_full_path })
+        vendored_bin_full_path
       end
 
       # TODO: decide how/when to connect stdin to child process for things like pry

lib/pdk/cli/test/unit.rb

@@ -22,9 +22,7 @@ module PDK::CLI
     run do |opts, _args, _cmd|
       require 'pdk/tests/unit'
 
-      if opts[:'puppet-version'] && opts[:'pe-version']
-        raise PDK::CLI::ExitWithError, _('You can not specify both --puppet-version and --pe-version at the same time.')
-      end
+      PDK::CLI::Util.validate_puppet_version_opts(opts)
 
       PDK::CLI::Util.ensure_in_module!(
         message:   _('Unit tests can only be run from inside a valid module directory.'),
@@ -64,6 +62,11 @@ module PDK::CLI
                            }]
                          end
 
+        # Ensure that the bundled gems are up to date and correct Ruby is activated before running tests.
+        puppet_env = PDK::CLI::Util.puppet_from_opts_or_env(opts)
+        PDK::Util::RubyVersion.use(puppet_env[:ruby_version])
+        PDK::Util::Bundler.ensure_bundle!(puppet_env[:gemset])
+
         exit_code = PDK::Test::Unit.invoke(report, opts)
 
         report_formats.each do |format|

lib/pdk/cli/util.rb

@@ -81,6 +81,87 @@ def module_version_check
         end
       end
       module_function :module_version_check
+
+      def puppet_from_opts_or_env(opts)
+        desired_puppet_version = (opts || {})[:'puppet-version'] || ENV['PDK_PUPPET_VERSION']
+        desired_pe_version = (opts || {})[:'pe-version'] || ENV['PDK_PE_VERSION']
+
+        begin
+          puppet_env =
+            if desired_puppet_version
+              PDK::Util::PuppetVersion.find_gem_for(desired_puppet_version)
+            elsif desired_pe_version
+              PDK::Util::PuppetVersion.from_pe_version(desired_pe_version)
+            else
+              PDK::Util::PuppetVersion.from_module_metadata || PDK::Util::PuppetVersion.latest_available
+            end
+        rescue ArgumentError => e
+          raise PDK::CLI::ExitWithError, e.message
+        end
+
+        # Notify user of what Ruby version will be used.
+        PDK.logger.info(_('Using Ruby %{version}') % {
+          version: puppet_env[:ruby_version],
+        })
+
+        gemset = { puppet: puppet_env[:gem_version].to_s }
+
+        # Notify user of what gems are being activated.
+        gemset.each do |gem, version|
+          next if version.nil?
+
+          PDK.logger.info(_('Using %{gem} %{version}') % {
+            gem: gem.to_s.capitalize,
+            version: version,
+          })
+        end
+
+        {
+          gemset: gemset,
+          ruby_version: puppet_env[:ruby_version],
+        }
+      end
+      module_function :puppet_from_opts_or_env
+
+      def validate_puppet_version_opts(opts)
+        puppet_ver_specs = []
+        puppet_ver_specs << '--puppet-version option' if opts[:'puppet-version']
+        puppet_ver_specs << 'PDK_PUPPET_VERSION environment variable' if ENV['PDK_PUPPET_VERSION'] && !ENV['PDK_PUPPET_VERSION'].empty?
+
+        pe_ver_specs = []
+        pe_ver_specs << '--pe-version option' if opts[:'pe-version']
+        pe_ver_specs << 'PDK_PE_VERSION environment variable' if ENV['PDK_PE_VERSION'] && !ENV['PDK_PE_VERSION'].empty?
+
+        puppet_ver_specs.each do |pup_ver_spec|
+          next if pe_ver_specs.empty?
+
+          offending = [pup_ver_spec, pe_ver_specs[0]].sort
+
+          raise PDK::CLI::ExitWithError, _('You cannot specify a %{first} and %{second} at the same time.') % {
+            first: offending[0],
+            second: offending[1],
+          }
+        end
+
+        if puppet_ver_specs.size == 2
+          warning_str = 'Puppet version option from command line: "--puppet-version=%{pup_ver_opt}" '
+          warning_str += 'overrides value from environment: "PDK_PUPPET_VERSION=%{pup_ver_env}". You should not specify both.'
+
+          PDK.logger.warn(_(warning_str) % {
+            pup_ver_opt: opts[:'puppet-version'],
+            pup_ver_env: ENV['PDK_PUPPET_VERSION'],
+          })
+        elsif pe_ver_specs.size == 2
+          warning_str = 'Puppet Enterprise version option from command line: "--pe-version=%{pe_ver_opt}" '
+          warning_str += 'overrides value from environment: "PDK_PE_VERSION=%{pe_ver_env}". You should not specify both.'
+
+          PDK.logger.warn(_(warning_str) % {
+            pup_ver_opt: opts[:'pe-version'],
+            pup_ver_env: ENV['PDK_PE_VERSION'],
+          })
+        end
+      end
+      module_function :validate_puppet_version_opts
     end
   end
 end

lib/pdk/cli/validate.rb

@@ -33,9 +33,7 @@ module PDK::CLI
         exit 0
       end
 
-      if opts[:'puppet-version'] && opts[:'pe-version']
-        raise PDK::CLI::ExitWithError, _('You can not specify both --puppet-version and --pe-version at the same time.')
-      end
+      PDK::CLI::Util.validate_puppet_version_opts(opts)
 
       PDK::CLI::Util.ensure_in_module!(
         message:   _('Code validation can only be run from inside a valid module directory'),
@@ -87,8 +85,10 @@ module PDK::CLI
       options = targets.empty? ? {} : { targets: targets }
       options[:auto_correct] = true if opts.key?(:'auto-correct')
 
-      # Ensure that the bundle is installed and tools are available before running any validations.
-      PDK::Util::Bundler.ensure_bundle!
+      # Ensure that the bundled gems are up to date and correct Ruby is activated before running any validations.
+      puppet_env = PDK::CLI::Util.puppet_from_opts_or_env(opts)
+      PDK::Util::RubyVersion.use(puppet_env[:ruby_version])
+      PDK::Util::Bundler.ensure_bundle!(puppet_env[:gemset])
 
       exit_code = 0
       if opts[:parallel]

lib/pdk/tests/unit.rb

@@ -62,7 +62,6 @@ def self.setup
       end
 
       def self.invoke(report, options = {})
-        PDK::Util::Bundler.ensure_bundle!
         PDK::Util::Bundler.ensure_binstubs!('rake')
 
         setup
@@ -181,7 +180,6 @@ def self.merge_json_results(json_data, duration)
 
       # @return array of { :id, :full_description }
       def self.list
-        PDK::Util::Bundler.ensure_bundle!
         PDK::Util::Bundler.ensure_binstubs!('rake')
 
         command_argv = [File.join(PDK::Util.module_root, 'bin', 'rake'), 'spec_list_json']

lib/pdk/util/bundler.rb

@@ -9,9 +9,12 @@ module Util
     module Bundler
       class BundleHelper; end
 
-      def self.ensure_bundle!(gem_overrides = {})
+      def self.ensure_bundle!(gem_overrides = nil)
         bundle = BundleHelper.new
 
+        # This will default ensure_bundle! to re-resolving everything to latest
+        gem_overrides ||= { puppet: nil, hiera: nil, facter: nil }
+
         if already_bundled?(bundle.gemfile, gem_overrides)
           PDK.logger.debug(_('Bundler managed gems already up to date.'))
           return
@@ -22,19 +25,33 @@ def self.ensure_bundle!(gem_overrides = {})
           return
         end
 
-        # Generate initial Gemfile.lock
-        if bundle.locked?
-          # Update puppet-related gem dependencies by re-resolving them specifically.
-          # If this is a packaged install, only consider already available gems at this point.
-          bundle.update_lock!(gem_overrides, local: PDK::Util.package_install?)
-        else
-          bundle.lock!(gem_overrides)
+        unless bundle.locked?
+          # Generate initial default Gemfile.lock, either from package cache or
+          # by invoking `bundle lock`
+          bundle.lock!
+        end
+
+        # Check if all dependencies will be available once we update the lockfile.
+        begin
+          original_lockfile = bundle.gemfile_lock
+          temp_lockfile = "#{original_lockfile}.tmp"
+
+          FileUtils.mv(original_lockfile, temp_lockfile)
+
+          all_deps_available = bundle.installed?(gem_overrides)
+        ensure
+          FileUtils.mv(temp_lockfile, original_lockfile, force: true)
         end
 
-        # Check for any still-unresolved dependencies. For packaged installs, this should
-        # only evaluate to false if the user has added custom gems that we don't vendor, in
-        # which case `bundle install` will resolve new dependencies as needed.
-        unless bundle.installed?(gem_overrides)
+        # Update puppet-related gem dependencies by re-resolving them specifically.
+        # If there are additional dependencies that aren't available locally, allow
+        # `bundle lock` to reach out to rubygems.org
+        bundle.update_lock!(gem_overrides, local: all_deps_available)
+
+        # If there were missing dependencies when we checked above, let `bundle install`
+        # go out and get them. For packaged installs, this should only be true if the user
+        # has added custom gems that we don't vendor.
+        unless bundle.installed?
           bundle.install!(gem_overrides)
         end
 
@@ -66,18 +83,23 @@ def gemfile
           @gemfile ||= PDK::Util.find_upwards('Gemfile')
         end
 
+        def gemfile_lock
+          return nil if gemfile.nil?
+          @gemfile_lock ||= File.join(File.dirname(gemfile), 'Gemfile.lock')
+        end
+
         def gemfile?
           !gemfile.nil?
         end
 
         def locked?
-          !gemfile_lock.nil?
+          !gemfile_lock.nil? && File.file?(gemfile_lock)
         end
 
         def installed?(gem_overrides = {})
           PDK.logger.debug(_('Checking for missing Gemfile dependencies.'))
 
-          argv = ['check', "--gemfile=#{gemfile}"]
+          argv = ['check', "--gemfile=#{gemfile}", '--dry-run']
           argv << "--path=#{bundle_cachedir}" unless PDK::Util.package_install?
 
           cmd = bundle_command(*argv).tap do |c|
@@ -93,42 +115,44 @@ def installed?(gem_overrides = {})
           result[:exit_code].zero?
         end
 
-        def lock!(gem_overrides = {})
+        def lock!
           if PDK::Util.package_install?
             # In packaged installs, use vendored Gemfile.lock as a starting point.
             # Subsequent 'bundle install' will still pick up any new dependencies.
-            vendored_gemfile_lock = File.join(PDK::Util.package_cachedir, 'Gemfile.lock')
+            vendored_lockfiles = [
+              File.join(PDK::Util.package_cachedir, "Gemfile-#{PDK::Util::RubyVersion.active_ruby_version}.lock"),
+              File.join(PDK::Util.package_cachedir, 'Gemfile.lock'),
+            ]
+
+            vendored_gemfile_lock = vendored_lockfiles.find { |lockfile| File.exist?(lockfile) }
 
-            unless File.exist?(vendored_gemfile_lock)
+            unless vendored_gemfile_lock
               raise PDK::CLI::FatalError, _('Vendored Gemfile.lock (%{source}) not found.') % {
                 source: vendored_gemfile_lock,
               }
             end
 
             PDK.logger.debug(_('Using vendored Gemfile.lock from %{source}.') % { source: vendored_gemfile_lock })
             FileUtils.cp(vendored_gemfile_lock, File.join(PDK::Util.module_root, 'Gemfile.lock'))
-
-            # Update the vendored lock with any overrides
-            update_lock!(gem_overrides, local: true) unless gem_overrides.empty?
           else
             argv = ['lock']
 
             cmd = bundle_command(*argv).tap do |c|
-              c.add_spinner(_('Resolving Gemfile dependencies.'))
-              c.update_environment(gemfile_env(gem_overrides)) unless gem_overrides.empty?
+              c.add_spinner(_('Resolving default Gemfile dependencies.'))
             end
 
             result = cmd.execute!
 
             unless result[:exit_code].zero?
               PDK.logger.fatal(result.values_at(:stdout, :stderr).join("\n"))
-              raise PDK::CLI::FatalError, _('Unable to resolve Gemfile dependencies.')
+              raise PDK::CLI::FatalError, _('Unable to resolve default Gemfile dependencies.')
             end
-          end
 
-          # After initial lockfile generation, re-resolve json gem to built-in
-          # version to avoid unncessary native compilation attempts.
-          update_lock!({ json: nil }, local: true)
+            # After initial lockfile generation, re-resolve json gem to built-in
+            # version to avoid unncessary native compilation attempts. For packaged
+            # installs this is done during the generation of the vendored Gemfile.lock
+            update_lock!({ json: nil }, local: true)
+          end
 
           true
         end
@@ -199,10 +223,6 @@ def bundle_command(*args)
           end
         end
 
-        def gemfile_lock
-          @gemfile_lock ||= PDK::Util.find_upwards('Gemfile.lock')
-        end
-
         def bundle_cachedir
           @bundle_cachedir ||= PDK::Util.package_install? ? PDK::Util.package_cachedir : File.join(PDK::Util.cachedir)
         end