From c9efa7ec526b491ee5739aea81062601aa9c2f43 Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Sat, 28 Jan 2017 14:15:15 -0800
Subject: [PATCH 01/12] fix #5 - Breaks when a Package title != name

---
 lib/puppet/type/aptly_purge.rb                | 22 ++++++++++
 spec/acceptance/purges_safely_spec.rb         |  1 +
 spec/acceptance/title_and_name_differ_spec.rb | 44 +++++++++++++++++++
 3 files changed, 67 insertions(+)
 create mode 100644 spec/acceptance/title_and_name_differ_spec.rb

diff --git a/lib/puppet/type/aptly_purge.rb b/lib/puppet/type/aptly_purge.rb
index d599dde..4d600ac 100644
--- a/lib/puppet/type/aptly_purge.rb
+++ b/lib/puppet/type/aptly_purge.rb
@@ -46,6 +46,16 @@ def generate
       p.provider.is_a?(Puppet::Type::Package::ProviderDpkg)
     end.each do |r|
       catalog_r = catalog.resource(r.ref)
+      if catalog_r.nil?
+        # A 'Package[<title>]' resource could not be found in the catalog.
+        # What about resources where title != name?
+        #   package {'arbitrarytitle': name => 'pkgname'}
+        # Because 'Package[<title>]' and 'Package[<name>]' are not synonyms,
+        # we need to check if the package resource we are after has been aliased
+        # to a different title.
+        ref = find_resource_alias(["Package", r.name, :held_apt])
+        catalog_r = catalog.resource(ref) if ref
+      end
       if catalog_r.nil?
         unmanaged_packages << r
       else
@@ -157,4 +167,16 @@ def get_purges
       p
     end
   end
+
+  # ref is of the form: ["Package", "name", :provider]
+  def find_resource_alias ref
+    @resource_aliases ||= catalog.instance_variable_get(:@aliases)
+
+    result = @resource_aliases.find do |ref_str, aliases|
+      aliases.find do |candidate_ref|
+          candidate_ref == ref
+      end
+    end
+    return result.nil? ? nil : result.first
+  end
 end
diff --git a/spec/acceptance/purges_safely_spec.rb b/spec/acceptance/purges_safely_spec.rb
index 3f46270..21c683e 100644
--- a/spec/acceptance/purges_safely_spec.rb
+++ b/spec/acceptance/purges_safely_spec.rb
@@ -7,6 +7,7 @@
       package { 'puppetlabs-release-pc1': }
       package { 'puppet-agent': }
       package { 'fortunes': }
+      package { 'openssh-server': }
       include package_purging::config
       aptly_purge { 'packages': }
     EOS
diff --git a/spec/acceptance/title_and_name_differ_spec.rb b/spec/acceptance/title_and_name_differ_spec.rb
new file mode 100644
index 0000000..332ac96
--- /dev/null
+++ b/spec/acceptance/title_and_name_differ_spec.rb
@@ -0,0 +1,44 @@
+require 'spec_helper_acceptance'
+
+describe 'title_and_name_differ' do
+  before :all do
+    hosts.each do |host|
+      install_package host, 'dict-jargon'
+      expect(check_for_package host, 'dictd').to be true
+      install_package host, 'fortunes'
+      expect(check_for_package host, 'fortunes-min').to be true
+      # same as `include package_purging::config`, saves a Puppet run
+      create_remote_file host, '/etc/apt/apt.conf.d/99always-purge', "APT::Get::Purge \"true\";\n";
+    end
+  end
+
+  context 'manifest contains a package resource where title != name' do
+    it 'should apply' do
+      m = <<-EOS
+        package { 'ubuntu-minimal': }
+        package { 'puppetlabs-release-pc1': }
+        package { 'puppet-agent': }
+        package { 'openssh-server': }
+        package {'fortunespkg':
+          name => 'fortunes',
+        }
+        aptly_purge {'packages': }
+      EOS
+      apply_manifest m, :debug => true
+      expect(@result.exit_code).to eq 0
+    end
+
+    describe package('dict-jargon') do
+      it { should_not be_installed }
+    end
+    describe package('dictd') do
+      it { should_not be_installed }
+    end
+    describe package('fortunes') do
+      it { should be_installed }
+    end
+    describe package('fortunes-min') do  # a dependency of fortunes
+      it { should be_installed }
+    end
+  end
+end

From d94d97d2dc02a6ef26fb760df80a884e0f3f5471 Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Wed, 8 Feb 2017 07:39:50 -0800
Subject: [PATCH 02/12] PR feedback

---
 lib/puppet/type/aptly_purge.rb | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/lib/puppet/type/aptly_purge.rb b/lib/puppet/type/aptly_purge.rb
index 4d600ac..384fc35 100644
--- a/lib/puppet/type/aptly_purge.rb
+++ b/lib/puppet/type/aptly_purge.rb
@@ -45,17 +45,7 @@ def generate
     package.instances.select do |p|
       p.provider.is_a?(Puppet::Type::Package::ProviderDpkg)
     end.each do |r|
-      catalog_r = catalog.resource(r.ref)
-      if catalog_r.nil?
-        # A 'Package[<title>]' resource could not be found in the catalog.
-        # What about resources where title != name?
-        #   package {'arbitrarytitle': name => 'pkgname'}
-        # Because 'Package[<title>]' and 'Package[<name>]' are not synonyms,
-        # we need to check if the package resource we are after has been aliased
-        # to a different title.
-        ref = find_resource_alias(["Package", r.name, :held_apt])
-        catalog_r = catalog.resource(ref) if ref
-      end
+      catalog_r = catalog.resource(r.ref) || find_resource_alias(["Package", r.name, :held_apt])
       if catalog_r.nil?
         unmanaged_packages << r
       else
@@ -169,6 +159,7 @@ def get_purges
   end
 
   # ref is of the form: ["Package", "name", :provider]
+  # returns nil if no alias exist
   def find_resource_alias ref
     @resource_aliases ||= catalog.instance_variable_get(:@aliases)
 
@@ -177,6 +168,6 @@ def find_resource_alias ref
           candidate_ref == ref
       end
     end
-    return result.nil? ? nil : result.first
+    return result.nil? ? nil : catalog.resource(result.first)
   end
 end

From 0227beb54ed388abe569c88675e9d0b911306ce3 Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Thu, 16 Feb 2017 10:38:16 -0800
Subject: [PATCH 03/12] move towards an actual "noop", apt-mark testing

---
 lib/puppet/type/aptly_purge.rb        | 23 ++++++++++++++++-------
 spec/acceptance/purges_safely_spec.rb | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/lib/puppet/type/aptly_purge.rb b/lib/puppet/type/aptly_purge.rb
index 384fc35..654e856 100644
--- a/lib/puppet/type/aptly_purge.rb
+++ b/lib/puppet/type/aptly_purge.rb
@@ -13,8 +13,6 @@
 be removed.  This type takes the resulting list and generates Puppet package
 resources with ensure=>absent for any unmanaged resources that apt-get would
 autoremove.
-
-NOTE: This type writes into the apt-mark system, even when run in noop mode.
 EOD
 
   newparam(:title) do
@@ -28,6 +26,10 @@
     defaultto false
   end
 
+  newparam(:purge, :boolean => true, :parent => Puppet::Parameter::Boolean) do
+    defaultto false
+  end
+
   newparam(:hold, :boolean => true, :parent => Puppet::Parameter::Boolean) do
     defaultto false
   end
@@ -64,12 +66,17 @@ def generate
       # You can't hold a package that isn't installed yet, so this should
       # really be done after all packages are installed.
 
-      holds = managed_packages.select do |p|
+      pinned = managed_packages.select do |p|
         # What we really want is to grab all packages with an explicit version
         # This is a cheap reproduction of what we really want.
         ![:latest, :absent, :present].include?(p.parameters[:ensure].value)
-      end.map do |p|
-        Puppet::Type.type(:dpkg_hold).new({ :name => p[:name], :ensure => :present })
+      end
+
+      holds = []
+      unless Puppet.settings[:noop] or self[:noop]
+        holds = pinned.each do |p|
+          Puppet::Type.type(:dpkg_hold).new({ :name => p[:name], :ensure => :present })
+        end
       end
     end
 
@@ -90,9 +97,11 @@ def generate
     # B is marked as 'auto' as it should
     # If some other process has marked A as auto, B will get ensure=>absent
     # Then dpkg will remove both A and B.  This is bad!
-    mark_manual managed_package_names, outfile
+    unless Puppet.settings[:noop] or self[:noop]
+      mark_manual managed_package_names, outfile
 
-    mark_auto unmanaged_package_names, outfile
+      mark_auto unmanaged_package_names, outfile
+    end
 
     apt_would_purge = get_purges()
     Puppet.debug "apt_would_purge: #{apt_would_purge.to_a}"
diff --git a/spec/acceptance/purges_safely_spec.rb b/spec/acceptance/purges_safely_spec.rb
index 21c683e..ec22bb0 100644
--- a/spec/acceptance/purges_safely_spec.rb
+++ b/spec/acceptance/purges_safely_spec.rb
@@ -13,6 +13,17 @@
     EOS
   end
 
+  def get_packages_state host
+    apt_mark = on(host, 'apt-mark showauto 2>&1').stdout
+    result = apt_mark.lines.each_with_object({}) { |line, h| h[line.rstrip] = 'auto' }
+    apt_mark = on(host, 'apt-mark showmanual 2>&1').stdout
+    apt_mark.lines.each_with_object(result) do |line, h|
+      package = line.rstrip
+      raise "Package #{package} appears both in apt-mark showauto and showmanual" if h.has_key?(package)
+      h[package] = 'manual'
+    end
+  end
+
   before :all do
     hosts.each do |host|
       # install dict-jargon outside of Puppet
@@ -38,6 +49,10 @@
       on host, 'puppet config set ordering random'
       on host, 'puppet config print ordering | grep -q random'
       expect(@result.exit_code).to eq 0
+
+      packages_state = get_packages_state host
+      # at this stage, apt-mark knows that 'fortunes' has been manually installed
+      expect(packages_state['fortunes']).to eq 'manual'
     end
   end
 

From f7dd5d760644681050362efc1e80b4b355606238 Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Thu, 16 Feb 2017 14:29:27 -0800
Subject: [PATCH 04/12] some more apt-mark tests

---
 spec/acceptance/purges_safely_spec.rb | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/spec/acceptance/purges_safely_spec.rb b/spec/acceptance/purges_safely_spec.rb
index ec22bb0..dfa9998 100644
--- a/spec/acceptance/purges_safely_spec.rb
+++ b/spec/acceptance/purges_safely_spec.rb
@@ -51,7 +51,8 @@ def get_packages_state host
       expect(@result.exit_code).to eq 0
 
       packages_state = get_packages_state host
-      # at this stage, apt-mark knows that 'fortunes' has been manually installed
+      expect(packages_state['dict-jargon']).to eq 'manual'
+      expect(packages_state['dictd']).to eq 'auto'
       expect(packages_state['fortunes']).to eq 'manual'
     end
   end
@@ -68,12 +69,25 @@ def get_packages_state host
       expect(@result.exit_code).to eq 0
     end
 
+    # The manifest has been applied, no packages will be removed until the next run
+    # because the settings at "include package_purging::config" have just been put
+    # in place.
     describe package('dict-jargon') do
       it { should be_installed }
     end
     describe package('dictd') do
       it { should be_installed }
     end
+
+    # Only 'fortunes' is in the catalog.
+    # 'dict-jargon' has been installed outside of puppet, 'dictd' is one
+    # of its dependencies. 'dict-jargon' gets apt-mark'ed as 'auto'.
+    it 'should correctly apt-mark packages' do
+      packages_state = get_packages_state default_node
+      expect(packages_state['dict-jargon']).to eq 'auto'
+      expect(packages_state['dictd']).to eq 'auto'
+      expect(packages_state['fortunes']).to eq 'manual'
+    end
   end
 
   context 'aptly_purge with unmanaged packages on the system, second puppet run' do
@@ -96,4 +110,14 @@ def get_packages_state host
     end
   end
 
+  context 'aptly_purge in noop mode' do
+    install_package default_node, 'dict-jargon'
+    # dictd gets automatically installed as a dependency of dict-jargon
+    expect(check_for_package default_node, 'dictd').to be true
+    it 'before puppet runs' do
+      packages_state = get_packages_state default_node
+      expect(packages_state['dict-jargon']).to eq 'manual'
+      expect(packages_state['dict']).to eq 'auto'
+    end
+  end
 end

From 91a9ca104d725c5b7bce11e3e92bc8dd24d7335c Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Thu, 16 Feb 2017 14:56:21 -0800
Subject: [PATCH 05/12] "noop does not apt-mark" test

---
 spec/acceptance/purges_safely_spec.rb | 38 ++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/spec/acceptance/purges_safely_spec.rb b/spec/acceptance/purges_safely_spec.rb
index dfa9998..0346902 100644
--- a/spec/acceptance/purges_safely_spec.rb
+++ b/spec/acceptance/purges_safely_spec.rb
@@ -111,13 +111,45 @@ def get_packages_state host
   end
 
   context 'aptly_purge in noop mode' do
-    install_package default_node, 'dict-jargon'
-    # dictd gets automatically installed as a dependency of dict-jargon
-    expect(check_for_package default_node, 'dictd').to be true
     it 'before puppet runs' do
+      install_package default_node, 'dict-jargon'
+      # dictd gets automatically installed as a dependency of dict-jargon
+      expect(check_for_package default_node, 'dictd').to be true
       packages_state = get_packages_state default_node
       expect(packages_state['dict-jargon']).to eq 'manual'
       expect(packages_state['dict']).to eq 'auto'
+      expect(packages_state['fortunes']).to eq 'manual'
+    end
+
+    it 'should not apt-mark packages' do
+      m = <<-EOS
+          package { 'ubuntu-minimal': }
+          package { 'puppetlabs-release-pc1': }
+          package { 'puppet-agent': }
+          package { 'fortunes': }
+          package { 'openssh-server': }
+          include package_purging::config
+          aptly_purge { 'packages': noop => true}
+      EOS
+      apply_manifest(m, :debug => true)
+      expect(@result.exit_code).to eq 0
+      packages_state = get_packages_state default_node
+      expect(packages_state['dict-jargon']).to eq 'manual'
+      expect(packages_state['dict']).to eq 'auto'
+      expect(packages_state['fortunes']).to eq 'manual'
+    end
+
+    describe package('dict-jargon') do
+      it { should be_installed }
+    end
+    describe package('dictd') do
+      it { should be_installed }
+    end
+    describe package('fortunes') do
+      it { should be_installed }
+    end
+    describe package('fortunes-min') do  # a dependency of fortunes
+      it { should be_installed }
     end
   end
 end

From 40354b18558f07409db2a217377889cf6e27426f Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Sun, 19 Feb 2017 14:20:11 -0800
Subject: [PATCH 06/12] purge toggle

---
 lib/puppet/type/aptly_purge.rb                |  2 +-
 spec/acceptance/purges_safely_spec.rb         | 94 ++++++++++++++++++-
 spec/acceptance/title_and_name_differ_spec.rb |  4 +-
 3 files changed, 96 insertions(+), 4 deletions(-)

diff --git a/lib/puppet/type/aptly_purge.rb b/lib/puppet/type/aptly_purge.rb
index 654e856..2dc3675 100644
--- a/lib/puppet/type/aptly_purge.rb
+++ b/lib/puppet/type/aptly_purge.rb
@@ -97,7 +97,7 @@ def generate
     # B is marked as 'auto' as it should
     # If some other process has marked A as auto, B will get ensure=>absent
     # Then dpkg will remove both A and B.  This is bad!
-    unless Puppet.settings[:noop] or self[:noop]
+    unless !(@parameters[:purge] && @parameters[:purge].value) or Puppet.settings[:noop] or self[:noop]
       mark_manual managed_package_names, outfile
 
       mark_auto unmanaged_package_names, outfile
diff --git a/spec/acceptance/purges_safely_spec.rb b/spec/acceptance/purges_safely_spec.rb
index 0346902..6ef0003 100644
--- a/spec/acceptance/purges_safely_spec.rb
+++ b/spec/acceptance/purges_safely_spec.rb
@@ -9,7 +9,9 @@
       package { 'fortunes': }
       package { 'openssh-server': }
       include package_purging::config
-      aptly_purge { 'packages': }
+      aptly_purge { 'packages':
+        purge => true,
+      }
     EOS
   end
 
@@ -129,7 +131,95 @@ def get_packages_state host
           package { 'fortunes': }
           package { 'openssh-server': }
           include package_purging::config
-          aptly_purge { 'packages': noop => true}
+          aptly_purge { 'packages': noop => true }
+      EOS
+      apply_manifest(m, :debug => true)
+      expect(@result.exit_code).to eq 0
+      packages_state = get_packages_state default_node
+      expect(packages_state['dict-jargon']).to eq 'manual'
+      expect(packages_state['dict']).to eq 'auto'
+      expect(packages_state['fortunes']).to eq 'manual'
+    end
+
+    describe package('dict-jargon') do
+      it { should be_installed }
+    end
+    describe package('dictd') do
+      it { should be_installed }
+    end
+    describe package('fortunes') do
+      it { should be_installed }
+    end
+    describe package('fortunes-min') do  # a dependency of fortunes
+      it { should be_installed }
+    end
+  end
+
+  context 'aptly_purge with purge => false' do
+    it 'before puppet runs' do
+      install_package default_node, 'dict-jargon'
+      # dictd gets automatically installed as a dependency of dict-jargon
+      expect(check_for_package default_node, 'dictd').to be true
+      packages_state = get_packages_state default_node
+      expect(packages_state['dict-jargon']).to eq 'manual'
+      expect(packages_state['dict']).to eq 'auto'
+      expect(packages_state['fortunes']).to eq 'manual'
+    end
+
+    it 'should not apt-mark packages' do
+      m = <<-EOS
+          package { 'ubuntu-minimal': }
+          package { 'puppetlabs-release-pc1': }
+          package { 'puppet-agent': }
+          package { 'fortunes': }
+          package { 'openssh-server': }
+          include package_purging::config
+          aptly_purge { 'packages':
+            purge => false,
+          }
+      EOS
+      apply_manifest(m, :debug => true)
+      expect(@result.exit_code).to eq 0
+      packages_state = get_packages_state default_node
+      expect(packages_state['dict-jargon']).to eq 'manual'
+      expect(packages_state['dict']).to eq 'auto'
+      expect(packages_state['fortunes']).to eq 'manual'
+    end
+
+    describe package('dict-jargon') do
+      it { should be_installed }
+    end
+    describe package('dictd') do
+      it { should be_installed }
+    end
+    describe package('fortunes') do
+      it { should be_installed }
+    end
+    describe package('fortunes-min') do  # a dependency of fortunes
+      it { should be_installed }
+    end
+  end
+
+  context 'aptly_purge by default' do
+    it 'before puppet runs' do
+      install_package default_node, 'dict-jargon'
+      # dictd gets automatically installed as a dependency of dict-jargon
+      expect(check_for_package default_node, 'dictd').to be true
+      packages_state = get_packages_state default_node
+      expect(packages_state['dict-jargon']).to eq 'manual'
+      expect(packages_state['dict']).to eq 'auto'
+      expect(packages_state['fortunes']).to eq 'manual'
+    end
+
+    it 'should not apt-mark packages' do
+      m = <<-EOS
+          package { 'ubuntu-minimal': }
+          package { 'puppetlabs-release-pc1': }
+          package { 'puppet-agent': }
+          package { 'fortunes': }
+          package { 'openssh-server': }
+          include package_purging::config
+          aptly_purge { 'packages': }
       EOS
       apply_manifest(m, :debug => true)
       expect(@result.exit_code).to eq 0
diff --git a/spec/acceptance/title_and_name_differ_spec.rb b/spec/acceptance/title_and_name_differ_spec.rb
index 332ac96..9491643 100644
--- a/spec/acceptance/title_and_name_differ_spec.rb
+++ b/spec/acceptance/title_and_name_differ_spec.rb
@@ -22,7 +22,9 @@
         package {'fortunespkg':
           name => 'fortunes',
         }
-        aptly_purge {'packages': }
+        aptly_purge {'packages':
+          purge => true,
+        }
       EOS
       apply_manifest m, :debug => true
       expect(@result.exit_code).to eq 0

From 09b1aafa12e8b59bbd098a46f530c2620905a8ac Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Mon, 20 Feb 2017 07:09:58 -0800
Subject: [PATCH 07/12] more/better purge noop tests

---
 lib/puppet/type/aptly_purge.rb        |  34 +++++---
 spec/acceptance/purges_safely_spec.rb | 109 ++++++--------------------
 2 files changed, 43 insertions(+), 100 deletions(-)

diff --git a/lib/puppet/type/aptly_purge.rb b/lib/puppet/type/aptly_purge.rb
index 2dc3675..6a96b73 100644
--- a/lib/puppet/type/aptly_purge.rb
+++ b/lib/puppet/type/aptly_purge.rb
@@ -73,7 +73,7 @@ def generate
       end
 
       holds = []
-      unless Puppet.settings[:noop] or self[:noop]
+      unless noop?
         holds = pinned.each do |p|
           Puppet::Type.type(:dpkg_hold).new({ :name => p[:name], :ensure => :present })
         end
@@ -97,7 +97,7 @@ def generate
     # B is marked as 'auto' as it should
     # If some other process has marked A as auto, B will get ensure=>absent
     # Then dpkg will remove both A and B.  This is bad!
-    unless !(@parameters[:purge] && @parameters[:purge].value) or Puppet.settings[:noop] or self[:noop]
+    if should_purge?
       mark_manual managed_package_names, outfile
 
       mark_auto unmanaged_package_names, outfile
@@ -106,17 +106,21 @@ def generate
     apt_would_purge = get_purges()
     Puppet.debug "apt_would_purge: #{apt_would_purge.to_a}"
 
-    removes = unmanaged_packages.select do |r|
-      # This is the crux.  We intersect the list of packages Puppet isn't
-      # managing with the list of packages that apt would purge.
-      apt_would_purge.include?(r.name)
-    end.each do |resource|
-      resource[:ensure] = 'absent'
-      @parameters.each do |name, param|
-        resource[name] = param.value if param.metaparam?
-      end
-
-      resource.purging
+    if should_purge?
+        removes = unmanaged_packages.select do |r|
+          # This is the crux.  We intersect the list of packages Puppet isn't
+          # managing with the list of packages that apt would purge.
+          apt_would_purge.include?(r.name)
+        end.each do |resource|
+          resource[:ensure] = 'absent'
+          @parameters.each do |name, param|
+            resource[name] = param.value if param.metaparam?
+          end
+
+          resource.purging
+        end
+    else
+        removes = []
     end
 
     holds + removes
@@ -179,4 +183,8 @@ def find_resource_alias ref
     end
     return result.nil? ? nil : catalog.resource(result.first)
   end
+
+  def should_purge?
+    @parameters[:purge].value && !noop?
+  end
 end
diff --git a/spec/acceptance/purges_safely_spec.rb b/spec/acceptance/purges_safely_spec.rb
index 6ef0003..f584f5d 100644
--- a/spec/acceptance/purges_safely_spec.rb
+++ b/spec/acceptance/purges_safely_spec.rb
@@ -112,18 +112,8 @@ def get_packages_state host
     end
   end
 
-  context 'aptly_purge in noop mode' do
-    it 'before puppet runs' do
-      install_package default_node, 'dict-jargon'
-      # dictd gets automatically installed as a dependency of dict-jargon
-      expect(check_for_package default_node, 'dictd').to be true
-      packages_state = get_packages_state default_node
-      expect(packages_state['dict-jargon']).to eq 'manual'
-      expect(packages_state['dict']).to eq 'auto'
-      expect(packages_state['fortunes']).to eq 'manual'
-    end
-
-    it 'should not apt-mark packages' do
+  RSpec.shared_examples 'aptly_purge noop' do |test_case|
+    let(:test_manifest) {
       m = <<-EOS
           package { 'ubuntu-minimal': }
           package { 'puppetlabs-release-pc1': }
@@ -131,31 +121,10 @@ def get_packages_state host
           package { 'fortunes': }
           package { 'openssh-server': }
           include package_purging::config
-          aptly_purge { 'packages': noop => true }
       EOS
-      apply_manifest(m, :debug => true)
-      expect(@result.exit_code).to eq 0
-      packages_state = get_packages_state default_node
-      expect(packages_state['dict-jargon']).to eq 'manual'
-      expect(packages_state['dict']).to eq 'auto'
-      expect(packages_state['fortunes']).to eq 'manual'
-    end
+      m + test_case
+    }
 
-    describe package('dict-jargon') do
-      it { should be_installed }
-    end
-    describe package('dictd') do
-      it { should be_installed }
-    end
-    describe package('fortunes') do
-      it { should be_installed }
-    end
-    describe package('fortunes-min') do  # a dependency of fortunes
-      it { should be_installed }
-    end
-  end
-
-  context 'aptly_purge with purge => false' do
     it 'before puppet runs' do
       install_package default_node, 'dict-jargon'
       # dictd gets automatically installed as a dependency of dict-jargon
@@ -164,24 +133,20 @@ def get_packages_state host
       expect(packages_state['dict-jargon']).to eq 'manual'
       expect(packages_state['dict']).to eq 'auto'
       expect(packages_state['fortunes']).to eq 'manual'
+
+      # Purposely mark dict-jargon as auto. We really want it to look like
+      # something that could be purged and make sure it gets left alone
+      # when running with noop or purge => false .
+      on default_node, 'apt-mark auto dict-jargon'
+      packages_state = get_packages_state default_node
+      expect(packages_state['dict-jargon']).to eq 'auto'
     end
 
     it 'should not apt-mark packages' do
-      m = <<-EOS
-          package { 'ubuntu-minimal': }
-          package { 'puppetlabs-release-pc1': }
-          package { 'puppet-agent': }
-          package { 'fortunes': }
-          package { 'openssh-server': }
-          include package_purging::config
-          aptly_purge { 'packages':
-            purge => false,
-          }
-      EOS
-      apply_manifest(m, :debug => true)
+      apply_manifest(test_manifest, :debug => true)
       expect(@result.exit_code).to eq 0
       packages_state = get_packages_state default_node
-      expect(packages_state['dict-jargon']).to eq 'manual'
+      expect(packages_state['dict-jargon']).to eq 'auto'
       expect(packages_state['dict']).to eq 'auto'
       expect(packages_state['fortunes']).to eq 'manual'
     end
@@ -200,46 +165,16 @@ def get_packages_state host
     end
   end
 
-  context 'aptly_purge by default' do
-    it 'before puppet runs' do
-      install_package default_node, 'dict-jargon'
-      # dictd gets automatically installed as a dependency of dict-jargon
-      expect(check_for_package default_node, 'dictd').to be true
-      packages_state = get_packages_state default_node
-      expect(packages_state['dict-jargon']).to eq 'manual'
-      expect(packages_state['dict']).to eq 'auto'
-      expect(packages_state['fortunes']).to eq 'manual'
-    end
+  context 'aptly_purge in noop mode' do
+    it_behaves_like 'aptly_purge noop', "aptly_purge { 'packages': noop => true }"
+  end
 
-    it 'should not apt-mark packages' do
-      m = <<-EOS
-          package { 'ubuntu-minimal': }
-          package { 'puppetlabs-release-pc1': }
-          package { 'puppet-agent': }
-          package { 'fortunes': }
-          package { 'openssh-server': }
-          include package_purging::config
-          aptly_purge { 'packages': }
-      EOS
-      apply_manifest(m, :debug => true)
-      expect(@result.exit_code).to eq 0
-      packages_state = get_packages_state default_node
-      expect(packages_state['dict-jargon']).to eq 'manual'
-      expect(packages_state['dict']).to eq 'auto'
-      expect(packages_state['fortunes']).to eq 'manual'
-    end
+  context 'aptly_purge with purge => false' do
+    it_behaves_like 'aptly_purge noop', "aptly_purge { 'packages': purge => false }"
+  end
 
-    describe package('dict-jargon') do
-      it { should be_installed }
-    end
-    describe package('dictd') do
-      it { should be_installed }
-    end
-    describe package('fortunes') do
-      it { should be_installed }
-    end
-    describe package('fortunes-min') do  # a dependency of fortunes
-      it { should be_installed }
-    end
+  context 'aptly_purge by default' do
+    it_behaves_like 'aptly_purge noop', "aptly_purge { 'packages': }"
   end
+
 end

From 17869de501080f80155c61970df5c1ef22ca8afa Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Mon, 27 Feb 2017 10:09:21 -0800
Subject: [PATCH 08/12] ability to "un-hold" packages - comprehensive "hold"
 tests

---
 lib/puppet/type/aptly_purge.rb       |  41 ++++-
 spec/acceptance/holds_safely_spec.rb | 237 +++++++++++++++++++++++++++
 2 files changed, 272 insertions(+), 6 deletions(-)
 create mode 100644 spec/acceptance/holds_safely_spec.rb

diff --git a/lib/puppet/type/aptly_purge.rb b/lib/puppet/type/aptly_purge.rb
index 6a96b73..46a8835 100644
--- a/lib/puppet/type/aptly_purge.rb
+++ b/lib/puppet/type/aptly_purge.rb
@@ -61,8 +61,7 @@ def generate
     Puppet.debug "unmanaged_package_names: #{unmanaged_package_names}"
 
     holds = []
-
-    if @parameters[:hold] then
+    if should_hold? then
       # You can't hold a package that isn't installed yet, so this should
       # really be done after all packages are installed.
 
@@ -72,9 +71,9 @@ def generate
         ![:latest, :absent, :present].include?(p.parameters[:ensure].value)
       end
 
-      holds = []
+      Puppet.debug "pinned: #{pinned.map(&:name)}"
       unless noop?
-        holds = pinned.each do |p|
+        holds = pinned.map do |p|
           Puppet::Type.type(:dpkg_hold).new({ :name => p[:name], :ensure => :present })
         end
       end
@@ -89,6 +88,7 @@ def generate
 EOS
       raise Puppet::Error.new("Could not purge packages during this Puppet run")
     end
+    Puppet.debug "holds: #{holds.map(&:name)}"
 
     # If we don't set managed packages to noauto here, it is possible to
     # set ensure=>absent on an unmanaged package that a managed package
@@ -123,7 +123,32 @@ def generate
         removes = []
     end
 
-    holds + removes
+    # un-hold packages
+    if should_hold?
+      dpkg_selections = Puppet::Util::Execution.execute('dpkg --get-selections')
+      dpkg_selections = Hash[*dpkg_selections.lines.map {|l| l.rstrip.split(/\s+/,2)}.flatten]
+      to_be_removed = Hash[removes.map(&:name).zip([])]
+      # unmanaged packages that are not already slated for removal
+      unholds = unmanaged_packages.select do |p|
+        !to_be_removed.include?(p.name)
+      end
+      # managed packages with ensure => present
+      unholds += managed_packages.select do |p|
+        p.parameters[:ensure].value == :present
+      end
+      # if the packages to be un-held are currently held, generate a dpkg_hold resource
+      unholds = unholds.select do |p|
+        dpkg_selections.include?(p.name) &&
+        dpkg_selections[p.name] == 'hold'
+      end.map do |p|
+        Puppet::Type.type(:dpkg_hold).new({ :name => p[:name], :ensure => :absent })
+      end
+    else
+      unholds = []
+    end
+    Puppet.debug "unholds: #{unholds.map(&:name)}"
+
+    holds + unholds + removes
   end
 
   private
@@ -185,6 +210,10 @@ def find_resource_alias ref
   end
 
   def should_purge?
-    @parameters[:purge].value && !noop?
+    @parameters[:purge] && @parameters[:purge].value && !noop?
+  end
+
+  def should_hold?
+    @parameters[:hold] && @parameters[:hold].value && !noop?
   end
 end
diff --git a/spec/acceptance/holds_safely_spec.rb b/spec/acceptance/holds_safely_spec.rb
new file mode 100644
index 0000000..5b5a849
--- /dev/null
+++ b/spec/acceptance/holds_safely_spec.rb
@@ -0,0 +1,237 @@
+require 'spec_helper_acceptance'
+
+describe 'package_holding_with_apt' do
+  def get_installed_version host, package_name
+    line = on(host, "dpkg -s #{package_name} | grep ^Version").stdout
+    version = line.gsub(/\s+/,'').split(':',2).last
+    version.empty? ? nil : version
+  end
+
+  def get_candidate_version host, package_name
+    line = on(host, "apt-cache policy #{package_name} | grep Candidate: | head -1").stdout
+    version = line.gsub(/\s+/,'').split(':',2).last
+    version.empty? ? nil : version
+  end
+
+  def get_packages_state host
+    packages_state = on(host, 'dpkg-query -W --showformat \'${Status} ${Package}\n\'').stdout
+    packages_state.lines.each_with_object({}) do |line, h|
+      if match = line.match(/^(\S+) +(\S+) +(\S+) (\S+)$/)
+        desired, error, status, name = match.captures
+        h[name] = desired
+      end
+    end
+  end
+
+  def set_package_state host, package, state
+    on(host, "echo #{package} #{state} | dpkg --set-selections")
+  end
+
+  before :all do
+    @managed_packages = [
+      'ubuntu-minimal',
+      'puppetlabs-release-pc1',
+      'puppet-agent',
+      'openssh-server',
+      'dict-jargon',
+      'fortunes',
+    ]
+    @package_versions = {}
+
+    hosts.each do |host|
+      install_package host, 'dict-jargon'
+      expect(check_for_package host, 'dictd').to be true
+      install_package host, 'fortunes'
+      expect(check_for_package host, 'fortunes-min').to be true
+      # same as `include package_purging::config`, saves a Puppet run
+      create_remote_file host, '/etc/apt/apt.conf.d/99always-purge', "APT::Get::Purge \"true\";\n";
+
+      @managed_packages.each do |p|
+        @package_versions[p] = get_installed_version(host, p) || get_candidate_version(host, p)
+      end
+
+      @managed_packages.each do |p|
+        set_package_state default_node, p, 'install'
+      end
+      packages_state = get_packages_state default_node
+      expect(packages_state.values_at(*@managed_packages)).to eq(['install'] * @managed_packages.length)
+    end
+  end
+
+  context 'manifest manages a few packages, all of them pin a specific version' do
+    it 'should hold all the packages' do
+      managed_packages = @managed_packages
+      m = @package_versions.map do |p, v|
+        "package { '#{p}': ensure => '#{v}' }"
+      end.join("\n")
+      m += <<-EOS
+
+        aptly_purge {'packages':
+          hold => true,
+        }
+      EOS
+      apply_manifest m, :debug => true
+      expect(@result.exit_code).to eq 0
+
+      packages_state = get_packages_state default_node
+      # our packages are held
+      expect(packages_state.values_at(*managed_packages)).to eq(['hold'] * managed_packages.length)
+      # everything else isn't
+      expect(packages_state.values_at(*(packages_state.keys - managed_packages))).not_to include('hold')
+    end
+  end
+
+  context '"fortunes" is not managed' do
+    it 'should stop holding "fortunes" as it\'s no longer in the manifest' do
+      managed_packages = @managed_packages - ['fortunes']
+      m = managed_packages.map do |p|
+        "package { '#{p}': ensure => '#{@package_versions[p]}' }"
+      end.join("\n")
+      m += <<-EOS
+
+        aptly_purge {'packages':
+          hold => true,
+        }
+      EOS
+      apply_manifest m, :debug => true
+      expect(@result.exit_code).to eq 0
+
+      packages_state = get_packages_state default_node
+      # managed packages are held
+      expect(packages_state.values_at(*managed_packages)).to eq(['hold'] * managed_packages.length)
+      # fortunes isn't
+      expect(packages_state['fortunes']).to eq('install')
+      # because the "purge" parameter defaults to false, the package is still installed
+      expect(package('fortunes')).to be_installed
+    end
+  end
+
+  context 'in the manifest, "fortunes" is set to ensure => present' do
+    it 'should not hold "fortunes" as it\'s not pinned to a specific version' do
+      pinned_packages = @managed_packages - ['fortunes']
+      m = pinned_packages.map do |p|
+        "package { '#{p}': ensure => '#{@package_versions[p]}' }"
+      end.join("\n")
+      m += <<-EOS
+
+        package{'fortunes': ensure => present}
+        aptly_purge {'packages':
+          hold => true,
+        }
+      EOS
+      apply_manifest m, :debug => true
+      expect(@result.exit_code).to eq 0
+
+      packages_state = get_packages_state default_node
+      # pinned packages are held
+      expect(packages_state.values_at(*pinned_packages)).to eq(['hold'] * pinned_packages.length)
+      # fortunes isn't
+      expect(packages_state['fortunes']).to eq('install')
+      expect(package('fortunes')).to be_installed
+    end
+  end
+
+  context '"fortunes" used to be held, now it is set to ensure => present' do
+    it 'should not hold "fortunes" as it\'s not pinned to a specific version' do
+      set_package_state default_node, 'fortunes', 'hold'
+      packages_state = get_packages_state default_node
+      expect(packages_state['fortunes']).to eq('hold')
+
+      pinned_packages = @managed_packages - ['fortunes']
+      m = pinned_packages.map do |p|
+        "package { '#{p}': ensure => '#{@package_versions[p]}' }"
+      end.join("\n")
+      m += <<-EOS
+
+        package{'fortunes': ensure => present}
+        aptly_purge {'packages':
+          hold => true,
+        }
+      EOS
+      apply_manifest m, :debug => true
+      expect(@result.exit_code).to eq 0
+
+      packages_state = get_packages_state default_node
+      # pinned packages are held
+      expect(packages_state.values_at(*pinned_packages)).to eq(['hold'] * pinned_packages.length)
+      # fortunes isn't
+      expect(packages_state['fortunes']).to eq('install')
+      # because the "purge" parameter defaults to false, the package is still installed
+      expect(package('fortunes')).to be_installed
+    end
+  end
+
+  context 'un-holding "fortunes" when declared with title != name' do
+    it 'should not hold "fortunes" as it\'s not pinned to a specific version' do
+      set_package_state default_node, 'fortunes', 'hold'
+      packages_state = get_packages_state default_node
+      expect(packages_state['fortunes']).to eq('hold')
+
+      pinned_packages = @managed_packages - ['fortunes']
+      m = pinned_packages.map do |p|
+        "package { '#{p}': ensure => '#{@package_versions[p]}' }"
+      end.join("\n")
+      m += <<-EOS
+
+        package {'fortunespkg':
+          name => 'fortunes',
+          ensure => present,
+        }
+        aptly_purge {'packages':
+          hold => true,
+        }
+      EOS
+      apply_manifest m, :debug => true
+      expect(@result.exit_code).to eq 0
+
+      packages_state = get_packages_state default_node
+      # pinned packages are held
+      expect(packages_state.values_at(*pinned_packages)).to eq(['hold'] * pinned_packages.length)
+      # fortunes isn't
+      expect(packages_state['fortunes']).to eq('install')
+      # because the "purge" parameter defaults to false, the package is still installed
+      expect(package('fortunes')).to be_installed
+    end
+  end
+
+
+  RSpec.shared_examples 'aptly_purge (hold) noop' do |test_case|
+    it 'maeks no changes' do
+      managed_packages = @managed_packages
+
+      managed_packages.each do |p|
+        set_package_state default_node, p, 'install'
+      end
+      packages_state = get_packages_state default_node
+      expect(packages_state.values_at(*managed_packages)).to eq(['install'] * managed_packages.length)
+
+      m = managed_packages.map do |p|
+        "package { '#{p}': ensure => '#{@package_versions[p]}' }"
+      end.join("\n")
+      m += <<-EOS
+
+        #{test_case}
+      EOS
+      apply_manifest m, :debug => true
+      expect(@result.exit_code).to eq 0
+
+      packages_state = get_packages_state default_node
+      # no managed/pinned packages are held
+      expect(packages_state.values_at(*managed_packages)).to eq(['install'] * managed_packages.length)
+      # so do all the other packages on the system
+      expect(packages_state.values_at(*(packages_state.keys - managed_packages))).not_to include('hold')
+    end
+  end
+
+  context 'aptly_purge (hold) in noop mode' do
+    it_behaves_like 'aptly_purge (hold) noop', "aptly_purge { 'packages': noop => true }"
+  end
+
+  context 'aptly_purge (hold) with hold => false' do
+    it_behaves_like 'aptly_purge (hold) noop', "aptly_purge { 'packages': hold => false }"
+  end
+
+  context 'aptly_purge (hold) by default' do
+    it_behaves_like 'aptly_purge (hold) noop', "aptly_purge { 'packages': }"
+  end
+end

From f780bf2fec0443008c298a58267211dc0d2f485f Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Sun, 5 Mar 2017 12:32:16 -0800
Subject: [PATCH 09/12] rename the tests so they run in a specific order

---
 .../{purges_safely_spec.rb => 00_purges_safely_spec.rb}           | 0
 ...e_and_name_differ_spec.rb => 01_title_and_name_differ_spec.rb} | 0
 spec/acceptance/{holds_safely_spec.rb => 02_holds_safely_spec.rb} | 0
 3 files changed, 0 insertions(+), 0 deletions(-)
 rename spec/acceptance/{purges_safely_spec.rb => 00_purges_safely_spec.rb} (100%)
 rename spec/acceptance/{title_and_name_differ_spec.rb => 01_title_and_name_differ_spec.rb} (100%)
 rename spec/acceptance/{holds_safely_spec.rb => 02_holds_safely_spec.rb} (100%)

diff --git a/spec/acceptance/purges_safely_spec.rb b/spec/acceptance/00_purges_safely_spec.rb
similarity index 100%
rename from spec/acceptance/purges_safely_spec.rb
rename to spec/acceptance/00_purges_safely_spec.rb
diff --git a/spec/acceptance/title_and_name_differ_spec.rb b/spec/acceptance/01_title_and_name_differ_spec.rb
similarity index 100%
rename from spec/acceptance/title_and_name_differ_spec.rb
rename to spec/acceptance/01_title_and_name_differ_spec.rb
diff --git a/spec/acceptance/holds_safely_spec.rb b/spec/acceptance/02_holds_safely_spec.rb
similarity index 100%
rename from spec/acceptance/holds_safely_spec.rb
rename to spec/acceptance/02_holds_safely_spec.rb

From 49d808f3a5573fe4871ebb75ad6b514cda708253 Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Sun, 5 Mar 2017 13:01:12 -0800
Subject: [PATCH 10/12] avoid unnecessary examples

---
 spec/acceptance/00_purges_safely_spec.rb      | 53 +++++--------------
 .../01_title_and_name_differ_spec.rb          | 17 ++----
 2 files changed, 18 insertions(+), 52 deletions(-)

diff --git a/spec/acceptance/00_purges_safely_spec.rb b/spec/acceptance/00_purges_safely_spec.rb
index f584f5d..d833da4 100644
--- a/spec/acceptance/00_purges_safely_spec.rb
+++ b/spec/acceptance/00_purges_safely_spec.rb
@@ -56,29 +56,21 @@ def get_packages_state host
       expect(packages_state['dict-jargon']).to eq 'manual'
       expect(packages_state['dictd']).to eq 'auto'
       expect(packages_state['fortunes']).to eq 'manual'
+      expect(check_for_package host, 'ubuntu-minimal').to be true
     end
   end
 
-  describe package('ubuntu-minimal') do
-    it { should be_installed }
-  end
-
   context 'aptly_purge with unmanaged packages on the system, first puppet run' do
     it 'should not remove any packages' do
       # aptly_purge generates the list of packages to purge at "parse time"
       # before/require ordering constraints don't work on it
       apply_manifest(package_purging_manifest)
       expect(@result.exit_code).to eq 0
-    end
-
-    # The manifest has been applied, no packages will be removed until the next run
-    # because the settings at "include package_purging::config" have just been put
-    # in place.
-    describe package('dict-jargon') do
-      it { should be_installed }
-    end
-    describe package('dictd') do
-      it { should be_installed }
+      # The manifest has been applied, no packages will be removed until the next run
+      # because the settings at "include package_purging::config" have just been put
+      # in place.
+      expect(package('dict-jargon')).to be_installed
+      expect(package('dictd')).to be_installed
     end
 
     # Only 'fortunes' is in the catalog.
@@ -96,19 +88,10 @@ def get_packages_state host
     it 'should remove unmanaged packages' do
       apply_manifest(package_purging_manifest, :debug => true)
       expect(@result.exit_code).to eq 0
-    end
-
-    describe package('dict-jargon') do
-      it { should_not be_installed }
-    end
-    describe package('dictd') do
-      it { should_not be_installed }
-    end
-    describe package('fortunes') do
-      it { should be_installed }
-    end
-    describe package('fortunes-min') do  # a dependency of fortunes
-      it { should be_installed }
+      expect(package('dict-jargon')).to_not be_installed
+      expect(package('dictd')).to_not be_installed
+      expect(package('fortunes')).to be_installed
+      expect(package('fortunes-min')).to be_installed  # a dependency of fortune
     end
   end
 
@@ -149,19 +132,11 @@ def get_packages_state host
       expect(packages_state['dict-jargon']).to eq 'auto'
       expect(packages_state['dict']).to eq 'auto'
       expect(packages_state['fortunes']).to eq 'manual'
-    end
 
-    describe package('dict-jargon') do
-      it { should be_installed }
-    end
-    describe package('dictd') do
-      it { should be_installed }
-    end
-    describe package('fortunes') do
-      it { should be_installed }
-    end
-    describe package('fortunes-min') do  # a dependency of fortunes
-      it { should be_installed }
+      expect(package('dict-jargon')).to be_installed
+      expect(package('dictd')).to be_installed
+      expect(package('fortunes')).to be_installed
+      expect(package('fortunes-min')).to be_installed  # a dependency of fortune
     end
   end
 
diff --git a/spec/acceptance/01_title_and_name_differ_spec.rb b/spec/acceptance/01_title_and_name_differ_spec.rb
index 9491643..2bdf39a 100644
--- a/spec/acceptance/01_title_and_name_differ_spec.rb
+++ b/spec/acceptance/01_title_and_name_differ_spec.rb
@@ -28,19 +28,10 @@
       EOS
       apply_manifest m, :debug => true
       expect(@result.exit_code).to eq 0
-    end
-
-    describe package('dict-jargon') do
-      it { should_not be_installed }
-    end
-    describe package('dictd') do
-      it { should_not be_installed }
-    end
-    describe package('fortunes') do
-      it { should be_installed }
-    end
-    describe package('fortunes-min') do  # a dependency of fortunes
-      it { should be_installed }
+      expect(package('dict-jargon')).to_not be_installed
+      expect(package('dictd')).to_not be_installed
+      expect(package('fortunes')).to be_installed
+      expect(package('fortunes-min')).to be_installed  # a dependency of fortune
     end
   end
 end

From b47c1a3b2180badc8dfcf790ec9cf654ee4b6629 Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Mon, 6 Mar 2017 04:47:01 -0800
Subject: [PATCH 11/12] style/consistency fixes

---
 lib/puppet/type/aptly_purge.rb | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/lib/puppet/type/aptly_purge.rb b/lib/puppet/type/aptly_purge.rb
index 46a8835..552cbf8 100644
--- a/lib/puppet/type/aptly_purge.rb
+++ b/lib/puppet/type/aptly_purge.rb
@@ -60,7 +60,6 @@ def generate
     unmanaged_package_names = unmanaged_packages.map(&:name)
     Puppet.debug "unmanaged_package_names: #{unmanaged_package_names}"
 
-    holds = []
     if should_hold? then
       # You can't hold a package that isn't installed yet, so this should
       # really be done after all packages are installed.
@@ -77,7 +76,10 @@ def generate
           Puppet::Type.type(:dpkg_hold).new({ :name => p[:name], :ensure => :present })
         end
       end
+    else
+      holds = []
     end
+    Puppet.debug "holds: #{holds.map(&:name)}"
 
     unless all_packages_synced
       notice <<EOS
@@ -88,7 +90,6 @@ def generate
 EOS
       raise Puppet::Error.new("Could not purge packages during this Puppet run")
     end
-    Puppet.debug "holds: #{holds.map(&:name)}"
 
     # If we don't set managed packages to noauto here, it is possible to
     # set ensure=>absent on an unmanaged package that a managed package
@@ -107,21 +108,22 @@ def generate
     Puppet.debug "apt_would_purge: #{apt_would_purge.to_a}"
 
     if should_purge?
-        removes = unmanaged_packages.select do |r|
-          # This is the crux.  We intersect the list of packages Puppet isn't
-          # managing with the list of packages that apt would purge.
-          apt_would_purge.include?(r.name)
-        end.each do |resource|
-          resource[:ensure] = 'absent'
-          @parameters.each do |name, param|
-            resource[name] = param.value if param.metaparam?
-          end
-
-          resource.purging
+      removes = unmanaged_packages.select do |r|
+        # This is the crux.  We intersect the list of packages Puppet isn't
+        # managing with the list of packages that apt would purge.
+        apt_would_purge.include?(r.name)
+      end.each do |resource|
+        resource[:ensure] = 'absent'
+        @parameters.each do |name, param|
+          resource[name] = param.value if param.metaparam?
+        end
+
+        resource.purging
         end
     else
-        removes = []
+      removes = []
     end
+    Puppet.debug "removes: #{removes.map(&:name)}"
 
     # un-hold packages
     if should_hold?
@@ -136,7 +138,7 @@ def generate
       unholds += managed_packages.select do |p|
         p.parameters[:ensure].value == :present
       end
-      # if the packages to be un-held are currently held, generate a dpkg_hold resource
+      # if the packages to be un-held are currently held, generate a dpkg_hold resource with ensure => absent
       unholds = unholds.select do |p|
         dpkg_selections.include?(p.name) &&
         dpkg_selections[p.name] == 'hold'

From 7b0ac682044d1e6f922d1734e8acf8e799664d2b Mon Sep 17 00:00:00 2001
From: Giuliano Cioffi <gcioffi@yelp.com>
Date: Mon, 6 Mar 2017 06:26:36 -0800
Subject: [PATCH 12/12] PR feedback - typo

---
 spec/acceptance/02_holds_safely_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/acceptance/02_holds_safely_spec.rb b/spec/acceptance/02_holds_safely_spec.rb
index 5b5a849..9a478e6 100644
--- a/spec/acceptance/02_holds_safely_spec.rb
+++ b/spec/acceptance/02_holds_safely_spec.rb
@@ -196,7 +196,7 @@ def set_package_state host, package, state
 
 
   RSpec.shared_examples 'aptly_purge (hold) noop' do |test_case|
-    it 'maeks no changes' do
+    it 'makes no changes' do
       managed_packages = @managed_packages
 
       managed_packages.each do |p|