(PDK-511) Add canonicalization checking if puppet strict is on.

lib/puppet/resource_api.rb

@@ -174,6 +174,8 @@ def my_provider
         result        = Puppet::Resource.new(self.class, title)
         current_state = my_provider.get(context).find { |h| h[namevar_name] == title }
 
+        strict_check(current_state) if current_state && (definition.key?(:features) && definition[:features].include?('canonicalize'))
+
         # require 'pry'; binding.pry
 
         if current_state
@@ -209,6 +211,36 @@ def my_provider
         raise 'Execution encountered an error' if context.failed?
       end
 
+      define_method(:strict_check) do |current_state|
+        return if Puppet.settings[:strict] == :off
+
+        # if strict checking is on we must notify if the values are changed by canonicalize
+        # make a deep copy to perform the operation on and to compare against later
+        state_clone = Marshal.load(Marshal.dump(current_state))
+        state_clone = my_provider.canonicalize(context, [state_clone]).first
+
+        # compare the clone against the current state to see if changes have been made by canonicalize
+        return unless state_clone && (current_state != state_clone)
+
+        #:nocov:
+        # codecov fails to register this multiline as covered, even though simplecov does.
+        message = <<MESSAGE.strip
+#{definition[:name]}[#{current_state[:name]}]#get has not provided canonicalized values.
+Returned values:       #{current_state.inspect}
+Canonicalized values:  #{state_clone.inspect}
+MESSAGE
+        #:nocov:
+
+        case Puppet.settings[:strict]
+        when :warning
+          Puppet.warning(message)
+        when :error
+          raise Puppet::Error, message
+        end
+
+        return nil
+      end
+
       define_singleton_method(:context) do
         @context ||= PuppetContext.new(definition[:name])
       end

spec/acceptance/device_spec.rb

@@ -2,23 +2,60 @@
 require 'tempfile'
 
 RSpec.describe 'exercising a device provider' do
-  let(:common_args) { '--verbose --trace --modulepath spec/fixtures' }
+  let(:common_args) { '--verbose --trace --strict=error --modulepath spec/fixtures' }
 
   before(:each) { skip 'No device --apply in the puppet gems yet' if ENV['PUPPET_GEM_VERSION'] }
 
   describe 'using `puppet resource`' do
     it 'reads resources from the target system' do
       stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider")
-      expect(stdout_str.strip).to match %r{\A(()|(DL is deprecated, please use Fiddle))\Z}
+      expected_values = 'device_provider { \"wibble\": \n  ensure => \'present\',\n  string => \'sample\',\n}'
+      expect(stdout_str.strip).to match %r{\A(DL is deprecated, please use Fiddle\n)?#{expected_values}\Z}
       expect(status).to eq 0
     end
     it 'manages resources on the target system' do
       stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider foo ensure=present")
       expect(stdout_str).to match %r{Notice: /Device_provider\[foo\]/ensure: defined 'ensure' as 'present'}
       expect(status).to eq 0
     end
-  end
 
+    context 'with strict checking at error level' do
+      let(:common_args) { '--verbose --trace --strict=error --modulepath spec/fixtures' }
+
+      it 'deals with canonicalized resources correctly' do
+        stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider wibble ensure=present")
+        stdmatch = 'Error: /Device_provider\[wibble\]: Could not evaluate: device_provider\[wibble\]#get has not provided canonicalized values.\n'\
+                   'Returned values:       \{:name=>"wibble", :ensure=>:present, :string=>"sample"\}\n'\
+                   'Canonicalized values:  \{:name=>"wibble", :ensure=>:present, :string=>"changed"\}'
+        expect(stdout_str).to match %r{#{stdmatch}}
+        expect(status.success?).to be_falsey # rubocop:disable RSpec/PredicateMatcher
+      end
+    end
+
+    context 'with strict checking at warning level' do
+      let(:common_args) { '--verbose --trace --strict=warning --modulepath spec/fixtures' }
+
+      it 'deals with canonicalized resources correctly' do
+        stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider wibble ensure=present")
+        stdmatch = 'Warning: device_provider\[wibble\]#get has not provided canonicalized values.\n'\
+                   'Returned values:       \{:name=>"wibble", :ensure=>:present, :string=>"sample"\}\n'\
+                   'Canonicalized values:  \{:name=>"wibble", :ensure=>:present, :string=>"changed"\}'
+        expect(stdout_str).to match %r{#{stdmatch}}
+        expect(status.success?).to be_truthy # rubocop:disable RSpec/PredicateMatcher
+      end
+    end
+
+    context 'with strict checking turned off' do
+      let(:common_args) { '--verbose --trace --strict=off --modulepath spec/fixtures' }
+
+      it 'deals with canonicalized resources correctly' do
+        stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider wibble ensure=present")
+        stdmatch = 'Notice: /Device_provider\[wibble\]/string: string changed \'sample\' to \'changed\''
+        expect(stdout_str).to match %r{#{stdmatch}}
+        expect(status.success?).to be_truthy # rubocop:disable RSpec/PredicateMatcher
+      end
+    end
+  end
   describe 'using `puppet device`' do
     let(:common_args) { super() + ' --target the_node' }
     let(:device_conf) { Tempfile.new('device.conf') }

spec/fixtures/test_module/.rubocop.yml

@@ -56,10 +56,6 @@ Style/TrailingCommaInArguments:
   Description: Prefer always trailing comma on multiline argument lists. This makes
     diffs, and re-ordering nicer.
   EnforcedStyleForMultiline: comma
-Style/TrailingCommaInLiteral:
-  Description: Prefer always trailing comma on multiline literals. This makes diffs,
-    and re-ordering nicer.
-  EnforcedStyleForMultiline: comma
 Style/SymbolArray:
   Description: Using percent style obscures symbolic intent of array's contents.
   EnforcedStyle: brackets
@@ -105,3 +101,13 @@ Style/IfUnlessModifier:
   Enabled: false
 Style/SymbolProc:
   Enabled: false
+
+# Updated in 0.53 (or thereabouts)
+Style/TrailingCommaInArrayLiteral:
+  Description: Prefer always trailing comma on multiline literals. This makes diffs,
+    and re-ordering nicer.
+  EnforcedStyleForMultiline: comma
+Style/TrailingCommaInHashLiteral:
+  Description: Prefer always trailing comma on multiline literals. This makes diffs,
+    and re-ordering nicer.
+  EnforcedStyleForMultiline: comma

spec/fixtures/test_module/lib/puppet/provider/device_provider/device_provider.rb

@@ -3,8 +3,15 @@
 # A example/test provider for Device support
 class Puppet::Provider::DeviceProvider::DeviceProvider
   def get(_context)
-    []
+    [{:name => 'wibble', :ensure => :present, :string => 'sample'}]
   end
 
   def set(context, changes); end
+
+  def canonicalize(context, resources)
+    if resources[0][:name] == 'wibble'
+      resources[0][:string] = 'changed'
+    end
+    resources
+  end
 end

spec/fixtures/test_module/lib/puppet/type/device_provider.rb

@@ -3,6 +3,7 @@
 Puppet::ResourceApi.register_type(
   name: 'device_provider',
   docs: 'A example/test provider for device support',
+  features: ['canonicalize'],
   attributes: {
     name: {
       type: 'String',

spec/puppet/resource_api_spec.rb

@@ -1,6 +1,23 @@
 require 'spec_helper'
 
 RSpec.describe Puppet::ResourceApi do
+  let(:strict_level) { :error }
+  let(:log_sink) { [] }
+
+  before(:each) do
+    # set default to strictest setting
+    # by default Puppet runs at warning level
+    Puppet.settings[:strict] = strict_level
+    # Enable debug logging
+    Puppet.debug = true
+
+    Puppet::Util::Log.newdestination(Puppet::Test::LogCollector.new(log_sink))
+  end
+
+  after(:each) do
+    Puppet::Util::Log.close_all
+  end
+
   it 'has a version number' do
     expect(Puppet::ResourceApi::VERSION).not_to be nil
   end
@@ -390,8 +407,13 @@
     let(:provider_class) do
       Class.new do
         def canonicalize(_context, resources)
-          resources[0][:test_string] = ['canon', resources[0][:test_string]].compact.join unless resources[0][:test_string] && resources[0][:test_string].start_with?('canon')
-          resources
+          resources.map do |resource|
+            result = resource.dup
+            unless resource[:test_string] && resource[:test_string].start_with?('canon')
+              result[:test_string] = ['canon', resource[:test_string]].compact.join
+            end
+            result
+          end
         end
 
         def get(_context)
@@ -412,6 +434,93 @@ def set(_context, changes)
 
     it { expect { described_class.register_type(definition) }.not_to raise_error }
 
+    describe '#strict_check' do
+      let(:type) { Puppet::Type.type(:canonicalizer) }
+      let(:instance) { type.new(name: 'somename', test_string: 'foo') }
+
+      context 'when current_state is not already canonicalized' do
+        context 'when Puppet strict setting is :off' do
+          let(:strict_level) { :off }
+
+          it { expect(instance.strict_check(nil)).to be_nil }
+
+          it 'will not log a warning message' do
+            expect(Puppet).not_to receive(:warning)
+            instance.strict_check(nil)
+          end
+        end
+
+        context 'when Puppet strict setting is :error' do
+          let(:strict_level) { :error }
+
+          it 'will throw an exception' do
+            expect {
+              instance.strict_check({})
+            }.to raise_error(Puppet::Error, %r{has not provided canonicalized values})
+          end
+        end
+
+        context 'when Puppet strict setting is :warning' do
+          let(:strict_level) { :warning }
+
+          it { expect(instance.strict_check({})).to be_nil }
+
+          it 'will log warning message' do
+            expect(Puppet).to receive(:warning).with(%r{has not provided canonicalized values})
+            instance.strict_check({})
+          end
+        end
+      end
+
+      context 'when current_state is already canonicalized' do
+        context 'when Puppet strict setting is :off' do
+          let(:strict_level) { :off }
+
+          it { expect(instance.strict_check(test_string: 'canon')).to be_nil }
+
+          it 'will not log a warning message' do
+            expect(Puppet).not_to receive(:warning)
+            instance.strict_check(test_string: 'canon')
+          end
+        end
+
+        context 'when Puppet strict setting is :error' do
+          let(:strict_level) { :error }
+
+          it 'will throw an exception' do
+            expect { instance.strict_check(test_string: 'canon') }.not_to raise_error
+          end
+        end
+
+        context 'when Puppet strict setting is :warning' do
+          let(:strict_level) { :warning }
+
+          it { expect(instance.strict_check(test_string: 'canon')).to be_nil }
+
+          it 'will not log a warning message' do
+            expect(Puppet).not_to receive(:warning)
+            instance.strict_check(test_string: 'canon')
+          end
+        end
+      end
+
+      context 'when canonicalize modifies current_state' do
+        let(:strict_level) { :error }
+
+        before(:each) do
+          allow(instance.my_provider).to receive(:canonicalize) do |_context, resources|
+            resources[0][:test_string] = 'canontest'
+            resources
+          end
+        end
+        it 'stills raise an error' do
+          expect {
+            instance.strict_check({})
+          }.to raise_error(Puppet::Error, %r{has not provided canonicalized values})
+        end
+      end
+    end
+
     describe 'the registered type' do
       subject(:type) { Puppet::Type.type(:canonicalizer) }
 
@@ -427,18 +536,15 @@ def set(_context, changes)
       context 'when manually creating an instance' do
         let(:test_string) { 'foo' }
         let(:instance) { type.new(name: 'somename', test_string: test_string) }
-        let(:log_sink) { [] }
 
         it('its provider class') { expect(instance.my_provider).not_to be_nil }
         it('its test_string value is canonicalized') { expect(instance[:test_string]).to eq('canonfoo') }
 
         context 'when flushing' do
           before(:each) do
             Puppet.debug = true
-            log_sink.clear
-            # Redirect log messages here
-            Puppet::Util::Log.newdestination(Puppet::Test::LogCollector.new(log_sink))
             instance.flush
+            puts log_sink(&:message).inspect
           end
 
           after(:each) do
@@ -460,8 +566,13 @@ def set(_context, changes)
                                                                 is: { name: 'somename', test_string: 'canonfoo' },
                                                                 should: { name: 'somename', test_string: 'canonbar' },
                                                               })
-              expect(log_sink[-2].message).to eq('Current State: {:name=>"somename", :test_string=>"canonfoo"}')
-              expect(log_sink.last.message).to eq('Target State: {:name=>"somename", :test_string=>"canonbar"}')
+            end
+
+            it 'logs correctly' do
+              expect(log_sink.map(&:message)).to eq [
+                'Current State: {:name=>"somename", :test_string=>"canonfoo"}',
+                'Target State: {:name=>"somename", :test_string=>"canonbar"}',
+              ]
             end
           end
         end
@@ -475,13 +586,9 @@ def set(_context, changes)
 
       context 'when retrieving an instance through `retrieve`' do
         let(:resource) { instance.retrieve }
-        let(:log_sink) { [] }
 
         before(:each) do
           Puppet.debug = true
-          log_sink.clear
-          # Redirect log messages here
-          Puppet::Util::Log.newdestination(Puppet::Test::LogCollector.new(log_sink))
         end
 
         after(:each) do
@@ -562,16 +669,12 @@ def set(_context, changes)
       context 'when manually creating an instance' do
         let(:test_string) { 'foo' }
         let(:instance) { type.new(name: 'somename', test_string: test_string) }
-        let(:log_sink) { [] }
 
         it('its provider class') { expect(instance.my_provider).not_to be_nil }
 
         context 'when flushing' do
           before(:each) do
             Puppet.debug = true
-            log_sink.clear
-            # Redirect log messages here
-            Puppet::Util::Log.newdestination(Puppet::Test::LogCollector.new(log_sink))
             instance.flush
           end
 
@@ -609,13 +712,9 @@ def set(_context, changes)
 
       context 'when retrieving an instance through `retrieve`' do
         let(:resource) { instance.retrieve }
-        let(:log_sink) { [] }
 
         before(:each) do
           Puppet.debug = true
-          log_sink.clear
-          # Redirect log messages here
-          Puppet::Util::Log.newdestination(Puppet::Test::LogCollector.new(log_sink))
         end
 
         after(:each) do
@@ -630,6 +729,12 @@ def set(_context, changes)
             expect(resource[:test_string]).to eq 'foo'
             expect(log_sink.last.message).to eq('Current State: {:name=>"somename", :test_string=>"foo"}')
           end
+          context 'when strict checking is on' do
+            it('will not throw') {
+              Puppet.settings[:strict] = :error
+              expect { described_class.register_type(definition) }.not_to raise_error
+            }
+          end
         end
 
         describe 'an absent instance' do
@@ -669,6 +774,7 @@ def set(_context, changes)
       stub_const('Puppet::Provider::Remoter', Module.new)
       stub_const('Puppet::Provider::Remoter::Remoter', provider_class)
       allow(provider_class).to receive(:new).and_return(provider)
+      Puppet.settings[:strict] = :warning
     end
 
     it { expect { described_class.register_type(definition) }.not_to raise_error }