Use fact() function for all os.distro.* facts #1017
Conversation
* On Puppet 6 facter 3.x requires lsb-release to resolve os.distro.* facts. Using $facts hash cause errors like "Evaluation Error: Operator '[]' is not applicable to an Undef Value." because os.distro is undefined causing the catalog to fail. Use fact() to identify Undef facts and throw an error to the user. Signed-off-by: Christos Papageorgiou <christos.papageorgioy@gmail.com>
|
|
There was a problem hiding this comment.
Hum… 🤔 This is a good start but not enough I think.
This is replacing a hard failure with a cryptic error message by no failure (at the puppet level) but a broken configuration (at the apt level) when running on a system without the 'apt-transport-https' package but 'https://' repositories (and all sources will lack a release which is probably not good BTW).
We can't add a dependency on lsb-release at this level since it is needed before the catalog is computed (i.e. already too late).
My guess is that we should rather fail hard with a clear error message when we detect that fact('os.distro.codename') is undef with a message like Cannot determine the distribution codename. Older versions of Facter rely on lsb-release to fetch it, is it installed?. Maybe in the apt class if all classes include it.
What do you think?
|
I don't think it's a cryptic error, it clearly states that a specific parameter must be declared in order for it to work 🤔 It fails to auto-detect the distro so the user must explicitly set it (or install lsb-release) |
|
What I call a cryptic error message is the following when adding a random resource: While the root problem is that a package is missing. As it is right now, the PR fix the catalog but I can imagine cases where this will produce broken apt config. I did a test for the above module but it seems to be okay (voxpupuli/puppet-elasticsearch#1149) so maybe I am paranoid? 🤷 😄 |
|
@smortex in that elasticsearch test run, why is it complaining about source.pp line 102? That's just a variable assignment. 🤔 |
Line 102 in the latest release, but line 103 on if ($facts['os']['distro']['codename'] in $_transport_https_releases) and $_location =~ /(?i:^https:\/\/)/ {Without |
Ah I see you point. Yea that was the only part which the module doesn't fail. Failing directly on the |
| if $facts['os']['distro']['codename'] { | ||
| $_release = $facts['os']['distro']['codename'] | ||
| if fact('os.distro.codename') { | ||
| $_release = fact('os.distro.codename') |
There was a problem hiding this comment.
Very strictly speaking, this line doesn't need to be modified since at this point you know it's available. But it works too.
Signed-off-by: Christos Papageorgiou <christos.papageorgioy@gmail.com>
|
A few times I've forgotten to merge after approving but having to wait for the tests. If I do, please ping me. |
lsb-releaseto resolve os.distro.* facts which may not always be available. Using $facts hash cause errors like "Evaluation Error: Operator '[]' is not applicable to an Undef Value." because os.distro is undefined causing the catalog to fail. Use fact() to identify Undef facts and throw an error to the user.Releated: