Add lvm_support fact #25
Conversation
|
Thank you for this contribution! I cloned this pull request locally, rebased it on master, and ran the specs, and I'm getting some failures: Failures:
1) lvm_support fact on non-Linux OS should not exist
Failure/Error: Facter.fact(:lvm_support).value.should == 'no'
expected: "no"
got: nil (using ==)
# ./spec/unit/facter/lvm_support_spec.rb:9:in `block (3 levels) in '
2) lvm_support fact on Linux when it has vgdisplay, one physical volume and two volume group should set lvm_vg_* and lvm_pv_* facts
Failure/Error: Facter.value(:lvm_support).should == 'yes'
expected: "yes"
got: nil (using ==)
# ./spec/unit/facter/lvm_support_spec.rb:42:in `block (4 levels) in '
Could you see if you can duplicate this on your end? I've also added this repo to travis-ci, so if you re-push this this branch then it should have the specs run automatically. If possible you should rebase against master to pull .travis.yml into your branch so the travis-ci run has the best coverage possible. |
|
I've refactored the code to separate fact declarations and use confines for dependencies between facts. All tests pass now. |
|
CLA Signed by raphink on 2012-04-25 21:00:00 -0700 |
|
@adrienthebo I squashed the commits to make it easier and rebased on Puppet Labs' master. |
| Facter.add('lvm_support') do | ||
| confine :kernel => :linux | ||
|
|
||
| vgdisplay = Facter::Util::Resolution.exec('which vgs') |
There was a problem hiding this comment.
Any sort of operation that is part of resolving a fact should be within the setcode block.
|
@raphink thanks for updating this! Please pardon the delay in response on this; working on the backlog of Puppet pull requests has kept me busy and I haven't been able to work much on this. Generally this PR is good, but I think it could use some refactoring to make sure that it's as easy to understand as use as possible. Could you check out the comments I've attached? |
|
I updated the PR with commits fixing most of the comments you left. The only thing left is the |
* Put Facter::Util::Resolution.exec inside setcode block when
possible
* Use true/nil instead of yes/no
* Use each_with_index
* Move doc before each fact declaration
|
@raphink the problem with using variables outside of setcode blocks is that they will never be reset unless the process running Facter is entirely restarted, like in (https://github.com/puppetlabs/puppetlabs-lvm/pull/25/files#L1R48). If dynamic facts like this are absolutely needed then we'll have to live with this, but to be sure this is the only way to solve this problem? Do we have to reuse that information? |
|
I noticed this hasn't seen an update in about 8 days; is this something still in motion? |
|
Sorry, same as the rest: been away for some time and I'm quite busy lately ;-) I haven't met this situation where the fact is not recalculated, because we call |
| Facter.add('lvm_vgs') do | ||
| confine :lvm_support => true | ||
| vgs = Facter::Util::Resolution.exec('vgs -o name --noheadings 2>/dev/null') | ||
| if vgs.nil? |
There was a problem hiding this comment.
Final nitpick; all of this logic from line 17 to line 22 should be in one setcode block.
There was a problem hiding this comment.
Right, but if I do that, vg_list will only be assigned locally, and won't be available on line 28. Or should I nest the facts then?
There was a problem hiding this comment.
Ah drat, I missed that. Facts can't be nested; I don't know what would happen but it wouldn't be good.
|
So with respect to resolving facts that need iteration, there isn't a really good way to do it. It generally works but if someone is using Facter in a long running process,things could break; those facts will never be updated until the process restarts. It isn't a common case, and we're talking about LVM here - this stuff doesn't change a lot anyways. These are all small concerns and basically, this is going to get merged shortly, but I'm hoping we can shoot for good instead of adequate. However, Facter has limitations so we do what we must. |
|
summary: merged into master in cf36df8. Thanks again for this contribution! |
|
Great thanks. Sorry it couldn't be nicer than this.
|
Add lvm_support fact
This PR adds a series of facts: