Remove defaults for AIX and Solaris as we can't verify/maintain these #100
Conversation
h-haaks
force-pushed
the
remove-defaults-for-OSes-we-can-not-test
branch
from
aff77d3 to
4dc564c
Compare
| "operatingsystemrelease": [ | ||
| "7.2", | ||
| "7.3" | ||
| ] |
There was a problem hiding this comment.
I added AIX in a previous PR, but dropped Solaris...
There was a problem hiding this comment.
Wait, if we remove the default from params.pp because the same value is already in the main class:
- This is not a backwards-incompatble change;
- There is no reason to drop support of OS.
I do not use these systems nor this module, but if I did I would consider this removal as an intention from the maintainers to not maintain compatibility with these OS anymore. If the OS is said to be supported, any failure would be considered a bug and would possibly be fixed (e.g. by providing fixed default values in Hiera).
There was a problem hiding this comment.
@smortex AIX and Solaris has never been mentioned as supported in any release of this module.
see https://forge.puppetlabs.com/modules/pcfens/ca_cert/
There was a problem hiding this comment.
Changed the PR title a bit.
About backwards-incompatible i added the label in case anyone uses those defaults.
There was a problem hiding this comment.
Added an example for custom certificates handling
| @@ -17,7 +22,7 @@ | |||
| $cert_dir_mode = '2665' | |||
| } | |||
| default: { | |||
| fail("Unsupported operatingsystem (${facts['os']['name']})") | |||
| $cert_dir_mode = '0755' | |||
There was a problem hiding this comment.
I remove all "Unsupported os" fails as that's hard to fit when moving to hiera.
Given that all settings here are now class parameters I really think the module supports any Linux OS out there.
h-haaks
force-pushed
the
remove-defaults-for-OSes-we-can-not-test
branch
4 times, most recently
from
dec185e to
d2617cf
Compare
| # | ||
| # === Parameters | ||
| # class { 'ca_cert': |
There was a problem hiding this comment.
you need another @example something something above each codeblock.
There was a problem hiding this comment.
Hmm, manage_all_user_CAs is not even a param any more ...
I'll fix this :)
manifests/init.pp
Outdated
| String[1] $package_name = $ca_cert::params::package_name, | ||
| String[1] $update_cmd = $ca_cert::params::update_cmd, | ||
| String[1] $trusted_cert_dir = $ca_cert::params::trusted_cert_dir, | ||
| Optional[String[1]] $distrusted_cert_dir = $ca_cert::params::distrusted_cert_dir, |
There was a problem hiding this comment.
I think it makes more sense to just assign undef as default value here:
| Optional[String[1]] $distrusted_cert_dir = $ca_cert::params::distrusted_cert_dir, | |
| Optional[String[1]] $distrusted_cert_dir = undef, |
There was a problem hiding this comment.
If I do that I'll have to add code in the class again to use it.
It will be changed to undef when I move from params class to hiera.
My plan is to create one more PR to align Debian defaults with package defaults before moving to hiera.
manifests/init.pp
Outdated
| String[1] $cert_dir_mode = $ca_cert::params::cert_dir_mode, | ||
| String[1] $ca_file_mode = $ca_cert::params::ca_file_mode, | ||
| String[1] $ca_file_extension = $ca_cert::params::ca_file_extension, | ||
| String[1] $package_ensure = 'installed', |
There was a problem hiding this comment.
I think https://github.com/puppetlabs/puppetlabs-stdlib/blob/main/types/ensure/package.pp would be good here
| String[1] $package_ensure = 'installed', | |
| Stdlib::Ensure::Package $package_ensure = 'installed', |
There was a problem hiding this comment.
I'll fix and add Stdlib::Absolutepath usage as well
manifests/init.pp
Outdated
| String[1] $cert_dir_group = $ca_cert::params::cert_dir_group, | ||
| String[1] $ca_file_group = $ca_cert::params::ca_file_group, | ||
| String[1] $cert_dir_mode = $ca_cert::params::cert_dir_mode, | ||
| String[1] $ca_file_mode = $ca_cert::params::ca_file_mode, |
There was a problem hiding this comment.
https://github.com/puppetlabs/puppetlabs-stdlib/blob/main/types/filemode.pp
| String[1] $ca_file_mode = $ca_cert::params::ca_file_mode, | |
| Stdlib::Filemode $ca_file_mode = '0644', |
h-haaks
changed the title
h-haaks
force-pushed
the
remove-defaults-for-OSes-we-can-not-test
branch
from
d2617cf to
af71aea
Compare
h-haaks
force-pushed
the
remove-defaults-for-OSes-we-can-not-test
branch
from
2558fbc to
2cba40b
Compare
|
Since the |
Since all settings in the params class are now defaults for class parameters I'll remove the defaults for AIX and Solaris as we have no way to verify their correctness.
AIX and Solaris users can still use the module by providing their own parameters.
Builds on #99