ghoneycutt / puppet-module-pam

Puppet module to manage PAM
Other
18 stars 79 forks source link

Drop puppet 3 support and use data types for parameters #167

Closed treydock closed 6 years ago

treydock commented 6 years ago

This PR is incomplete, I have not updated the main pam class. I wanted to know, should things that currently accept string or boolean and use str2bool be continued or should the data type just be Boolean? These changes likely require a major version bump so figured maybe good time to force parameters to be the proper data type.

treydock commented 6 years ago

@ghoneycutt Just FYI, relates to PR discussion for MOTD module, the message for Optional[Array] changes in puppet >= 4.10.x in such a way that the only way to match the error is just to say /Array/.

Puppet 4.9.x: expects an Array value, got String Puppet 4.10.x/5.x: expects a value of type Undef or Array, got String

ghoneycutt commented 6 years ago

Thanks for all the hard work!

I think it's time we drop string 'true', 'false' and just use Boolean.

ghoneycutt commented 6 years ago

Happy to go to a new major version. Are there other things we should change about the module?

@Phil-Friderici what do you think?

treydock commented 6 years ago

@ghoneycutt Part of major version, maybe move various defaults to module level Hiera? The pam class code is extremely long and mostly just default values. This may be unrealistic given some defaults are based on $ensure_vas and $vas_major_version.

Maybe saving some lines by using pick instead of 5 line conditions for picking which value to use for Optional parameters.

ghoneycutt commented 6 years ago

I'd like to drop support for VAS in the next major version and instead just document what values should be used. We could setup a vas/ directory with the hiera structure in it so that you could copy those to your hiera and have VAS working. <-- @Phil-Friderici

Would definitely like to switch to data in modules, which we just did for sgnl05/sssd. This will also cut way down on lines of code.

https://github.com/sgnl05/sgnl05-sssd

treydock commented 6 years ago

@ghoneycutt I'd be happy to help with such changes via different pull requests. May be a good time to use something like https://skywinder.github.io/github-changelog-generator/ to generate CHANGELOG, something I've seen used in voxpupuli projects.

treydock commented 6 years ago

@ghoneycutt There are several deprecated parameters in init.pp. Okay to bundle a commit removing those into this pull request?

ghoneycutt commented 6 years ago

+1 to the changelog generator. I'm doing that on other projects.

ghoneycutt commented 6 years ago

I made a branch named puppet5. Could you point this against that branch? That way we can all work on the breaking changes and get them merged as they come in and time when we merge the whole thing into master.

I wont make any changes to master in the meantime, so we aren't stuck in rebase hell.

ghoneycutt commented 6 years ago

Go ahead and remove the deprecated params in their own commit.

treydock commented 6 years ago

@ghoneycutt Added commit for removing deprecated parameters, let me know if anything in this PR should be split into separate PR.

ghoneycutt commented 6 years ago

I dont think we actually need yard and redcarpet as puppet-strings will pull in its dependencies

Phil-Friderici commented 6 years ago

Happy to go to a new major version. Are there other things we should change about the module? @Phil-Friderici what do you think?

Changing from 'true' and 'false' to booleans is great. Chaning from 'unset' to undef would be great too. (used for $pam_d_login_oracle_options)