Deprecation Request

[TraGicCode]

I feel like we should deprecate this repo. in favor of using the puppet shipped

https://github.com/puppetlabs/puppet/tree/master/lib/puppet/parameter/boolean.rb and
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/property/boolean.rb

Any thoughts?

/cc @voxpupuli/collaborators

[bastelfreak]

I'm not sure how this boolean magic works, but if those files can replace this repo, than +1 for deprecation

[ekohl]

I recall that @alexjfisher discovered those built in types are unusable due to bugs but it might be good to ensure the built in types do work and we can deprecate this repo.

[TraGicCode]

At first glance and when accompanied with basic tests of a custom type i'm working on it seems to be working as i would expect.

require 'puppet/parameter/boolean'

Puppet::Type.newtype(:wsusserver_approvalrule) do
    
   newproperty(:enabled, :parent => Puppet::Property::Boolean) do
        desc 'Specifies whether the rule is enabled or disabled.'
    end

end

Matcher to check for boolean type

RSpec::Matchers.define :be_boolean do
    match do |value|
      [true, false].include? value
    end
  end
  

Test verifies things get munged to booleans

    [true, false, 'yes', 'no'].each do |value|
        it "should accept #{value} as a value for :enabled" do
            expect(type_class.new(name: 'test', ensure: :present, enabled: value))
        end

        it "should be munge #{value} to true or false" do
            expect(type_class.new(name: 'test', ensure: :present, enabled: value)[:enabled]).to be_boolean
        end
    end

    it "should convert yes to true" do
        expect(type_class.new(name: 'test', ensure: :present, enabled: 'yes')[:enabled]).to be true
    end

    it "should convert no to false" do
        expect(type_class.new(name: 'test', ensure: :present, enabled: 'no')[:enabled]).to be false
    end

[ekohl]

At first glance it appears to work but theforeman/puppet-pulp#284 showed there were edge cases where it didn't. Maybe I shouldn't have included defaults when I refactored and I haven't had time to properly review it.

[alexjfisher]

@TraGicCode Try updating a boolean property from true to false...

[alexjfisher]

https://tickets.puppetlabs.com/browse/PUP-2368

[alexjfisher]

Having said that, I can't see why this module is any better. However you munge to false, if param.should will always be false and it's still not possible to sync the property from true to false.
https://github.com/puppetlabs/puppet/blob/77017e41a1fbea88abc0d59b29d24fc8a292299c/lib/puppet/transaction/resource_harness.rb#L123

The fix is probably

unless param.nil? && !param.safe_insync?(current_value)

[TraGicCode]

Thanks for the input @alexjfisher . Strange this has been around so long. Reminds me of the isrequired method still left behind on the type on the Puppet::Type class

[wyardley]

I've mentioned this elsewhere, I think, but I find it kind of odd that the desired behavior for booleans with types / providers is to allow true, false, "true", "false", "yes", etc., when the Puppet language types only allow true / false. Am I the only one who finds this a little inconsistent?

Assuming you just want to allow those, wouldn't something like this work?
https://github.com/voxpupuli/puppet-rabbitmq/blob/master/lib/puppet/type/rabbitmq_erlang_cookie.rb#L24-L27

[waveclaw]

I have to use this or Adrien Thebo's version because I'm munging 1's and 0's to true and false on some of the parameters for the subscription-manager module.

It is really unpleasant but a user visible issue with the underlying system Puppet is abstracting. As a bonus, outputting the wrong entry in a property breaks the commands underlying the provider. Some can silently take the string true, others must be 1 or 0.

Likewise, letting the property take on 1, 0, true, false or string equivalents means I'm letting a user direct mapping the expected resource appearance into Puppet. No need to explain that what is a true (takes 1 or 0) property or a regular boolean (takes true or false). The resource statement can be written to look like the actual on disk config. And I munge the values back to true or false correctly.

[alexjfisher]

I don't see how this module provides any benefit over what's now been in core puppet for years. https://github.com/puppetlabs/puppet/blob/e257636f1f3a7024593e33571d7eb5e2686744fc/lib/puppet/coercion.rb

However, I still think both implementations are broken for properties.

FWIW, subscription-manager doesn't use this anymore. waveclaw/puppet-subscription_manager@2c71a37#diff-490694a9db8f7a371538da1abe484314

[waveclaw]

I dropped boolean support completely in favor of treating the values as enumerations with exclusive dual values. The core feature is unusable and puppet-boolean has challenges in Puppet 6.

My decision for supporting my modules should not affect your decisions on support of your code. I picked a pragmatic solution that supported external and internal customers of my code. Treating boolean logic as a special cased enumeration is not ideal but functional.

[bastelfreak]

I will proceed and archive the module.