Implement purge_behavior parameter (#60)

* Implement `purge_behavior` parameter

This allows for specifying classes or data keys that should be present
or have defined values, without removing or modifying other data already
present in the node group.

* Implement tests for purge_behavior

* Fix bugs in purge_behavior implementation

Also simplify merge logic

* Clean up rspec tests

Update all mocking to rspec mocks, update gem versions

* Increase test coverage, simplify purge_behavior

There should be no need to define a special insync? method for the
classes and data properties, given that we set `should` to an
appropriate value, and Ruby compares hashes in a way such that key order
is irrelevant.

* Further simplify tests

For easier reading

Author: reidmv

Diff for: CHANGELOG.md

@@ -1,3 +1,7 @@
+## Unreleased
+
+- Add `purge_behavior` parameter to node\_group resource type
+
 ## 2019-12-27 - Release 0.7.3
 
 ## Summary

Diff for: Gemfile

@@ -1,10 +1,10 @@
 source "https://rubygems.org"
+gem 'pry'
 
 group :test do
   gem "rake"
 #  gem "puppet", ENV['PUPPET_VERSION'] || '~> 3.7.3'
   gem "rspec-puppet", :git => 'https://github.com/rodjek/rspec-puppet.git'
-#  gem "puppetlabs_spec_helper"
   gem 'rspec-puppet-utils', :git => 'https://github.com/Accuity/rspec-puppet-utils.git'
   gem 'hiera-puppet-helper', :git => 'https://github.com/bobtfish/hiera-puppet-helper.git'
   # there seems to be a bug with puppet-blacksmith and metadata-json-lint
@@ -26,6 +26,6 @@ end
 source 'https://rubygems.org'
 
 gem 'puppet', nil || ENV['PUPPET_VERSION']
-gem 'puppetlabs_spec_helper', '0.10.3'
-gem 'webmock', '1.22.1'
+gem 'puppetlabs_spec_helper'
+gem 'webmock'
 gem 'puppetclassify', '0.1.7'

Diff for: README.md

@@ -135,6 +135,14 @@ This parameter is supported for PE >=2017.3.x.
 
   Default (empty hash): `{}`
 
+* `purge_behavior`
+
+  Defines how purging of classification or data will be handled. By default, or when set to `all`, the node\_group resource will ensure classes and data are matched exactly, and remove any values not described by the resource. When set to `none`, the node\_group resource will ensure data and classes described are present with the prescribed values, but will not remove other classification, or other data, present in the node group. The `data` setting purges only data values, and the `classes` setting purges only classes values.
+
+  Default: `all`
+
+  Values: `all`, `data`, `classes`, `none`
+
 ## Tasks
 
 ### update_classes

Diff for: lib/puppet/type/node_group.rb

@@ -6,6 +6,11 @@
   newparam(:name, :namevar => true) do
     desc 'This is the common name for the node group'
   end
+  newparam(:purge_behavior) do
+    desc 'Whether or not to remove data or class parameters not specified'
+    newvalues(:none, :data, :classes, :all)
+    defaultto :all
+  end
   newproperty(:id) do
     desc 'The ID of the group'
     validate do |value|
@@ -45,6 +50,17 @@
     munge do |value|
       PuppetX::Node_manager::Common.sort_hash(value)
     end
+    def should
+      case @resource[:purge_behavior]
+      when :classes, :all
+        super
+      else
+        a = @resource.property(:classes).retrieve || {}
+        b = shouldorig.first
+        merged = a.merge(b) { |k, x, y| x.merge(y) }
+        PuppetX::Node_manager::Common.sort_hash(merged)
+      end
+    end
   end
   newproperty(:data) do
     desc 'Data applied to this group'
@@ -56,6 +72,17 @@
     munge do |value|
       PuppetX::Node_manager::Common.sort_hash(value)
     end
+    def should
+      case @resource[:purge_behavior]
+      when :data, :all
+        super
+      else
+        a = @resource.property(:data).retrieve || {}
+        b = shouldorig.first
+        merged = a.merge(b) { |k, x, y| x.merge(y) }
+        PuppetX::Node_manager::Common.sort_hash(merged)
+      end
+    end
   end
   newproperty(:description) do
     desc 'Description of this group'

Diff for: spec/functions/node_groups_spec.rb

@@ -71,20 +71,26 @@
   }
 
   before do
-    YAML.stubs(:load_file).returns({
-      'server' => 'stubserver',
-      'port'   => '8080',
-    })
+    allow(YAML).to(receive(:load_file))
+               .with('/dev/null/classifier.yaml')
+               .and_return({'server' => 'stubserver', 'port' => '8080'})
+
+    allow(File).to receive(:read).and_call_original
+    allow(File).to receive(:read).with(%r{/dev/null/ssl/}).and_return('helloworld')
+
+    allow(OpenSSL::X509::Certificate).to(receive(:new))
+                                     .and_return(double("Certificate", :save => true))
+
+    allow(OpenSSL::PKey::RSA).to(receive(:new))
+                             .and_return(double("Key", :save => true))
+
     stub_request(
       :get,
       'https://stubserver:8080/classifier-api/v1/groups',
     ).to_return(
       :status => 200,
       :body   => groups_response
     )
-    File.stubs(:read).returns('helloworld')
-    OpenSSL::X509::Certificate.stubs(:new) {mock_model(OpenSSL::X509::Certificate, :save => true)}
-    OpenSSL::PKey::RSA.stubs(:new) {mock_model(OpenSSL::PKey::RSA, :save => true)}
   end
 
   describe 'without an argument' do

Diff for: spec/integration/puppet/provider/node_group/https_spec.rb

@@ -1,4 +1,4 @@
-require 'puppetlabs_spec_helper/module_spec_helper'
+require 'spec_helper'
 require 'webmock/rspec'
 
 describe Puppet::Type.type(:node_group).provider(:https) do
@@ -58,11 +58,18 @@
   end
 
   before do
-      YAML.stubs(:load_file).with('/dev/null/classifier.yaml')
-        .returns({'server' => 'stubserver', 'port' => '8080'})
-    File.stubs(:read).returns('helloworld')
-    OpenSSL::X509::Certificate.stubs(:new) {mock_model(OpenSSL::X509::Certificate, :save => true)}
-    OpenSSL::PKey::RSA.stubs(:new) {mock_model(OpenSSL::PKey::RSA, :save => true)}
+    allow(YAML).to(receive(:load_file))
+               .with('/dev/null/classifier.yaml')
+               .and_return({'server' => 'stubserver', 'port' => '8080'})
+
+    allow(File).to receive(:read).and_call_original
+    allow(File).to receive(:read).with(%r{/dev/null/ssl/}).and_return('helloworld')
+
+    allow(OpenSSL::X509::Certificate).to(receive(:new))
+                                     .and_return(double("Certificate", :save => true))
+
+    allow(OpenSSL::PKey::RSA).to(receive(:new))
+                             .and_return(double("Key", :save => true))
   end
 
   describe "#instances" do

Diff for: spec/spec_helper.rb

@@ -1,10 +1,11 @@
-require 'puppetlabs_spec_helper/module_spec_helper'
 require 'rspec-puppet-utils'
 
 # Uncomment this to show coverage report, also useful for debugging
 #at_exit { RSpec::Puppet::Coverage.report! }
 
-#RSpec.configure do |c|
-#    c.formatter = 'documentation'
-#    config.mock_with :rspec
-#end
+RSpec.configure do |c|
+  c.mock_with :rspec
+  c.mock_framework = :rspec
+end
+
+require 'puppetlabs_spec_helper/module_spec_helper'

Diff for: spec/unit/puppet/type/node_group_spec.rb

@@ -1,7 +1,6 @@
-require 'puppetlabs_spec_helper/module_spec_helper'
+require 'spec_helper'
 
 describe Puppet::Type.type(:node_group) do
-
   it "should allow agent-specified environment" do
     expect {
       Puppet::Type.type(:node_group).new(
@@ -73,4 +72,128 @@
     }.to_not raise_error
   end
 
+  describe "purge_behavior" do
+    let(:resource_hash) do
+      {
+        :name        => 'test_group',
+        :environment => 'test_env',
+        :data        => {
+          'data::class1' => { 'param1' => 'resource',
+                              'param2' => 'resource' },
+          'data::class2' => { 'param1' => 'resource',
+                              'param2' => 'resource' },
+        },
+        :classes     => {
+          'classes::class1' => { 'param1' => 'resource',
+                                 'param2' => 'resource' },
+        },
+      }
+    end
+
+    let(:existing_data) do
+      { 'data::class1' => { 'param1' => 'existing',
+                            'param3' => 'existing' },
+        'data::class3' => { 'param1' => 'existing',
+                            'param2' => 'existing' }}
+    end
+    let(:merged_data) do
+      { "data::class1" => { "param1" => "resource",
+                            "param2" => "resource",
+                            "param3" => "existing"},
+        "data::class2" => { "param1" => "resource",
+                            "param2" => "resource"},
+        "data::class3" => { "param1" => "existing",
+                            "param2" => "existing"}}
+    end
+
+    let(:existing_classes) do
+      { 'classes::class1' => { 'param1' => 'existing',
+                               'param3' => 'existing' },
+        'classes::class3' => { 'param1' => 'existing',
+                               'param2' => 'existing' }}
+    end
+    let(:merged_classes) do
+      { "classes::class1" => { "param1" => "resource",
+                               "param2" => "resource",
+                               "param3" => "existing"},
+        "classes::class3" => { "param1" => "existing",
+                               "param2" => "existing"}}
+    end
+
+    it "should match classes and data exactly by default" do
+      rsrc = described_class.new(resource_hash)
+      allow(rsrc.property(:data)).to receive(:retrieve).and_return(existing_data)
+      allow(rsrc.property(:classes)).to receive(:retrieve).and_return(existing_classes)
+      expect(rsrc.property(:data).should).to eq resource_hash[:data]
+      expect(rsrc.property(:classes).should).to eq resource_hash[:classes]
+    end
+
+    it "should merge in classes and data when set to :none" do
+      rsrc = described_class.new(resource_hash.merge(:purge_behavior => 'none'))
+      allow(rsrc.property(:data)).to receive(:retrieve).and_return(existing_data)
+      allow(rsrc.property(:classes)).to receive(:retrieve).and_return(existing_classes)
+      expect(rsrc.property(:data).should).to eq (merged_data)
+      expect(rsrc.property(:classes).should).to eq (merged_classes)
+    end
+
+    it "should merge in classes and match data exactly when set to :data" do
+      rsrc = described_class.new(resource_hash.merge(:purge_behavior => 'data'))
+      allow(rsrc.property(:data)).to receive(:retrieve).and_return(existing_data)
+      allow(rsrc.property(:classes)).to receive(:retrieve).and_return(existing_classes)
+      expect(rsrc.property(:data).should).to eq (resource_hash[:data])
+      expect(rsrc.property(:classes).should).to eq (merged_classes)
+    end
+
+    it "should merge in data and match classes exactly when set to :classes" do
+      rsrc = described_class.new(resource_hash.merge(:purge_behavior => 'classes'))
+      allow(rsrc.property(:data)).to receive(:retrieve).and_return(existing_data)
+      allow(rsrc.property(:classes)).to receive(:retrieve).and_return(existing_classes)
+      expect(rsrc.property(:data).should).to eq (merged_data)
+      expect(rsrc.property(:classes).should).to eq (resource_hash[:classes])
+    end
+  end
+
+  describe ".insync? for data, classes" do
+    let(:hash) do
+      {
+        'class1' => { 'param1' => 'value1',
+                      'param2' => 'value2' },
+        'class2' => { 'param1' => 'value1',
+                      'param2' => 'value2' },
+        'class3' => { 'param1' => 'value1',
+                      'param2' => 'value2' },
+      }
+    end
+    let(:resource) do
+      described_class.new({
+        :name        => 'test_group',
+        :environment => 'test_env',
+        :classes     => hash,
+        :data        => hash,
+      })
+    end
+
+    before(:each) do
+      allow(resource.property(:data)).to receive(:should).and_return(hash)
+      allow(resource.property(:classes)).to receive(:should).and_return(hash)
+    end
+
+    it 'is insync when `is` and `should` are identical' do
+      expect(resource.property(:data).insync?(hash)).to eq(true)
+      expect(resource.property(:classes).insync?(hash)).to eq(true)
+    end
+
+    it 'is insync when `is` and `should` are identical but have different ordering' do
+      reverse_hash = hash.to_a.map{ |i| [i[0], i[1].to_a.reverse.to_h] }.reverse.to_h
+      expect(resource.property(:data).insync?(reverse_hash)).to eq(true)
+      expect(resource.property(:classes).insync?(reverse_hash)).to eq(true)
+    end
+
+    it 'is not insync when `is` is only a subset of `should`' do
+      subset = hash.select { |k| k != 'class2' }
+      expect(resource.property(:data).insync?(subset)).to eq(false)
+      expect(resource.property(:classes).insync?(subset)).to eq(false)
+    end
+  end
+
 end