-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In another module, we use dig()
to check if $facts['os']['distro']
exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But fact() was exactly designed for this purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just saying:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's different because it's Ruby, not Puppet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not according to my local tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we can probably get rid of a lot anyway: #164 is a start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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.