Fix boolean parameters

[alexjfisher]

The unsafe rubocop fixes done in
189ac29 broke the boolean parameters.

Before this commit, if you explicitly tried to set either of them to false, they would actually be set to the symbol :false with is not falsey so would act as if you'd set them to true.

[alexjfisher]

Also see comments in #430 (review)

[joshcooper]

The defaultto needs to change to symbol :false too. See 189ac29#diff-44ad177ebcd7857beabf27a6995960e777ebfeac942fb4ec34b1d6720a934e6b

Also the cause of this pain is https://puppet.atlassian.net/browse/PUP-8442

[alexjfisher]

Good spot. It appeared to work as false. Not sure that's because the munging from Puppet::Parameter::Boolean is being applied to the defaultto or if the default (ie as if defaultto isn't specified) is just falsey anyway. Either way, I don't want it to be working by accident, so will update it as suggested.

Meanwhile... do you think there are other custom types where the same rubocop 'fix' might have been repeated?

[joshcooper]

Seems likely this could be an issue in other modules. Hopefully that cop isn't autocorrect-able?

[alexjfisher]

It is if you use --autocorrect-all...

Doing a quick github search, I'm suspicious of puppetlabs/puppetlabs-registry@3f8ae6e (but don't currently work on Windows so can't really do any testing)

Also is https://github.com/puppetlabs/puppetlabs-apt/blob/d172e8588c88c3e2ad85ad4be8ade1c603d23e6d/lib/puppet/type/apt_key.rb#L75 a bug? Does that need to be a symbol?

[joshcooper]

Sorry I've conflated a few different things here.

For parameters

If you do defaultto false, then the value (as seen by the provider) will be nil due to https://puppet.atlassian.net/browse/PUP-1967. This may or may not be a problem, depending on whether the provider treats nil and false the same. For example, the file :backup parameter defaults to false, but the provider only cares if the value is truthy, so it's not an issue
https://github.com/puppetlabs/puppet/blob/e227c27540975c25aa22d533a52424a9d2fc886a/lib/puppet/type/file.rb#L127

However, It would be a problem if the provider did:

if resource[:backup] == false

Second, the recommended way to create a boolean parameter is to do

newparam(:force, :boolean => true, :parent => Puppet::Parameter::Boolean) do

The boolean option is recommended for consistency and adds a predicate method to Puppet::Type::File#force?. See https://github.com/puppetlabs/puppet/blob/e227c27540975c25aa22d533a52424a9d2fc886a/lib/puppet/type/file.rb#L1022

The parent option is recommended so that all parameters handle the same values. By default, this is:

irb(main):014:0> Puppet::Type.type(:file).paramclass(:force).value_collection.values
=> [:true, :false, :yes, :no]

If a parameter does newvalues(:true, :false) then it restricts the values to just those two:

irb(main):002:0> Puppet::Type.type(:user).paramclass(:purge_ssh_keys).value_collection.values
=> [:true, :false]

For Properties

There's a separate but related issue that if you define a boolean property with newvalues(true, false) then puppet will ignore the property if its desired value is false. See https://puppet.atlassian.net/browse/PUP-2368

Conclusion

To preserve the original behavior in java_ks, I would add back newvalues(:true, :false) and defaultto :false

The registry change may be ok, but we'd need to audit the entire provider to see if it handles nil and false the same. I'd recommend reverting the change.

The apt_key default may be ok, as long as the provider handles nil and false the same.

We really need to fix boolean parameter and property handling because this is insane.

[alexjfisher]

I might be inclined to leave newvalues(:true, :false) out and let Puppet::Parameter::Boolean handle what the allowed values are even if that does mean the type technically allows a bit more than it did before (ie :yes and :no). The fewer custom lines of code in each type, the better.

Then maybe as part of the language steering committee consider deprecating yes and no for all types using Puppet::Parameter::Boolean??

and Puppet::Property::Boolean should either be deprecated or fixed (if possible). https://puppet.atlassian.net/browse/PUP-2368 makes it unusable IMO.