Skip to content
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

Properly detect Cumulus Linux platform / version #921

Merged
merged 5 commits into from
Dec 20, 2016
Merged

Properly detect Cumulus Linux platform / version #921

merged 5 commits into from
Dec 20, 2016

Conversation

tas50
Copy link
Contributor

@tas50 tas50 commented Dec 14, 2016

Description

Detect cumulus Linux systems if they have /etc/cumulus directory. Use platform of ‘cumulus’ and platform_family debian since it’s Debian 8.5. Parse version from their config directory.

Issues Resolved

N/A

Check List

@tas50 tas50 force-pushed the cumulus branch 6 times, most recently from 2357617 to 3106626 Compare December 15, 2016 23:30
Detect cumulus Linux systems if they have /etc/cumulus directory. Use platform of ‘cumulus’ and platform_family debian since it’s Debian 8.5. Parse version from their config directory.

Signed-off-by: Tim Smith <tsmith@chef.io>
@@ -187,6 +225,12 @@
expect(@plugin[:platform]).to eq("slackware")
expect(@plugin[:platform_family]).to eq("slackware")
end

it "should set platform_version on slackware" do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was missing so I figured I should add it while I was here

@tas50 tas50 changed the title WIP: Properly detect Cumulus Linux platform / version Properly detect Cumulus Linux platform / version Dec 16, 2016
Signed-off-by: Tim Smith <tsmith@chef.io>
Check the more popular platforms first since those are most likely to match. Parallels have to come before RHEL though since it looks like RHEL.

Signed-off-by: Tim Smith <tsmith@chef.io>
if platform == "cumulus"
if File.exist?("/etc/cumulus/etc.replace/os-release")
release_contents = File.read("/etc/cumulus/etc.replace/os-release")
release_contents.match(/VERSION_ID=(.*)/)[1] rescue nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you want the warning below here as well instead of a nil rescue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. I keep going back and forth with how much Ohai should warn when things don't go right. Recently leaning more to towards Ohai being verbose when things fail so that people see that things are failing and file issues. We have a lot of hidden failures right now, which doesn't help anyone.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully support you doing the right thing, whatever that is.

@tas50
Copy link
Contributor Author

tas50 commented Dec 20, 2016

@btm does that last commit work for you?

Signed-off-by: Tim Smith <tsmith@chef.io>
end

#
# Determines the platform_family based on the platform_family
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"based on the platform"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like my wording better ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Determines the platform_family based on the platform family? :)

Copy link
Contributor

@btm btm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, avoiding blanket rescue's always feels good.

Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50 tas50 merged commit f6daf86 into master Dec 20, 2016
@tas50 tas50 deleted the cumulus branch December 20, 2016 17:23
@thommay thommay added Type: Enhancement Adds new functionality. and removed Enhancement labels Jan 24, 2017
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Enhancement Adds new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants