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

Fix #2663 - refactor to use shell_out in rpm provider #2698

Closed
wants to merge 5 commits into from

Conversation

david-crowder
Copy link

Fixes #2663 by using shell_out instead of popen4

@david-crowder
Copy link
Author

The appveyor failure does not appear to be tied to any of the changes I made.
Curious 😐

@btm
Copy link
Contributor

btm commented Dec 22, 2014

Yeah, looks like master was broken by #2288. I've pinged someone to fix it. Apparently it's a transient error that they thought they resolved, so keep an eye out for a fix and then rebase against master. If you push another commit it might pass the next time :)

@new_resource.version($2)
@candidate_version = $2
end
status = shell_out("rpm -qp --queryformat '%{NAME} %{VERSION}-%{RELEASE}\n' #{@new_resource.source}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to use shell_out! here so an exception is raised if rpm returns a failure via a non-zero return code.

@btm
Copy link
Contributor

btm commented Dec 22, 2014

It'd wouldn't be a bad opportunity to switch this test over to use let instead of instance variables.

@btm btm added Bug labels Dec 22, 2014
@randomcamel
Copy link
Contributor

Appveyor should pass consistently now.

@david-crowder
Copy link
Author

Rebased off of master and fixed some of the recommendations by @btm. Still looking into changing the tests to use let instead of instance variables.

@lamont-granquist lamont-granquist added this to the 12.0.4 milestone Dec 23, 2014
@david-crowder
Copy link
Author

Having a little trouble parsing why appveyor failed. Anyone have any clues?

@david-crowder
Copy link
Author

Updated (or at least attempted) to the let syntax for the tests. Feel free to call out any mistakes, the whole syntax is new to me and I am not super familiar with rspec in general.

As I write this comment I realize that every chefspec test I have written has a let(:chef_run). Definitely a lightbulb moment 😃

@btm
Copy link
Contributor

btm commented Dec 23, 2014

 1) Chef::Provider::Package::Rpm when determining the current state of the package should raise an exception if rpm fails to run
     Failure/Error: Unable to find matching line from backtrace
       expected Chef::Exceptions::Package, got #<NoMethodError: undefined method `action_any' for #<Chef::Provider::Package::Rpm:0xe7a1d30>> with backtrace:
         # C:/projects/chef/lib/chef/provider.rb:145:in `run_action'
         # C:/projects/chef/spec/unit/provider/package/rpm_spec.rb:89:in `block (4 levels) in <top (required)>'
         # C:/projects/chef/spec/unit/provider/package/rpm_spec.rb:89:in `block (3 levels) in <top (required)>'
     # C:/projects/chef/spec/unit/provider/package/rpm_spec.rb:89:in `block (3 levels) in <top (required)>'

@david-crowder
Copy link
Author

Right, so it is failing with a NoMethodError, but why is it failing on windows but not in travis? Currently I don't have a means to reproduce locally. I suppose I can spin up a vm and install all the tools required to test. Didn't know if this is an error in my pull request or if it too was part of a larger windows error that someone with more context could quickly identify.

@btm
Copy link
Contributor

btm commented Dec 23, 2014

So Chef::Provider.run_action calls a few methods, including define_resource_requirements, process_resource_requirements, and load_current_resource, then later calls "action_#{@action}". The test expects to fail and never make it to the part where action_any gets called (any because that's what the test is passing as the action). However it is getting that far, causing that NoMethodError.

In lib/chef/provider/package/rpm.rb you call 'shell_out' line but in the test you're stubbing 'shell_out!', "rpm -q --queryformat '%{NAME} %{VERSION}-%{RELEASE}\n' #{@current_resource.package_name}" is actually getting run, and since it's shell_out whatever failures happen are getting swallowed since we never call error! on the returned shell_out object.

I think you probably just need to change shell_out to shell_out! in the provider there.

@btm
Copy link
Contributor

btm commented Dec 23, 2014

I think the reason it's failing on one platform and not the other, is it is actually running a command, and who knows what exitstatus it's getting, but that exitstatus could meet the assertion in define_resource_requirements.

@david-crowder
Copy link
Author

Awesome, thank you for your help. I really didn't know what to do with that error. The fact that it was failing on one platform and not the other had me super confused. I went ahead and changed the provider to use shell_out! and fixed the tests where appropriate.

@btm
Copy link
Contributor

btm commented Dec 23, 2014

It's interesting the assertation wants either exitstatus 0 or exitstatus 1. I wish there was a comment that explained why, I looked through the git history and didn't see anything.

I played around a bit on a windows and ubuntu system to see what I got from running "rpm" via shellout. Windows was returning 1. My best guess is that the code expects 0 if the package is installed and we get a version, and 1 if it is not installed, and something else if something goes wrong.

There's probably a subtle bug in there somewhere.

@tyler-ball
Copy link
Contributor

Can you add a require 'chef/mixin/shell_out' at the top of this file? I'm guessing it is getting transitively added, but I don't immediately see how.


@new_resource = Chef::Resource::Package.new("ImageMagick-c++")
@new_resource.source "/tmp/ImageMagick-c++-6.5.4.7-7.el6_5.x86_64.rpm"
subject(:provider) { Chef::Provider::Package::Rpm.new(new_resource, run_context) }
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't normally use the specific subject method - this should just be let(:provider).

I think subject is useful if you want to use the subject method in the spec, but we also normally don't do that.

@tyler-ball
Copy link
Contributor

This is looking good. I think there are a few more cleanup steps that can be done during the let conversion, then it should be good to go. Ping me if you need some help with my comments or if you want more explanation about my reasoning.

… require shellout explicitly in the provider
@lamont-granquist lamont-granquist modified the milestones: 12.1.0, 12.0.4 Jan 23, 2015
@lamont-granquist
Copy link
Contributor

👍

@chef/client-core @chef/client-engineers

@danielsdeleo
Copy link
Contributor

👍

2 similar comments
@jonlives
Copy link
Contributor

👍

@tyler-ball
Copy link
Contributor

👍

@jaymzh
Copy link
Collaborator

jaymzh commented Jan 28, 2015

👍

@tyler-ball
Copy link
Contributor

Merged in #2833

@tyler-ball tyler-ball closed this Jan 28, 2015
@thommay thommay added Type: Bug Does not work as expected. Status: Ready for Merge and removed Bug labels Jan 25, 2017
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Platform: RHEL-like Type: Bug Does not work as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rpm_package provider no candidate version error
10 participants