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

(FACT-830) Use xen-toolstack to find xen command. #872

Merged
merged 1 commit into from Mar 30, 2015

Conversation

@cropalato
Copy link

cropalato commented Mar 20, 2015

No description provided.

@melissa

This comment has been minimized.

Copy link
Member

melissa commented Mar 24, 2015

Thanks for submitting this as a pull request! This looks like something we'll be able to pull in to the next Facter 2.x release, but I'll need you to make a few updates.

First, could you update the commit message to be something like (FACT-830) Use xen-toolstack to find xen command, or something. Feel free to make the wording to be more accurate. I'll also need some more explanation in the commit message explaining why you made this change, both the problem and the solution in this commit. https://github.com/puppetlabs/facter/blob/2.x/CONTRIBUTING.md has more guidelines about formatting the commit message and what content it needs.

It would also be awesome to update the spec tests for this fact (spec/unit/util/xendomains_spec.rb). It looks like there's already a test verifies that we prefer xl over xm if both are present. Could you add something that parses the output of xen-toolstack if that command is present, and then gets the right output based on which command has been set to default?

The last thing is that we don't yet have this fact in Facter 3. Facter 3 is our port of the facter codebase from Ruby to C++11. Would you be interested in porting this so we have it available when Facter 3 comes out? That codebase lives in the facter/master branch.

@melissa melissa added the PR Triage label Mar 25, 2015
@melissa melissa changed the title * Fixing FACT-830. facter was returning empty on debian hosts. (FACT-830) Use xen-toolstack to find xen command. Mar 25, 2015
@melissa melissa added the Triaged label Mar 25, 2015
@cropalato

This comment has been minimized.

Copy link
Author

cropalato commented Mar 27, 2015

@melissa

I'm reading a little bit more about rspec (never used before).
About facter3, sure I can help. I'll take a look next week if it is not urgent.

@melissa

This comment has been minimized.

Copy link
Member

melissa commented Mar 27, 2015

@cropalato wonderful, thank you! If you want any help with rspec, just let me know. I'd be happy to help or offer guidance.

There is no rush on facter 3. Whenever you have the time or interest to do that work would be wonderful!

@cropalato

This comment has been minimized.

Copy link
Author

cropalato commented Mar 27, 2015

@melissa
I'm trying to update the spec tests. but it is failing. not sure yet how to use that. Can you point me a good doc or if you have time, can you take a look and let me know what i'm doing wrong?
Thanks,

@whopper

This comment has been minimized.

Copy link
Contributor

whopper commented Mar 28, 2015

@cropalato thanks for hammering on those tests! @melissa and I spent some time reviewing your code and found a few points that should be addressed, which will also fix the spec failures. I'll leave a few comments in the diff.

Also, for reference, all of the changes I mentioned are in a test branch on my fork, and can be found here: https://github.com/whopper/facter/blob/test/xendomains/lib/facter/util/xendomains.rb and here: https://github.com/whopper/facter/blob/test/xendomains/spec/unit/util/xendomains_spec.rb. The specs there are all passing.

@@ -2,6 +2,10 @@
#
module Facter::Util::Xendomains
XEN_COMMANDS = ['/usr/sbin/xl', '/usr/sbin/xm']
if File.file?('/usr/lib/xen-common/bin/xen-toolstack')

This comment has been minimized.

Copy link
@whopper

whopper Mar 28, 2015

Contributor

Putting this logic inside of the self.xen_commands method would be preferable, and make this much easier to test. It could look like this:

def self.xen_command
  if File.file?('/usr/lib/xen-common/bin/xen-toolstack')
    xen_toolstack_cmd = Facter::Util::Resolution.exec('/usr/lib/xen-common/bin/xen-toolstack')
    if xen_toolstack_cmd
      xen_toolstack_cmd.chomp
    else
      nil
    end
  else
    XEN_COMMANDS.find { |cmd| Facter::Util::Resolution.which(cmd) }
  end
end

Note that we don't need to alter the constant XEN_COMMANDS array if we know that xen-toolstack is available. In my understanding, xen-toolstack will return the full path of the selected tool, either /usr/sbin/xl or /usr/sbin/xm (or nil, which is accounted for here), so all we need to do is return its output.

No need to run which on the returned path from xen-toolstack, as it's already an absolute path according to your specs. Correct me if I'm wrong on this, and if so we'd just need to return Facter::Util::Resolution.which(xen_toolstack_cmd.chomp) instead.

@@ -7,60 +7,105 @@

let(:xen0_domains) { my_fixture_read("xendomains") }

describe "when the xl command is present" do

This comment has been minimized.

Copy link
@whopper

whopper Mar 28, 2015

Contributor

With the Facter::Util::Xendomains.xen_command method updated as described above, these tests become much more manageable. See https://github.com/whopper/facter/blob/test/xendomains/spec/unit/util/xendomains_spec.rb for a working example.

One of the issues you were running into here was that the call to File.file? to look for xen-toolstack was not being stubbed here, resulting in your new logic being skipped while running through the tests.

@whopper

This comment has been minimized.

Copy link
Contributor

whopper commented Mar 28, 2015

@cropalato let us know if our feedback helps! Also, I made a few assumptions about how xen-toolstack works in my example code, so feel free to change it as needed if I was off!

@cropalato

This comment has been minimized.

Copy link
Author

cropalato commented Mar 30, 2015

@whopper / @melissa thanks for the help. I read and now i know a little bit better what to do.
I changed my code reflect your suggestions. thanks a lot. It is working now.

if File.file?('/usr/lib/xen-common/bin/xen-toolstack')
xen_toolstack_cmd = Facter::Util::Resolution.exec('/usr/lib/xen-common/bin/xen-toolstack')
if xen_toolstack_cmd
xen_toolstack_cmd.chomp

This comment has been minimized.

Copy link
@melissa

melissa Mar 30, 2015

Member

It looks like you have a hard tab here, could you change it to four two-space soft tabs?

if xen_toolstack_cmd
xen_toolstack_cmd.chomp
else
nil

This comment has been minimized.

Copy link
@melissa

melissa Mar 30, 2015

Member

Same on this line

before do
Facter::Util::Resolution.stubs(:which).with('/usr/sbin/xm').returns(nil)
Facter::Util::Resolution.expects(:exec).with('/usr/sbin/xm list 2>/dev/null').never
Facter::Util::Resolution.stubs(:exec).with('/usr/lib/xen-common/bin/xen-toolstack').returns('/usr/sbin/xl')

This comment has been minimized.

Copy link
@melissa

melissa Mar 30, 2015

Member

This hard tab should be soft tabs

Facter::Util::Resolution.expects(:exec).with('/usr/sbin/xl list 2>/dev/null').returns(xen0_domains)
Facter::Util::Xendomains.get_domains.should == %{web01,mailserver}
Facter::Util::Resolution.expects(:exec).with('/usr/sbin/xl list 2>/dev/null').returns(xen0_domains)
Facter::Util::Xendomains.get_domains.should == %{web01,mailserver}

This comment has been minimized.

Copy link
@melissa

melissa Mar 30, 2015

Member

hard tab => soft tabs

before do
Facter::Util::Resolution.stubs(:which).with('/usr/sbin/xm').returns('/usr/bin/xm')
Facter::Util::Resolution.expects(:exec).with('/usr/sbin/xm list 2>/dev/null').never
Facter::Util::Resolution.stubs(:exec).with('/usr/lib/xen-common/bin/xen-toolstack').returns('/usr/sbin/xm')

This comment has been minimized.

Copy link
@melissa

melissa Mar 30, 2015

Member

hard tab => soft tabs

Facter::Util::Xendomains.get_domains.should == %{web01,mailserver}
it "lists the domains running on Xen0 with the 'xm' command" do
Facter::Util::Resolution.expects(:exec).with('/usr/sbin/xm list 2>/dev/null').returns(xen0_domains)
Facter::Util::Xendomains.get_domains.should == %{web01,mailserver}

This comment has been minimized.

Copy link
@melissa

melissa Mar 30, 2015

Member

hard tab => soft tabs

before do
Facter::Util::Resolution.stubs(:which).with('/usr/sbin/xm').returns('/usr/sbin/xm')
Facter::Util::Resolution.stubs(:exec).with('/usr/lib/xen-common/bin/xen-toolstack').returns(nil)

This comment has been minimized.

Copy link
@melissa

melissa Mar 30, 2015

Member

hard tab => soft tabs

Facter::Util::Resolution.expects(:exec).with('/usr/sbin/xm list 2>/dev/null').returns(xen0_domains)
Facter::Util::Xendomains.get_domains.should == %{web01,mailserver}
it "returns nil" do
Facter::Util::Xendomains.get_domains.should == nil

This comment has been minimized.

Copy link
@melissa

melissa Mar 30, 2015

Member

hard tab => soft tabs

end
end
end

This comment has been minimized.

Copy link
@melissa

melissa Mar 30, 2015

Member

It looks like you have some extra spaces in these new lines, could you remove those?

@melissa

This comment has been minimized.

Copy link
Member

melissa commented Mar 30, 2015

@cropalato It's looking really good! All we have left are a few little changes before we can merge this. I left some comments about some whitespace and tabbing that should get fixed up. Thank you for all the work you've put into this!

@cropalato

This comment has been minimized.

Copy link
Author

cropalato commented Mar 30, 2015

@melissa, sorry about the tabs (i didn't use my computer). this was fixed. not sure if i understood the spec/unit/util/xendomains_spec.rb comment (It looks like you have some extra spaces in these new lines, could you remove those?). Are you talking about empty lines or about spaces at the end of the lines?

@melissa

This comment has been minimized.

Copy link
Member

melissa commented Mar 30, 2015

@cropalato there were a few trailing white spaces at the end of those lines, but it looks like you've cleaned them up. Thank you again for all your work! This looks good to me! I'm 👍 to merging it in.

@whopper

This comment has been minimized.

Copy link
Contributor

whopper commented Mar 30, 2015

👍

melissa added a commit that referenced this pull request Mar 30, 2015
(FACT-830) Use xen-toolstack to find xen command.
@melissa melissa merged commit 5cd1fff into puppetlabs:2.x Mar 30, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.