Install lsb-release on Ubuntu 16/18 docker images

[root-expert]

lsb_release tools is now missing from Ubuntu 16/18 docker images causing acceptance puppet 6 tests to fail.
Facter 3.x utilizes lsb_release tool for obtaining os.distro fact.

Some PRs that are failing due to this:

• Default denykey to undef puppet-zabbix#813
• acceptance tests: os.distro fact requires lsb-release on Debian puppet-cvmfs#148 (workaround on this specific module)
• Release 4.0.0 puppet-nginx#1496

And probably all of the modules that use os.distro fact will fail on Ubuntu 16/18 platforms.

P.S it's the first time touching this utility so any help is appreciated it 😄

[codecov]

Codecov Report

Merging #244 (f1fd541) into master (08f6074) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head f1fd541 differs from pull request most recent head d4ef3d4. Consider uploading reports for the commit d4ef3d4 to get more accurate results

@@            Coverage Diff            @@
##           master    #244      +/-   ##
=========================================
- Coverage    0.52%   0.52%   -0.01%     
=========================================
  Files          14      14              
  Lines        2477    2486       +9     
=========================================
  Hits           13      13              
- Misses       2464    2473       +9     

Impacted Files	Coverage Δ
lib/beaker-hostgenerator/data.rb	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08f6074...d4ef3d4. Read the comment docs.

[ekohl]

I wonder if this really works. It's not a deep merge so this part will overwrite it:

beaker-hostgenerator/lib/beaker-hostgenerator/hypervisor/docker.rb

Lines 40 to 42 in 08f6074

	docker_commands << "apt-get install -y net-tools wget #{extra_packages_to_install.join(' ')}"
	docker_commands << 'locale-gen en_US.UTF-8'
	docker_commands << 'echo LANG=en_US.UTF-8 > /etc/default/locale'

Please change it there.

[root-expert]

Wouldn't the rest docker_image_commands get overridden by that as well? E.g

https://github.com/voxpupuli/beaker-hostgenerator/blob/08f607482950966e80b76eedbe303c7d0351502f/lib/beaker-hostgenerator/data.rb#L660-#L664

[ekohl]

Taking a step back: doing so gives the impression you can always count on these facts. That can lead to subtle breakages. For example, I think in the case of Zabbix the solution is to use safer methods that don't fail as hard (voxpupuli/puppet-zabbix#813 (comment)). Nginx now passes so I no longer see why it failed. It may need a similar treatment.

I think I'm generally more in favor of that than pretending it's always present. For example, now it won't be in the README and users may run into it and find out the hard way.

[root-expert]

IMO these facts on Puppet 7 are supposed to be always present like os.family because their value is based on /etc/os-release unlike with Puppet 6 which is based on lsb_release.

I agree, though, that accessing facts in a more secure way should become a coding guideline here in Voxpupuli. E.g we already have the "no topscope variables for facts" maybe we should enforce fact() or dig, as you said, as well.

I still believe this PR still has its value since multiple modules' PRs are failing due to this right now and migrating away from these facts or accessing them with fact() is time consuming.

[ekohl]

IMHO if modules fail on this, that's legitimate. They should be dealing with it differently.

My reasoning is that you may have the following code:

package { 'lsb-release':
  ensure => installed,
}

Then you would eventually be consistent. However, because the catalog fails to compile when using $facts['os']['distro']['id'], it can never be executed. That means you need to solve this via provisioning or some other mechanism outside of Puppet.

Perhaps we should have a lint rule that says "Don't ever write $facts['os']['distro']['id'] if you want Facter < 4 compatibility"?

[root-expert]

That's a good point actually. I believe that lsb-release should be a recommended package for puppet-agent (as bastelfreak state in IRC) since os.distro facts are not legacy and Facter 3 needs lsb-release to resolve them. But I don't think that's gonna happen.

Linting this is a good idea for Voxpupuli. Puppet Inc. uses these facts on some their modules as well:

• puppetlabs-apt
• puppetlabs-postgresql

So some our workflows will still fail because of external modules.

[baurmatt]

Soooooo, what's the recommended solution for this? :) puppet-prometheus is running into this as well.

For me it sounds totally valid that hostgenerator adds lsb-release for Ubuntu 16.04/18.04 as those Docker images contained it before.

[ekohl]

I'm still 👎 on this. If a module needs lsb-release, it should clearly state this and deal with it. It's not something you can count on. I don't have lsb-release installed on my actual systems (VMs) either.