modulesync 2.12.0 #153
modulesync 2.12.0 #153
Conversation
|
The files |
|
I think in the past we sometimes dropped one OS from the metadata.json and didn't purge all related code. |
manifests/params.pp
Outdated
| # strict variables wasn't added until 3.5.0, so this should be fine. | ||
| if ! $::settings::strict_variables { | ||
| $xfacts = { | ||
| 'lsbdistid' => $::lsbdistid, | ||
| 'lsbdistcodename' => $::lsbdistcodename, | ||
| 'lsbmajdistrelease' => $::lsbmajdistrelease, | ||
| 'lsbdistrelease' => $::lsbdistrelease, | ||
| 'lsbdistid' => $facts['os']['distro']['id'], | ||
| 'lsbdistcodename' => $facts['os']['distro']['codename'], | ||
| 'lsbmajdistrelease' => $facts['os']['distro']['release']['major'], | ||
| 'lsbdistrelease' => $facts['os']['distro']['release']['full'], | ||
| } | ||
| } else { | ||
| # Strict variables facts lookup compatibility | ||
| $xfacts = { | ||
| 'lsbdistid' => defined('$lsbdistid') ? { | ||
| true => $::lsbdistid, | ||
| true => $facts['os']['distro']['id'], | ||
| default => undef, | ||
| }, | ||
| 'lsbdistcodename' => defined('$lsbdistcodename') ? { | ||
| true => $::lsbdistcodename, | ||
| true => $facts['os']['distro']['codename'], | ||
| default => undef, | ||
| }, | ||
| 'lsbmajdistrelease' => defined('$lsbmajdistrelease') ? { | ||
| true => $::lsbmajdistrelease, | ||
| true => $facts['os']['distro']['release']['major'], | ||
| default => undef, | ||
| }, | ||
| 'lsbdistrelease' => defined('$lsbdistrelease') ? { | ||
| true => $::lsbdistrelease, | ||
| true => $facts['os']['distro']['release']['full'], | ||
| default => undef, | ||
| }, | ||
| } |
There was a problem hiding this comment.
This approach doesn't work with structured facts. If lsb-release is not installed, $facts['os']['distro'] is not defined and you'll get an error that you can't call [] on Undef. I'd suggest to use fact() from stdlib which does the right thing. This also removes the need for the special case.
There was a problem hiding this comment.
In another module, we use dig() to check if $facts['os']['distro'] exists.
There was a problem hiding this comment.
But fact() was exactly designed for this purpose.
There was a problem hiding this comment.
I am just saying:
There was a problem hiding this comment.
at https://voxpupuli.org/docs/reviewing_pr/ , we agreed on using fact():
Are facts used? They should only be accessed via
$facts[]orfact()function from stdlib, but not topscope variables
so I would go with fact() in this particular usecase, or uppdate our styleguide.
There was a problem hiding this comment.
That's different because it's Ruby, not Puppet.
There was a problem hiding this comment.
Just to be explicit, I think this whole block can be replaced with:
$xfacts = {
'lsbdistid' => fact('os.distro.id'),
'lsbdistcodename' => fact('os.distro.codename'),
'lsbmajdistrelease' => fact('os.distro.release.major'),
'lsbdistrelease' => fact('os.distro.release.full'),
}There was a problem hiding this comment.
Not according to my local tests.
There was a problem hiding this comment.
And we can probably get rid of a lot anyway: #164 is a start.
There was a problem hiding this comment.
#165 would take it a step further. I'd suggest to look at those 2 PRs and then rebase modulesync based on that. In my experience it's better to first simplify and modernize.
|
@ekohl merged and merged :) |
No description provided.