(#21260) Add Cumulus Linux detection support by dorkmatt · Pull Request #458 · puppetlabs/facter

dorkmatt · 2013-06-14T23:29:14Z

No description provided.

puppetcla · 2013-06-14T23:30:17Z

CLA Signed by dorkmatt on 2012-10-02 21:00:00 -0700

…s matched

adrienthebo · 2013-06-20T17:02:04Z

lib/facter/operatingsystem.rb

Is /etc/os-release going to be unique for Cumulus Linux? Do we have to do a regex to extract this value or can we rely on the presence of the file?

The file is /etc/os-release is supported in a number a OS's these days - Gentoo, Ubuntu and of course Cumulus Linux. The regex value is to extract a certain data point from this file.

Well that's pretty nifty. However if this is present on a number of platforms it could change existing behavior. Line 39 may mitigate this, but this might be a bit aggressive.

Perhaps this could be a fallback behavior instead of the first option selected?

The problem here is the existing logic, Cumulus Linux ships with a /etc/debian_version to show our OS heritage (such as Ubuntu does too) - even though both Cumulus & Ubuntu are clearly not Debian. I'm happy to change that logic too, but wanted to do the least amount of impact on other OS selections - thoughts here?

I was afraid of that. I want to ensure that our primary supported platforms (Ubuntu, Debian, CentOS, Redhat) won't regress with this change, and if not then this is great. I don't want to make this PR diverge too much from the goal of getting in Cumulus Linux support, but I can't offer a lot of great solutions aside from manual verification.

I just ran into a snag with this, because of how Debian formats /etc/os-release

PRETTY_NAME="Debian GNU/Linux 7.0 (wheezy)" NAME="Debian GNU/Linux" VERSION_ID="7.0" VERSION="7.0 (wheezy)" ID=debian ANSI_COLOR="1;31" HOME_URL="http://www.debian.org/" SUPPORT_URL="http://www.debian.org/support/" BUG_REPORT_URL="http://bugs.debian.org/"

We can't normalize the value of NAME with a regex so this can't be a general solution. I think it could be a fallback mechanism but I really don't want to change the operatingsystem fact on debian to debiangnulinux

adrienthebo · 2013-06-20T17:08:47Z

Thank you very much for this contribution!

It would be good to have test coverage for these changes, to ensure that these changes don't accidentally regress. Would you be able to get sample output from /etc/os-release so we can work out how to write tests? https://github.com/puppetlabs/puppet/blob/master/README_DEVELOPER.md#a-brief-introduction-to-testing-in-puppet provides an introduction to testing in the Puppet ecosystem, if you're not familiar with the tests used in Puppet and Facter.

adrienthebo · 2013-06-27T22:24:32Z

I noticed this hasn't seen any activity in about a week. Is there a chance you'll be able to work on this in the coming week? No pressure, just checking in.

Cumulus Linux RSpec additions

dorkmatt · 2013-06-28T00:50:33Z

We've added an initial round of rspec unit tests, as requested here's some examples of /etc/os-release

NAME="Cumulus Linux"
VERSION_ID=1.5.0
VERSION="1.5.0-83579fb-MODIFIED-201306261532"
PRETTY_NAME="Cumulus Linux"
ID=cumulus-linux
ID_LIKE=debian
CPE_NAME=cpe:/o:cumulusnetworks:cumulus_linux:1.5.0-83579fb-MODIFIED-201306261532
HOME_URL="http://www.cumulusnetworks.com/"

NAME="Ubuntu"
VERSION="12.04.2 LTS, Precise Pangolin"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu precise (12.04.2 LTS)"
VERSION_ID="12.04"

adrienthebo · 2013-06-28T17:12:21Z

lib/facter/operatingsystem.rb

Two comments on this:

Using global match variables like $1 isn't threadsafe; it's better to use an explicit MatchData variable:

matchdata = File.read('/etc/os-release').match /^NAME=["']?(.+?)["']?$/ matchdata[1]

In addition, this regular expression is pretty aggressive. Do we really want to strip out numbers or underscores?

As mentioned above if this is a fallback option that would be fine, but I would like to avoid defaulting to the more intrusive option.

The agressive regex was to follow other setcode's for the operatingsystem fact - all of which seem to have mixed upper/lower case and no spaces.

Fair point. We don't have a formal convention around the output of operatingsystem fact names. On one hand I am nervous about this breaking things, but on the other hand it would be nice to actually have that convention.

adrienthebo · 2013-07-10T18:27:14Z

Does Cumulus Linux have a file like /etc/cumulus-release that we can use to identify this as definitely Cumulus Linux? I still have concerns about the behavior of checking /etc/os-release on every platform.

dorkmatt · 2013-07-10T19:26:17Z

Not currently, our entire point was to not invent yet-another-standard.
Would moving the os-release match statement towards the match statement
make you feel more comfortable?

adrienthebo · 2013-07-15T16:44:24Z

👍 for not inventing a new standard.

So here's what I think we need to do to handle this:

Check /etc/os-release. If it's cumulus, use that value. If not, skip.
Check all existing resolutions.
If everything fails, fall back to /etc/os-release.

We can't use /etc/os-release in preference to our existing resolutions right now until we can confirm that this won't cause a regression across any of our PE supported platforms. However we can special case for Cumulus, and then use os-release as a fallback. What do you think of this approach?

puppetcla · 2013-07-15T19:20:24Z

CLA signed by all contributors.

adrienthebo · 2013-07-22T20:04:50Z

@dorkmatt do you want to close this in favor of #493?

adrienthebo · 2013-08-01T00:02:23Z

I'm closing this in favor of GH-493; if this is in error let me know.

* (FACT-2562) Remove reset from to_hash. Save external and custom facts in options. * (FACT-2562) Read custom and external facts from options instead of LegacyFacter. * (FACT-2562) Fix tests. * (FACT-2562) Update tests description.

(#21260) Add Cumulus Linux OS detection support

13d16b7

Matt Peterson added 3 commits June 14, 2013 18:11

collapsed in a parsable symbol

5548df7

Per pull request #453, don't match an array when a single OS string i…

a55f4c4

…s matched

liberal regex

6628b07

adrienthebo reviewed Jun 20, 2013
View reviewed changes

Spec for Cumulus Linux

c207878

Matt Peterson and others added 2 commits June 27, 2013 17:38

Per Adrien feedback, an explicit check and return value

e54fb62

Merge pull request #1 from sergeyhush/master

7efa491

Cumulus Linux RSpec additions

dorkmatt closed this Jun 28, 2013

dorkmatt reopened this Jun 28, 2013

adrienthebo reviewed Jun 28, 2013
View reviewed changes

preserve agressive regex to match other setcodes, also explicit variable

330166b

sergeyhush mentioned this pull request Jul 16, 2013

Cumulus Linux os resolution #493

Merged

Sergey Sudakovich added 2 commits July 29, 2013 13:07

pulled in os resolution changes from puppet

7099422

(#21260) Add Cumulus Linux OS detection support

9944632

adrienthebo closed this Aug 1, 2013

Conversation

dorkmatt commented Jun 14, 2013

Uh oh!

puppetcla commented Jun 14, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrienthebo commented Jun 20, 2013

Uh oh!

adrienthebo commented Jun 27, 2013

Uh oh!

dorkmatt commented Jun 28, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrienthebo commented Jul 10, 2013

Uh oh!

dorkmatt commented Jul 10, 2013

Uh oh!

adrienthebo commented Jul 15, 2013

Uh oh!

puppetcla commented Jul 15, 2013

Uh oh!

adrienthebo commented Jul 22, 2013

Uh oh!

adrienthebo commented Aug 1, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants