(puppetlabsGH-2549) Use new default modulepath

This updates Bolt to use the new default modulepath `['modules']` and
append the `.modules/` directory to the end of both the default
modulepath and configured modulepath.

!feature

* **Update default modulepath**
  ([puppetlabs#2549](puppetlabs#2549))

  Bolt's default modulepath is now `['modules']` instead of
  `['modules', 'site', 'site-modules']`. Bolt will also automatically
  append the project's `.modules/` directory to all modulepaths,
  whether a project uses the default modulepath or a configured
  modulepath.

lib/bolt/config.rb

@@ -335,7 +335,7 @@ def validate
         )
       end
 
-      if @project.modules && @data['modulepath']&.include?(@project.managed_moduledir.to_s)
+      if @data['modulepath']&.include?(@project.managed_moduledir.to_s)
         raise Bolt::ValidationError,
               "Found invalid path in modulepath: #{@project.managed_moduledir}. This path "\
               "is automatically appended to the modulepath and cannot be configured."
@@ -372,17 +372,11 @@ def puppetfile
     end
 
     def modulepath
-      path = @data['modulepath'] || @project.modulepath
-
-      if @project.modules.any?
-        path + [@project.managed_moduledir.to_s]
-      else
-        path
-      end
+      (@data['modulepath'] || @project.modulepath) + [@project.managed_moduledir.to_s]
     end
 
     def modulepath=(value)
-      @data['modulepath'] = value
+      @data['modulepath'] = Array(value)
     end
 
     def plugin_cache

lib/bolt/config/options.rb

@@ -214,7 +214,7 @@ module Options
           },
           _plugin: false,
           _example: ["~/.puppetlabs/bolt/modules", "~/.puppetlabs/bolt/site-modules"],
-          _default: ["project/modules", "project/site-modules", "project/site"]
+          _default: ["project/modules"]
         },
         "module-install" => {
           description: "Options that configure where Bolt downloads modules from. This option is only used when "\

lib/bolt/project.rb

@@ -114,6 +114,7 @@ def initialize(data, path, type = 'option')
       @backup_dir        = @path + '.bolt-bak'
       @plugin_cache_file = @path + '.plugin_cache.json'
       @plan_cache_file   = @path + '.plan_cache.json'
+      @modulepath        = [(@path + 'modules').to_s]
 
       if (tc = Bolt::Config::INVENTORY_OPTIONS.keys & data.keys).any?
         Bolt::Logger.warn(
@@ -124,14 +125,6 @@ def initialize(data, path, type = 'option')
 
       @data = data.slice(*Bolt::Config::PROJECT_OPTIONS)
 
-      # If the 'modules' key is present in the project configuration file,
-      # use the new, shorter modulepath.
-      @modulepath = if @data.key?('modules')
-                      [(@path + 'modules').to_s]
-                    else
-                      [(@path + 'modules').to_s, (@path + 'site-modules').to_s, (@path + 'site').to_s]
-                    end
-
       validate if project_file?
     end
 

lib/bolt/project_manager.rb

@@ -195,7 +195,7 @@ def migrate
 
       migrator.migrate(
         @config.project,
-        @config.modulepath
+        @config.modulepath[0...-1]
       )
     end
   end

lib/bolt/project_manager/module_migrator.rb

@@ -6,19 +6,20 @@ module Bolt
   class ProjectManager
     class ModuleMigrator < Migrator
       def migrate(project, configured_modulepath)
-        return true unless project.modules.empty?
+        return true if project.managed_moduledir.exist?
 
         @outputter.print_message "Migrating project modules\n\n"
 
         config            = project.project_file
         puppetfile        = project.puppetfile
         managed_moduledir = project.managed_moduledir
-        modulepath        = [(project.path + 'modules').to_s,
+        new_modulepath    = [(project.path + 'modules').to_s]
+        old_modulepath    = [(project.path + 'modules').to_s,
                              (project.path + 'site-modules').to_s,
                              (project.path + 'site').to_s]
 
         # Notify user to manually migrate modules if using non-default modulepath
-        if configured_modulepath != modulepath
+        if configured_modulepath != new_modulepath && configured_modulepath != old_modulepath
           @outputter.print_action_step(
             "Project has a non-default configured modulepath, unable to automatically "\
             "migrate project modules. To migrate project modules manually, see "\
@@ -27,10 +28,10 @@ def migrate(project, configured_modulepath)
           true
         # Migrate modules from Puppetfile
         elsif File.exist?(puppetfile)
-          migrate_modules_from_puppetfile(config, puppetfile, managed_moduledir, modulepath)
+          migrate_modules_from_puppetfile(config, puppetfile, managed_moduledir, old_modulepath)
         # Migrate modules to updated modulepath
         else
-          consolidate_modules(modulepath)
+          consolidate_modules(old_modulepath)
           update_project_config([], config)
         end
       end

spec/bolt/cli_spec.rb

@@ -2435,7 +2435,7 @@ def stub_config(file_content = {})
     it 'reads modulepath' do
       cli = Bolt::CLI.new(%w[command run uptime] + config_flags)
       cli.parse
-      expect(cli.config.modulepath).to eq(modulepath)
+      expect(cli.config.modulepath).to include(*modulepath)
     end
 
     it 'reads concurrency' do

spec/bolt/config_spec.rb

@@ -41,13 +41,13 @@
     it "treats relative modulepath as relative to project" do
       module_dirs = %w[site modules]
       config = Bolt::Config.new(project, 'modulepath' => module_dirs.join(File::PATH_SEPARATOR))
-      expect(config.modulepath).to eq(module_dirs.map { |dir| (project.path + dir).to_s })
+      expect(config.modulepath).to include(*module_dirs.map { |dir| (project.path + dir).to_s })
     end
 
     it "accepts an array for modulepath" do
       module_dirs = %w[site modules]
       config = Bolt::Config.new(project, 'modulepath' => module_dirs)
-      expect(config.modulepath).to eq(module_dirs.map { |dir| (project.path + dir).to_s })
+      expect(config.modulepath).to include(*module_dirs.map { |dir| (project.path + dir).to_s })
     end
 
     it 'modifies concurrency if ulimit is low', :ssh do

spec/bolt/project_manager/module_migrator_spec.rb

@@ -45,17 +45,15 @@ def make_moduledirs
     allow(outputter).to receive(:stop_spin)
   end
 
-  context 'with modules configured' do
-    let(:project_config) { { 'modules' => [] } }
-
+  context 'with a managed moduledir' do
     it 'does not migrate' do
+      FileUtils.mkdir(@tmpdir + '.modules')
       expect(migrate).to be(true)
-      expect(File.exist?(project.managed_moduledir)).to be(false)
     end
   end
 
   context 'with a non-default modulepath' do
-    let(:modulepath) { [(@tmpdir + 'modules').to_s] }
+    let(:modulepath) { [(@tmpdir + 'mymodules').to_s] }
 
     it 'does not migrate' do
       migrate

spec/bolt/project_spec.rb

@@ -232,24 +232,4 @@
       Bolt::Project.create_project('myproject', 'user')
     end
   end
-
-  describe '#modulepath' do
-    context 'with modules set' do
-      let(:project_config) { { 'modules' => [] } }
-
-      it 'returns the new default modulepath' do
-        expect(project.modulepath).to match_array([(project.path + 'modules').to_s])
-      end
-    end
-
-    context 'without modules set' do
-      it 'returns the old default modulepath if modules is not set' do
-        expect(project.modulepath).to match_array([
-                                                    (project.path + 'modules').to_s,
-                                                    (project.path + 'site-modules').to_s,
-                                                    (project.path + 'site').to_s
-                                                  ])
-      end
-    end
-  end
 end