(CODEMGMT-1422) Delete spec dirs on module sync

CHANGELOG.mkd

@@ -4,6 +4,7 @@ CHANGELOG
 Unreleased
 ----------
 
+- (CODEMGMT-1422) Delete spec dirs for module deploys [#1198](https://github.com/puppetlabs/r10k/pull/1198)
 - Always sync git cache on `ref: 'HEAD'` [#1182](https://github.com/puppetlabs/r10k/pull/1182)
 - (CODEMGMT-1421) Add skeleton for deploy_spec option [#1189](https://github.com/puppetlabs/r10k/pull/1189)
 - (RK-369) Make module deploys run the postrun command if any environments were updated. [#982](https://github.com/puppetlabs/r10k/issues/982)

lib/r10k/action/deploy/environment.rb

@@ -238,6 +238,7 @@ def allowed_initialize_opts
                       modules: :self,
                       cachedir: :self,
                       'no-force': :self,
+                      'deploy-spec': :self,
                       'generate-types': :self,
                       'puppet-path': :self,
                       'puppet-conf': :self,

lib/r10k/action/deploy/module.rb

@@ -38,6 +38,7 @@ def initialize(opts, argv, settings = {})
                 generate_types: @generate_types
               },
               modules: {
+                deploy_spec: settings.dig(:deploy, :deploy_spec),
                 requested_modules: @argv.map.to_a,
                 # force here is used to make it easier to reason about
                 force: !@no_force
@@ -117,6 +118,7 @@ def visit_environment(environment)
         def allowed_initialize_opts
           super.merge(environment: true,
                       cachedir: :self,
+                      'deploy-spec': :self,
                       'no-force': :self,
                       'generate-types': :self,
                       'puppet-path': :self,

lib/r10k/module/base.rb

@@ -35,6 +35,10 @@ class R10K::Module::Base
   #   @return [String] Where the module was sourced from. E.g., "Puppetfile"
   attr_accessor :origin
 
+  # @!attribute [rw] spec_deletable
+  #   @return [Boolean] set this to true if the spec dir can be safely removed, ie in the moduledir
+  attr_accessor :spec_deletable
+
   # There's been some churn over `author` vs `owner` and `full_name` over
   # `title`, so in the short run it's easier to support both and deprecate one
   # later.
@@ -52,6 +56,7 @@ def initialize(title, dirname, args, environment=nil)
     @path = Pathname.new(File.join(@dirname, @name))
     @environment = environment
     @overrides = args.delete(:overrides) || {}
+    @spec_deletable = true
     @deploy_spec = args.delete(:deploy_spec)
     @deploy_spec = @overrides[:modules].delete(:deploy_spec) if @overrides.dig(:modules, :deploy_spec)
     @origin = 'external' # Expect Puppetfile or R10k::Environment to set this to a specific value
@@ -66,6 +71,36 @@ def full_path
     path.to_s
   end
 
+  # Delete the spec dir unless @deploy_spec has been set to true or @spec_deletable is false
+  def maybe_delete_spec_dir
+    unless @deploy_spec
+      if @spec_deletable
+        delete_spec_dir
+      else
+        logger.info _("Spec dir for #{@title} will not be deleted because it is not in the moduledir")
+      end
+    end
+  end
+
+  # Actually remove the spec dir
+  def delete_spec_dir
+    spec_path = @path + 'spec'
+    if spec_path.symlink?
+      spec_path = spec_path.realpath
+    end
+    if spec_path.directory?
+      logger.debug2 _("Deleting spec data at #{spec_path}")
+      # Use the secure flag for the #rm_rf method to avoid security issues
+      # involving TOCTTOU(time of check to time of use); more details here:
+      # https://ruby-doc.org/stdlib-2.7.0/libdoc/fileutils/rdoc/FileUtils.html#method-c-rm_rf
+      # Additionally, #rm_rf also has problems in windows with with symlink targets
+      # also being deleted; this should be revisted if Windows becomes higher priority.
+      FileUtils.rm_rf(spec_path, secure: true)
+    else
+      logger.debug2 _("No spec dir detected at #{spec_path}, skipping deletion")
+    end
+  end
+
   # Synchronize this module with the indicated state.
   # @param [Hash] opts Deprecated
   def sync(opts={})

lib/r10k/module/forge.rb

@@ -68,6 +68,7 @@ def sync(opts={})
       when :mismatched
         reinstall
       end
+      maybe_delete_spec_dir
     end
   end
 

lib/r10k/module/git.rb

@@ -86,6 +86,7 @@ def properties
   def sync(opts={})
     force = opts[:force] || @force
     @repo.sync(version, force) if should_sync?
+    maybe_delete_spec_dir
   end
 
   def status

lib/r10k/module/svn.rb

@@ -80,6 +80,7 @@ def sync(opts={})
       when :outdated
         update
       end
+      maybe_delete_spec_dir
     end
   end
 

lib/r10k/module_loader/puppetfile.rb

@@ -103,11 +103,14 @@ def add_module(name, module_info)
 
         module_info[:overrides] = @overrides
 
+        spec_deletable = false
+
         if install_path = module_info.delete(:install_path)
           install_path = resolve_path(@basedir, install_path)
           validate_install_path(install_path, name)
         else
           install_path = @moduledir
+          spec_deletable = true
         end
 
         if @default_branch_override
@@ -116,6 +119,7 @@ def add_module(name, module_info)
 
         mod = R10K::Module.new(name, install_path, module_info, @environment)
         mod.origin = :puppetfile
+        mod.spec_deletable = spec_deletable
 
         # Do not save modules if they would conflict with the attached
         # environment

spec/unit/action/deploy/environment_spec.rb

@@ -59,6 +59,10 @@
       described_class.new({ 'github-app-key': '/nonexistent' }, [], {})
     end
 
+    it 'can accept a deploy-spec option' do
+      described_class.new({ :'deploy-spec' => true }, [], {})
+    end
+
     describe "initializing errors" do
       let (:settings) { { deploy: { purge_levels: [:environment],
                                     purge_whitelist: ['coolfile', 'coolfile2'],

spec/unit/action/deploy/module_spec.rb

@@ -53,6 +53,10 @@
     it 'can accept a ssl private key option' do
       described_class.new({ 'github-app-key': '/nonexistent' }, [], {})
     end
+
+    it 'can accept a deploy-spec option' do
+      described_class.new({ :'deploy-spec' => true }, [], {})
+    end
   end
 
   describe "with no-force" do

spec/unit/module/base_spec.rb

@@ -28,6 +28,52 @@
     end
   end
 
+  describe 'deleting the spec dir' do
+    let(:module_org) { "coolorg" }
+    let(:module_name) { "coolmod" }
+    let(:title) { "#{module_org}-#{module_name}" }
+    let(:dirname) { Pathname.new(Dir.mktmpdir) }
+    let(:spec_path) { dirname + module_name + 'spec' }
+
+    before(:each) do
+      logger = double("logger")
+      allow_any_instance_of(described_class).to receive(:logger).and_return(logger)
+      allow(logger).to receive(:debug2).with(any_args)
+      allow(logger).to receive(:info).with(any_args)
+    end
+
+    it 'removes the spec directory' do
+      FileUtils.mkdir_p(spec_path)
+      m = described_class.new(title, dirname, {})
+      m.maybe_delete_spec_dir
+      expect(Dir.exist?(spec_path)).to eq false
+    end
+
+    it 'detects a symlink and deletes the target' do
+      Dir.mkdir(dirname + module_name)
+      target_dir = Dir.mktmpdir
+      FileUtils.ln_s(target_dir, spec_path)
+      m = described_class.new(title, dirname, {})
+      m.maybe_delete_spec_dir
+      expect(Dir.exist?(target_dir)).to eq false
+    end
+
+    it 'does not remove the spec directory if deploy_spec is set' do
+      FileUtils.mkdir_p(spec_path)
+      m = described_class.new(title, dirname, {deploy_spec: true})
+      m.maybe_delete_spec_dir
+      expect(Dir.exist?(spec_path)).to eq true
+    end
+
+    it 'does not remove the spec directory if spec_deletable is false' do
+      FileUtils.mkdir_p(spec_path)
+      m = described_class.new(title, dirname, {})
+      m.spec_deletable = false
+      m.maybe_delete_spec_dir
+      expect(Dir.exist?(spec_path)).to eq true
+    end
+  end
+
   describe "path variables" do
     it "uses the module name as the name" do
       m = described_class.new('eight_hundred', '/moduledir', [])

spec/unit/module/forge_spec.rb

@@ -78,6 +78,7 @@
       allow_any_instance_of(described_class).to receive(:logger).and_return(logger_dbl)
 
       allow(logger_dbl).to receive(:info).with(/Deploying module to.*/)
+      allow(logger_dbl).to receive(:debug2).with(/No spec dir detected/)
       expect(logger_dbl).to receive(:warn).with(/puppet forge module.*puppetlabs-corosync.*has been deprecated/i)
 
       subject.sync
@@ -90,6 +91,7 @@
       allow_any_instance_of(described_class).to receive(:logger).and_return(logger_dbl)
 
       allow(logger_dbl).to receive(:info).with(/Deploying module to.*/)
+      allow(logger_dbl).to receive(:debug2).with(/No spec dir detected/)
       expect(logger_dbl).to_not receive(:warn).with(/puppet forge module.*puppetlabs-corosync.*has been deprecated/i)
 
       subject.sync
@@ -165,6 +167,23 @@
   describe "#sync" do
     subject { described_class.new('branan/eight_hundred', fixture_modulepath, { version: '8.0.0' }) }
 
+    context "syncing the repo" do
+      let(:module_org) { "coolorg" }
+      let(:module_name) { "coolmod" }
+      let(:title) { "#{module_org}-#{module_name}" }
+      let(:dirname) { Pathname.new(Dir.mktmpdir) }
+      let(:spec_path) { dirname + module_name + 'spec' }
+      subject { described_class.new(title, dirname, {}) }
+
+      it 'defaults to deleting the spec dir' do
+        FileUtils.mkdir_p(spec_path)
+        expect(subject).to receive(:status).and_return(:absent)
+        expect(subject).to receive(:install)
+        subject.sync
+        expect(Dir.exist?(spec_path)).to eq false
+      end
+    end
+
     it 'does nothing when the module is in sync' do
       allow(subject).to receive(:status).and_return :insync
 

spec/unit/module/git_spec.rb

@@ -89,6 +89,23 @@
     end
   end
 
+  describe 'syncing the repo' do
+    let(:module_org) { "coolorg" }
+    let(:module_name) { "coolmod" }
+    let(:title) { "#{module_org}-#{module_name}" }
+    let(:dirname) { Pathname.new(Dir.mktmpdir) }
+    let(:spec_path) { dirname + module_name + 'spec' }
+    subject { described_class.new(title, dirname, {}) }
+
+    it 'defaults to deleting the spec dir' do
+      FileUtils.mkdir_p(spec_path)
+      allow(mock_repo).to receive(:resolve).with('master').and_return('abc123')
+      allow(mock_repo).to receive(:sync)
+      subject.sync
+      expect(Dir.exist?(spec_path)).to eq false
+    end
+  end
+
   describe "determining the status" do
     subject do
       described_class.new(

spec/unit/module/svn_spec.rb

@@ -119,6 +119,24 @@
     end
   end
 
+  describe 'the default spec dir' do
+    let(:module_org) { "coolorg" }
+    let(:module_name) { "coolmod" }
+    let(:title) { "#{module_org}-#{module_name}" }
+    let(:dirname) { Pathname.new(Dir.mktmpdir) }
+    let(:spec_path) { dirname + module_name + 'spec' }
+    subject { described_class.new(title, dirname, {}) }
+
+    it 'gets deleted by default' do
+
+      FileUtils.mkdir_p(spec_path)
+      expect(subject).to receive(:status).and_return(:absent)
+      expect(subject).to receive(:install).and_return(nil)
+      subject.sync
+      expect(Dir.exist?(spec_path)).to eq false
+    end
+  end
+
   describe "synchronizing" do
 
     subject { described_class.new('foo', '/moduledir', :svn => 'https://github.com/adrienthebo/r10k-fixture-repo', :rev => 123) }

spec/unit/module_loader/puppetfile_spec.rb

@@ -107,6 +107,18 @@
       expect(subject.modules.collect(&:name)).not_to include('test_module')
     end
 
+    it 'should set :spec_deletable to true for modules in the basedir' do
+      module_opts = { git: '[email protected]:puppet/test_module.git' }
+      subject.add_module('puppet/test_module', module_opts)
+      expect(subject.modules[0].spec_deletable).to be true
+    end
+
+    it 'should set :spec_deletable to false for modules outside the basedir' do
+      module_opts = { git: '[email protected]:puppet/test_module.git', install_path: 'some/path' }
+      subject.add_module('puppet/test_module', module_opts)
+      expect(subject.modules[0].spec_deletable).to be false
+    end
+
     it 'should accept non-Forge modules with a hash arg' do
       module_opts = { git: '[email protected]:puppet/test_module.git' }
 
@@ -165,6 +177,7 @@
       mod = instance_double('R10K::Module::Base', name: 'conflict', origin: :puppetfile, 'origin=': nil)
       loader = R10K::ModuleLoader::Puppetfile.new(basedir: basedir, environment: env)
       allow(env).to receive(:'module_conflicts?').with(mod).and_return(true)
+      allow(mod).to receive(:spec_deletable=)
 
       expect(R10K::Module).to receive(:new).with('conflict', anything, anything, anything).and_return(mod)
       expect { loader.add_module('conflict', {}) }.not_to change { loader.modules }