Skip to content

Fix to Virtual Machine detection on Darwin#608

Closed
keeleysam wants to merge 2 commits intopuppetlabs:masterfrom
keeleysam:master
Closed

Fix to Virtual Machine detection on Darwin#608
keeleysam wants to merge 2 commits intopuppetlabs:masterfrom
keeleysam:master

Conversation

@keeleysam
Copy link

Using SPDisplaysDataType causes lagging issues on pre-2013 Macs, so use
other sources from system_profiler which do not have the issue and are
also more reliable.

Using SPDisplaysDataType causes lagging issues on pre-2013 Macs, so use
other sources from system_profiler which do not have the issue and are
also more reliable.
@glarizza
Copy link

I'm positive the Facter team will review your pull request, but just a heads up - this looks like it would cause some build failures ( https://travis-ci.org/puppetlabs/facter/jobs/17210641 ) in the spec tests, specifically these tests --> https://github.com/puppetlabs/facter/blob/master/spec/unit/virtual_spec.rb#L50-L74

If you feel comfortable, can you take a look at submitting a commit to update the tests in line with your pull request?

@keeleysam
Copy link
Author

Thanks for the heads-up. I did actually update the tests, just forgot to git add them! Doing that now.

@ahpook
Copy link

ahpook commented Mar 12, 2014

We are in kind of a weird place with regard to facter pulls right now because we're in 2.0 RC and do not have another 1.7.x release planned. That said we'd like to get this in a release, especially now that tests are passing. @adrienthebo can you look at this in the next PR triage and see if it can get into both pe_facter and future oss releases?

@adrienthebo
Copy link

@glarizza I'm not an OSX expert, are you okay with this approach for getting this data? If so we can merge it.

Choose a reason for hiding this comment

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

The previous implementation of this had some limited case insensitivity, does that need to be preserved?

@keeleysam
Copy link
Author

I have check all versions of Fusion, VMware Fusion/ESXi, and Parallels, and it they all show the same, it does not need to be case insensitive.

@adrienthebo
Copy link

This looks good, but before we merge this it would be good to do a little bit of bookkeeping. I can't recall if I already asked for this but it would be good to have a JIRA issue associated with this issue. In addition the commits should be squashed into a single commit, and the commit message should be updated to match the format in https://github.com/puppetlabs/facter/blob/master/CONTRIBUTING.md#making-changes . These changes make it easier to understand what was done here in case we need to review this later. Thanks

@ahpook
Copy link

ahpook commented Mar 13, 2014

@adrienthebo
Copy link

Merged into facter-2 in f221887; this should be released in 2.1.0. Thanks for the contribution and sorry about the delay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants