Value type appears to be incorrect

[itgrl]

String $patch_cron_user = $patch_data_owner,
Boolean $install_delta_rpm = false,
Optional[Boolean] $reboot_override = undef,

if ( $::osfamily == 'RedHat' ) {
package { 'deltarpm':
ensure => $install_delta_rpm,
}
}

If using Puppet's package resource type, then the parameter type $install_delta_rpm should be type String to match the values used.

Allowed values:

present
absent
purged
held
installed
latest
/./

[albatrossflavour]

Hey, thanks for raising the ticket.

I can completely see what you're saying about matching the values of the package resource but I'm not 100% sure it's the right way to handle it.

That being said, i'm also not convinced that having delta rpm in there in the first place is the right thing!

If we have just a true/false, you can prevent the module from managing the package. This can be used if you want to do advanced management of it (pinning to a specific version, using latest etc).

I don't know that I'm right (or if there is a right in a scenario like this) but I'd like to hear your thoughts on it.

I want to give the option to manage yum-plugin-security too - which is a more useful addition given the scope of the module - so would like to decide this first.

[itgrl]

Well if you wish to use a boolean, then the code would have to be restructured to something more like.

if $install_delta_rpm {
if ( $::osfamily == 'RedHat' ) {
package { 'deltarpm':
ensure => 'installed',
}
}
}

As it currently stands:

ensure => $install_delta_rpm,

This line would have to comply with the Package resource methods and not a boolean since the package resource expects a string with the values mentioned before.

We could even take this a step further and leverage packages to install per OS with lookups of the packages either with a hiera-like data file, params.pp or a data lookup function specific to the module.

Then it the boolean would work as it would lookup the packages to manage and manage them per OS.

[rnelson0]

An Enum would probably be best, using the valid values here. Or maybe adding a new type to Stdlib, like this, and relying on at least that version of stdlib.

[albatrossflavour]

If we use an enum with the standard entries for the package, we don’t have the ability to tell the module that it isn’t responsible for managing the package. We could do an enum with the standard entries and add in `false` to prevent management. What do you think?

…

On 18 Sep 2018, at 22:48, Rob Nelson wrote: An Enum would probably be best, using the valid values [here](https://puppet.com/docs/puppet/5.5/type.html#package-attribute-ensure). Or maybe adding a new type to Stdlib, like [this](https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/types/ensure/service.pp), and relying on at least that version of stdlib. -- You are receiving this because you were assigned. Reply to this email directly or view it on GitHub: #48 (comment)

[rnelson0]

Hrm. Here's where we are:

Boolean $install_delta_rpm = false,

if ( $::osfamily == 'RedHat' ) {
  package { 'deltarpm':
    ensure => $install_delta_rpm,
  }
}

If we change that to be an enum with 'false' then it would have to accept that as a string (Enum doesn't accept true and false, just the strings 'true' and 'false'). What if we coupled a feature flag with a package state, like this?

Boolean $manage_delta_rpm = false,
Enum['present', 'latest', 'absent', 'latest', ...] $delta_rpm_ensure = 'present',

# Assuming there are other items for the OS Family, this is two checks, otherwise combine to a single one
if ( $::osfamily == 'RedHat' ) {
  if $manage_delta_rpm {
    package { 'deltarpm':
      ensure => $delta_rpm_ensure,
    }
  }
}

For most users, they could just flip the feature flag to true and probably not care beyond that, especially if they use immutable nodes. For anyone specific needs, they flip that to true and specify something other than present.

# most users
os_patching::manage_delta_rpm: true

# specific needs
os_patching::manage_delta_rpm: true
os_patching::delta_rpm_ensure: 'latest'

(I did this by hand and made a few edits, typos/syntax errors are mine)

[itgrl]

The above proposal would certainly work.

[albatrossflavour]

“If we change that to be an enum with 'false' then it would have to accept that as a string (Enum doesn't accept true and false, just the strings 'true' and 'false'). What if we coupled a feature flag with a package state, like this?” I think that’s the better option. You can’t do boolean within an enum but you can wrap it in a variant and do both enum and boolean - but that’s a bit messy. I’ll go that way and will get the code updated in the next day or two. Thanks

…

On 19 Sep 2018, at 23:03, Rob Nelson wrote: Hrm. Here's where we are: ``` Boolean $install_delta_rpm = false, if ( $::osfamily == 'RedHat' ) { package { 'deltarpm': ensure => $install_delta_rpm, } } ``` If we change that to be an enum with 'false' then it would have to accept that as a string (Enum doesn't accept `true` and `false`, just the strings `'true'` and `'false'`). What if we coupled a feature flag with a package state, like this? ``` Boolean $manage_delta_rpm = false, Enum['present', 'latest', 'absent', 'latest', ...] $delta_rpm_ensure = 'present', # Assuming there are other items for the OS Family, this is two checks, otherwise combine to a single one if ( $::osfamily == 'RedHat' ) { if $manage_delta_rpm { package { 'deltarpm': ensure => $delta_rpm_ensure, } } } ``` For most users, they could just flip the feature flag to true and probably not care beyond that, especially if they use immutable nodes. For anyone specific needs, they flip that to true and specify something other than `present`. ``` # most users os_patching::manage_delta_rpm: true # specific needs os_patching::manage_delta_rpm: true os_patching::delta_rpm_ensure: 'latest' ``` (I did this by hand and made a few edits, typos/syntax errors are mine) -- You are receiving this because you were assigned. Reply to this email directly or view it on GitHub: #48 (comment)

[albatrossflavour]

https://github.com/albatrossflavour/puppet_os_patching/tree/feature/rpm_attribute_fix

It now manages optionally also manages yum_plugin_security and uses the two step setting. I've used an enum so we can't set it to a specific version (enum won't take regex). I'll update the doco to state if you want to pin to a specific release you should manage it outside of the module.

What do you think?

[albatrossflavour]

Resolved by this pull request