From 6f5e090a0173f7fcf382a1ab742ce646ca1ba0ad Mon Sep 17 00:00:00 2001 From: Michael T Lombardi Date: Fri, 22 Feb 2019 17:40:19 -0600 Subject: [PATCH] (MODULES-8047) Fix puppet resource chocolateyconfig Prior to this commit running `puppet resource chocolateyconfig` would fail during the validation block which verified that no config items were both :present *and* had an empty :value or were missing :value altogether. This is because Chocolatey represents a configuration item which is using the defaults as an empty string. When collecting instances of Chocolatey config items from the configuration file, it would find the items, mark them as :present, and save them with a :value of "". This would then fail the validation block. During a `puppet apply` run the validation block is called before the instances are fetched - and, therefore, before `@property_hash` is populated. This allows us to distinguish between the two calls and error on any declared resources which specify `:ensure` as `:present` but forget to specify a `:value`. This commit moves the validation logic from the type into the provider in order to allow us to take advantage of the @property_hash information during a `puppet resource` run. This commit also updates the `get_config` method to treat any discovered Chocolatey config item with an empty value as `:absent` - this matches the behavior of `:ensure => :absent` which calls `choco config unset` - this command replaces the existing value of a config item with an empty string. Now when returning the list of Chocolatey configs our model of them is consistent, with `:absent` meaning that the configuration item is set to the default. This commit does not change the behavior of a `puppet apply` run. This commit *does* include changes to the unit tests to reflect the moving of the validation logic. --- .../provider/chocolateyconfig/windows.rb | 15 +++++++++--- lib/puppet/type/chocolateyconfig.rb | 4 ---- .../provider/chocolateyconfig/windows_spec.rb | 24 +++++++++++++++++++ .../unit/puppet/type/chocolateyconfig_spec.rb | 23 ------------------ 4 files changed, 36 insertions(+), 30 deletions(-) diff --git a/lib/puppet/provider/chocolateyconfig/windows.rb b/lib/puppet/provider/chocolateyconfig/windows.rb index e512f360..57217d51 100644 --- a/lib/puppet/provider/chocolateyconfig/windows.rb +++ b/lib/puppet/provider/chocolateyconfig/windows.rb @@ -54,9 +54,9 @@ def self.get_config(element) config[:name] = element.attributes['key'] if element.attributes['key'] config[:value] = element.attributes['value'] if element.attributes['value'] config[:description] = element.attributes['description'] if element.attributes['description'] - - config[:ensure] = :present - + # If the value is empty it is the default value and so is not set by Puppet. + # If a config item is ensured as absent it sets the value to an empty string. + config[:ensure] = element.attributes['value'].to_s.empty? ? :absent : :present Puppet.debug("Loaded config '#{config.inspect}'.") config @@ -99,6 +99,15 @@ def destroy end def validate + # We want to ensure that specifying a config item as :present fails if no :value for the config + # is specified. However, during puppet resource runs the resource has an :ensure of present. + # We are able to overcome this by checking if the hash is empty: + # The hash *is* empty when the validate block is called during a puppet apply run. + # The hash is *not* empty when the validate block is called during a puppet resource run. + # If the hash is empty, fail only if :ensure is true and :value is not specified or is an empty string. + if @property_hash.empty? && resource[:ensure] == :present && resource[:value].to_s.empty? + raise ArgumentError, "Unless ensure => absent, value is required." + end choco_version = Gem::Version.new(PuppetX::Chocolatey::ChocolateyCommon.choco_version) if PuppetX::Chocolatey::ChocolateyCommon.file_exists?(PuppetX::Chocolatey::ChocolateyCommon.chocolatey_command) && choco_version < Gem::Version.new(CONFIG_MINIMUM_SUPPORTED_CHOCO_VERSION) raise Puppet::ResourceError, "Chocolatey version must be '#{CONFIG_MINIMUM_SUPPORTED_CHOCO_VERSION}' to manage configuration values. Detected '#{choco_version}' as your version. Please upgrade Chocolatey." diff --git a/lib/puppet/type/chocolateyconfig.rb b/lib/puppet/type/chocolateyconfig.rb index ba4d4a65..35fa4f8f 100644 --- a/lib/puppet/type/chocolateyconfig.rb +++ b/lib/puppet/type/chocolateyconfig.rb @@ -70,10 +70,6 @@ def insync?(is) end validate do - if self[:ensure] != :absent - raise ArgumentError, "Unless ensure => absent, value is required." if self[:value].nil? || self[:value].empty? - end - if provider.respond_to?(:validate) provider.validate end diff --git a/spec/unit/puppet/provider/chocolateyconfig/windows_spec.rb b/spec/unit/puppet/provider/chocolateyconfig/windows_spec.rb index 2cdbd599..74dabc00 100644 --- a/spec/unit/puppet/provider/chocolateyconfig/windows_spec.rb +++ b/spec/unit/puppet/provider/chocolateyconfig/windows_spec.rb @@ -211,6 +211,30 @@ resource.provider.validate }.to raise_error(Puppet::ResourceError, /Chocolatey version must be '0.9.10.0' to manage configuration values. Detected '#{last_unsupported_version}'/) end + + it "should error when the property_hash is empty, :ensure is :present, and no :value is supplied" do + resource.delete(:value) + + expect { + resource.provider.validate + }.to raise_error(ArgumentError, "Unless ensure => absent, value is required.") + end + + it "should not error when the property_hash is defined, even if :ensure is :present and no :value is supplied" do + resource.delete(:value) + resource.provider.instance_variable_set("@property_hash", {:value => "something"}) + resource.provider.validate + end + + it "should not error when the property_hash is empty, :ensure is :present and a :value is supplied" do + resource.provider.validate + end + + it "should not error when the property_hash is empty, :ensure is :absent and no :value is supplied" do + resource[:ensure] = :absent + resource.delete(:value) + resource.provider.validate + end end context ".flush" do diff --git a/spec/unit/puppet/type/chocolateyconfig_spec.rb b/spec/unit/puppet/type/chocolateyconfig_spec.rb index 241d891c..db5c24ea 100644 --- a/spec/unit/puppet/type/chocolateyconfig_spec.rb +++ b/spec/unit/puppet/type/chocolateyconfig_spec.rb @@ -77,27 +77,4 @@ reqs[0].target.must == resource end - context ".validate" do - it "should pass when ensure => absent with no value" do - resource[:ensure] = :absent - - resource.validate - end - - it "should pass when ensure => present with a value" do - resource[:ensure] = :present - resource[:value] = 'yo' - - resource.validate - end - - it "should fail when ensure => present with no value" do - resource[:ensure] = :present - - expect { - resource.validate - }.to raise_error(ArgumentError, /Unless ensure => absent, value is required/) - end - end - end