MODULES-1596 - Repository repeatedly destroyed/created with force
authorMorgan Haskel <morgan@puppetlabs.com>
Fri, 26 Dec 2014 23:27:20 +0000 (15:27 -0800)
committerMorgan Haskel <morgan@puppetlabs.com>
Fri, 26 Dec 2014 23:49:52 +0000 (15:49 -0800)
The `retrieve` method was calling `create` and `destroy` on every run
with `force => true`. Retrieve should not be making any changes to the
system, so removed that code, and updated `working_copy_exists` to make
sure that the directory not only contains a `.git` directory, but also
if `source` is specified it also matches `#{path}/.git/config` so that
it will overwrite a git repo with a different source.

Updated tests to not check for the old broken behavior. Added a regression test.

lib/puppet/provider/vcsrepo/git.rb
lib/puppet/type/vcsrepo.rb
spec/acceptance/beaker/git/clone/negative/clone_over_different_exiting_repo.rb
spec/acceptance/clone_repo_spec.rb
spec/acceptance/modules_1596_spec.rb [new file with mode: 0644]
spec/unit/puppet/provider/vcsrepo/git_spec.rb

index 1c6588c..b1e556d 100644 (file)
@@ -83,7 +83,11 @@ Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo)
   end
 
   def working_copy_exists?
-    File.directory?(File.join(@resource.value(:path), '.git'))
+    if @resource.value(:source) and File.exists?(File.join(@resource.value(:path), '.git', 'config'))
+      File.readlines(File.join(@resource.value(:path), '.git', 'config')).grep(/#{@resource.value(:source)}/).any?
+    else
+      File.directory?(File.join(@resource.value(:path), '.git'))
+    end
   end
 
   def exists?
index f678389..b8836d4 100644 (file)
@@ -100,16 +100,6 @@ Puppet::Type.newtype(:vcsrepo) do
       prov = @resource.provider
       if prov
         if prov.working_copy_exists?
-          if @resource.value(:force)
-            if noop
-              notice "Noop Mode - Would have deleted repository and re-created from latest"
-            else
-              notice "Deleting current repository before recloning"
-              prov.destroy
-              notice "Create repository from latest"
-              prov.create
-            end
-          end
           (@should.include?(:latest) && prov.latest?) ? :latest : :present
         elsif prov.class.feature?(:bare_repositories) and prov.bare_exists?
           :bare
index 6826673..1e3b4bb 100644 (file)
@@ -35,8 +35,7 @@ hosts.each do |host|
     }
     EOS
 
-    apply_manifest_on(host, pp, :catch_failures => true)
-    apply_manifest_on(host, pp, :catch_changes  => true)
+    apply_manifest_on(host, pp, :expect_failures => true)
   end
 
   step 'verify original repo was not replaced' do
index f3e77db..4e9293b 100644 (file)
@@ -350,9 +350,7 @@ describe 'clones a remote repo' do
         }
         EOS
 
-        apply_manifest(pp, :catch_changes => true) do |r|
-          expect(r.stdout).to match(/Noop Mode/)
-        end
+        apply_manifest(pp, :catch_changes => true)
       end
     end
   end
diff --git a/spec/acceptance/modules_1596_spec.rb b/spec/acceptance/modules_1596_spec.rb
new file mode 100644 (file)
index 0000000..fa36285
--- /dev/null
@@ -0,0 +1,72 @@
+require 'spec_helper_acceptance'
+
+tmpdir = default.tmpdir('vcsrepo')
+
+describe 'clones a remote repo' do
+  before(:all) do
+    my_root = File.expand_path(File.join(File.dirname(__FILE__), '..'))
+    shell("mkdir -p #{tmpdir}") # win test
+  end
+
+  after(:all) do
+    shell("rm -rf #{tmpdir}/vcsrepo")
+  end
+
+  context 'force with a remote' do
+    it 'clones from remote' do
+      pp = <<-EOS
+      vcsrepo { "#{tmpdir}/vcsrepo":
+        ensure   => present,
+        provider => git,
+        source   => 'https://github.com/puppetlabs/puppetlabs-vcsrepo',
+        force    => true,
+      }
+      EOS
+
+      # Run it twice to test for idempotency
+      apply_manifest(pp, :catch_failures => true)
+      # need to create a file to make sure we aren't destroying the repo
+      # because fun fact, if you call destroy/create in 'retrieve' puppet won't
+      # register that any changes happen, because that method isn't supposed to
+      # be making any changes.
+      shell("touch #{tmpdir}/vcsrepo/foo")
+      apply_manifest(pp, :catch_changes  => true)
+    end
+
+    describe file("#{tmpdir}/vcsrepo/foo") do
+      it { is_expected.to be_file }
+    end
+  end
+
+  context 'force over an existing repo' do
+    it 'clones from remote' do
+      pp = <<-EOS
+      vcsrepo { "#{tmpdir}/vcsrepo":
+        ensure   => present,
+        provider => git,
+        source   => 'https://github.com/puppetlabs/puppetlabs-vcsrepo',
+        force    => true,
+      }
+      EOS
+
+      pp2 = <<-EOS
+      vcsrepo { "#{tmpdir}/vcsrepo":
+        ensure   => present,
+        provider => git,
+        source   => 'https://github.com/puppetlabs/puppetlabs-stdlib',
+        force    => true,
+      }
+      EOS
+
+
+      apply_manifest(pp, :catch_failures => true)
+      # create a file to make sure we're destroying the repo
+      shell("touch #{tmpdir}/vcsrepo/foo")
+      apply_manifest(pp2, :catch_failures  => true)
+    end
+
+    describe file("#{tmpdir}/vcsrepo/foo") do
+      it { is_expected.to_not be_file }
+    end
+  end
+end
index 122eb62..edc7202 100644 (file)
@@ -175,28 +175,6 @@ branches
         provider.expects(:git).with('checkout', '--force', resource.value(:revision))
         provider.create
       end
-      it "should warn about destroying it using force and noop attribute" do
-        resource[:force] = true
-        resource[:noop] = true
-        resource.delete(:revision)
-        provider.expects(:working_copy_exists?).returns(true)
-
-        provider.expects(:destroy).never
-        provider.expects(:create).never
-        Puppet::Type::Vcsrepo::Ensure.any_instance.expects(:send_log).with(:notice, "Noop Mode - Would have deleted repository and re-created from latest")
-        provider.resource.retrieve
-      end
-      it "should warn about destroying it using force and global noop" do
-        resource[:force] = true
-        Puppet[:noop] = true
-        resource.delete(:revision)
-        provider.expects(:working_copy_exists?).returns(true)
-
-        provider.expects(:destroy).never
-        provider.expects(:create).never
-        Puppet::Type::Vcsrepo::Ensure.any_instance.expects(:send_log).with(:notice, "Noop Mode - Would have deleted repository and re-created from latest")
-        provider.resource.retrieve
-      end
     end
 
     context "when the path is not empty and not a repository" do