Skip to content

Conversation

@caseywilliams
Copy link
Contributor

First commit (preparation for second commit)

Adds a vector of OEM strings to the collected data from the DMI source. This information is not available in the /sys/class/dmi/id/ directory, so it can only be collected with root privileges via dmidecode (or the DMI table).

Not all vendors store useful information in the OEM strings, but VirtualBox, for example, reports the hypervisor version and revision there.

Also refactors DMI tests so that a single fixture class can use fixtures for files in /sys/class/dmi/id/ and dmidecode output simultaneously.

Second commit

Introduces two new entities:

  • A metadata object wrapping a map of keys (strings) to values (strings or bools, currently)
  • A result object to replace the boolean return type of detectors. A result reports whether a hypervisor was detected and contains a metadata object with any other hypervisor information gleaned by the detector.

Changes the return type of the main hypervisors() function from a vector of hypervisor name strings to a vector of result objects. Updates the VirtualBox detector to include version and revision metadata.

@caseywilliams caseywilliams requested a review from Magisus July 18, 2017 22:23
@coveralls
Copy link

Coverage Status

Coverage increased (+5.4%) to 94.527% when pulling 2b32db7 on caseywilliams:metadata into cb3d39b on puppetlabs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+5.4%) to 94.527% when pulling b855d32 on caseywilliams:metadata into cb3d39b on puppetlabs:master.

Copy link
Contributor

@Magisus Magisus left a comment

Choose a reason for hiding this comment

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

Over all this looks good. Have you been able to test it working in a real environment? If so, out of curiosity, how have you been setting those up?

LOG_DEBUG("dmidecode executable not found");
} else {
int dmi_type {-1};
int dmi_type{-1};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure curly brace initialization usually has the space that was deleted here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm noticing those spaces are missing throughout. Is that intentional? (I'm happy to be corrected)

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 just wasn't really aware of what the standard way to do it was - I will adjust to include the space!

case 0: { // BIOS information
if (index == 0) {
member = &data_->bios_vendor;
data_->bios_vendor = move(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the way you've refactored this switch :)

@caseywilliams
Copy link
Contributor Author

@Magisus So far I've just been testing on local Linux and Windows VMs inside of VirtualBox or VMware - those cases look fine (usually I just write the tests first without fixtures, run them on the system I'm testing and then modify them once it seems to work). That process will stop being very useful pretty soon - it'd be great to talk with the team at some point about how to do it better.

@Magisus
Copy link
Contributor

Magisus commented Jul 19, 2017

Awesome, thanks. And yeah we might have to get creative in some cases. I kind of like the idea though, will give us more insight into the kinds of deployments customers actually use.

Adds a vector of OEM strings to the collected data from the DMI source.
This information is not available in the /sys/class/dmi/id/ directory,
so it can only be collected with root privileges via dmidecode (or the
DMI table).

Not all vendors store useful information in the OEM strings, but
VirtualBox, for example, reports the hypervisor version and revision
there.

Also refactors DMI tests so that a single fixture class can use fixtures
for files in /sys/class/dmi/id/ and dmidecode output simultaneously.
Introduces two new entities:
- A metadata object wrapping a map of keys (strings) to values (strings
  or bools, currently)
- A result object to replace the boolean return type of detectors.
  A result reports whether a hypervisor was detected and contains a
  metadata object with any other hypervisor information gleaned by the
  detector.

Changes the return type of the main `hypervisors()` function from a
vector of hyperivsor name strings to a vector of result objects. Updates
the VirtualBox detector to include version and revision metadata.
@coveralls
Copy link

coveralls commented Jul 19, 2017

Coverage Status

Coverage increased (+5.4%) to 94.527% when pulling a474009 on caseywilliams:metadata into cb3d39b on puppetlabs:master.

@Magisus Magisus merged commit b2c9319 into puppetlabs:master Jul 19, 2017
@caseywilliams caseywilliams deleted the metadata branch November 7, 2017 12:30
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.

3 participants