Fixes Razor issue 297; MK now uses 'lshw -disable dmi' under kvm #56

Merged
merged 1 commit into from Jan 25, 2013

Conversation

Projects
None yet
2 participants
@tjmcs

tjmcs commented Jan 23, 2013

Issue 297 on the Razor issues list was submitted by a KVM use who found that the 'lshw' command that is used by the Razor Microkernel to gather additional hardware information was hanging for them in a KVM environment unless they included the '-disable dmi' flag. The changes in this pull request should fix that issue without having to resort to changes to Facter (which is how the user who submitted Issue #297 appears to have "fixed" the problem). These changes have been tested in a KVM environment running under VMware Fusion; tests on real hardware have not been run (since I'm on an OS X machine, such tests are not possible, locally)

@slippycheeze

This comment has been minimized.

Show comment Hide comment
@slippycheeze

slippycheeze Jan 24, 2013

Contributor

Hey. The code change itself looks reasonable enough, so 👍 to that.

The commit message is pretty solid too - it includes the essential detail of why this solution was chosen, over the million other possibilities. The one shortfall is the first line: please make it a summary suitable for use in a changelog or release announcement. It should summarise the user-visible effect of the change. (eg: "Disable lshw DMI parsing on KVM virtual machines")

In general our release announcements - and a lot of history review - is done with the information you get from git log --oneline, so having a good single-line summary is pretty important.

It is especially important that we can understand that without having to go to a potentially ephemeral source; if we can't understand what was done without checking the bug tracking system we used five years ago, and that no longer exists, we have a potential problem. ;)

This is pretty solid on that front: you can understand everything without reference to #297, except the summary, so good job on that front.

Once you have changed that up you are welcome to merge this yourself, or to ping me and I will get it merged.

Contributor

slippycheeze commented Jan 24, 2013

Hey. The code change itself looks reasonable enough, so 👍 to that.

The commit message is pretty solid too - it includes the essential detail of why this solution was chosen, over the million other possibilities. The one shortfall is the first line: please make it a summary suitable for use in a changelog or release announcement. It should summarise the user-visible effect of the change. (eg: "Disable lshw DMI parsing on KVM virtual machines")

In general our release announcements - and a lot of history review - is done with the information you get from git log --oneline, so having a good single-line summary is pretty important.

It is especially important that we can understand that without having to go to a potentially ephemeral source; if we can't understand what was done without checking the bug tracking system we used five years ago, and that no longer exists, we have a potential problem. ;)

This is pretty solid on that front: you can understand everything without reference to #297, except the summary, so good job on that front.

Once you have changed that up you are welcome to merge this yourself, or to ping me and I will get it merged.

@tjmcs

This comment has been minimized.

Show comment Hide comment
@tjmcs

tjmcs Jan 24, 2013

changed the summary of the commit message to something a bit more descriptive (albeit a bit longer). If the revised summary is OK with you, @daniel-pittman, feel free to merge the pull request.

tjmcs commented Jan 24, 2013

changed the summary of the commit message to something a bit more descriptive (albeit a bit longer). If the revised summary is OK with you, @daniel-pittman, feel free to merge the pull request.

@slippycheeze

This comment has been minimized.

Show comment Hide comment
@slippycheeze

slippycheeze Jan 24, 2013

Contributor

Ideally we want that to be 50 characters or less; I think the reference to the ticket could probably go from the summary line without losing much. That would bring it down to a good length.

Anyone who reads the body can find the link to the ticket and discussion, which is awesome and useful, but we probably don't need that in, eg, the announcement message or whatever. At least, not more than an (#297) MK now uses 'lshw -disable dmi' under kvm anyway.

I am happy enough with a merge with that updated, though.

Contributor

slippycheeze commented Jan 24, 2013

Ideally we want that to be 50 characters or less; I think the reference to the ticket could probably go from the summary line without losing much. That would bring it down to a good length.

Anyone who reads the body can find the link to the ticket and discussion, which is awesome and useful, but we probably don't need that in, eg, the announcement message or whatever. At least, not more than an (#297) MK now uses 'lshw -disable dmi' under kvm anyway.

I am happy enough with a merge with that updated, though.

@tjmcs

This comment has been minimized.

Show comment Hide comment
@tjmcs

tjmcs Jan 25, 2013

Just modified the summary; LMK if that works for you, @daniel-pittman

tjmcs commented Jan 25, 2013

Just modified the summary; LMK if that works for you, @daniel-pittman

@slippycheeze

This comment has been minimized.

Show comment Hide comment
@slippycheeze

slippycheeze Jan 25, 2013

Contributor

That looks reasonable enough, thanks.

Contributor

slippycheeze commented Jan 25, 2013

That looks reasonable enough, thanks.

slippycheeze added a commit that referenced this pull request Jan 25, 2013

Merge pull request #56 from tjmcs/tb/fixes_razor_issue_297
Fixes Razor issue 297; MK now uses 'lshw -disable dmi' under kvm

@slippycheeze slippycheeze merged commit 4fbd56e into puppetlabs:master Jan 25, 2013

@tjmcs tjmcs deleted the tjmcs:tb/fixes_razor_issue_297 branch Jan 25, 2013

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