Uninstall wasn't working properly because the instances method relied on the pip command having been defined by calling lazy_pip. This lazy_pip pattern is a nice way of getting around Puppet's problem of disqualifying providers at the beginning of a run because the command doesn't yet exist even though part way through the run the command might exist.
Really, we need to fix puppet to lazily evaluate a provider command for validity only at the time the provider is needed. The refactoring also pointed out that query could just reuse the logic from instances. The tests were also refactored to use real resources instead of stubbed ones, and they reflect the implementation changes to instances. Paired-with: Richard Crowley <r...@rcrowley.org> Signed-off-by: Matt Robinson <m...@puppetlabs.com> --- Local-branch: feature/next/6257-pip_package_provider lib/puppet/provider/package/pip.rb | 18 ++---- spec/unit/provider/package/pip_spec.rb | 98 ++++++++++++++------------------ 2 files changed, 48 insertions(+), 68 deletions(-) diff --git a/lib/puppet/provider/package/pip.rb b/lib/puppet/provider/package/pip.rb index 0079756..5abbc13 100644 --- a/lib/puppet/provider/package/pip.rb +++ b/lib/puppet/provider/package/pip.rb @@ -25,29 +25,23 @@ Puppet::Type.type(:package).provide :pip, # that's managed by `pip` or an empty array if `pip` is not available. def self.instances packages = [] - execpipe "#{command :pip} freeze" do |process| + pip_cmd = which('pip') or return [] + execpipe "#{pip_cmd} freeze" do |process| process.collect do |line| next unless options = parse(line) packages << new(options) end end packages - rescue Puppet::DevError - [] end # Return structured information about a particular package or `nil` if # it is not installed or `pip` itself is not available. def query - execpipe "#{command :pip} freeze" do |process| - process.each do |line| - options = self.class.parse(line) - return options if options[:name] == @resource[:name] - end + self.class.instances.each do |provider_pip| + return provider_pip.properties if @resource[:name] == provider_pip.name end - nil - rescue Puppet::DevError - nil + return nil end # Ask the PyPI API for the latest version number. There is no local @@ -104,7 +98,7 @@ Puppet::Type.type(:package).provide :pip, def lazy_pip(*args) pip *args rescue NoMethodError => e - if pathname = `which pip`.chomp + if pathname = which('pip') self.class.commands :pip => pathname pip *args else diff --git a/spec/unit/provider/package/pip_spec.rb b/spec/unit/provider/package/pip_spec.rb index ad8201a..6809d3f 100644 --- a/spec/unit/provider/package/pip_spec.rb +++ b/spec/unit/provider/package/pip_spec.rb @@ -7,17 +7,16 @@ provider_class = Puppet::Type.type(:package).provider(:pip) describe provider_class do before do - @resource = stub("resource") - @provider = provider_class.new - @provider.instance_variable_set(:@resource, @resource) + @resource = Puppet::Resource.new(:package, "sdsfdssdhdfyjymdgfcjdfjxdrssf") + @provider = provider_class.new(@resource) end describe "parse" do it "should return a hash on valid input" do provider_class.parse("Django==1.2.5").should == { - :ensure => "1.2.5", - :name => "Django", + :ensure => "1.2.5", + :name => "Django", :provider => :pip, } end @@ -31,7 +30,7 @@ describe provider_class do describe "instances" do it "should return an array when pip is present" do - provider_class.expects(:command).with(:pip).returns("/fake/bin/pip") + provider_class.expects(:which).with('pip').returns("/fake/bin/pip") p = stub("process") p.expects(:collect).yields("Django==1.2.5") provider_class.expects(:execpipe).with("/fake/bin/pip freeze").yields(p) @@ -39,8 +38,7 @@ describe provider_class do end it "should return an empty array when pip is missing" do - provider_class.expects(:command).with(:pip).raises( - Puppet::DevError.new("Pretend pip isn't installed.")) + provider_class.expects(:which).with('pip').returns nil provider_class.instances.should == [] end @@ -49,32 +47,25 @@ describe provider_class do describe "query" do before do - @resource.stubs(:[]).with(:name).returns("Django") + @resource[:name] = "Django" end it "should return a hash when pip and the package are present" do - @provider.expects(:command).with(:pip).returns("/fake/bin/pip") - p = stub("process") - p.expects(:each).yields("Django==1.2.5") - @provider.expects(:execpipe).with("/fake/bin/pip freeze").yields(p) + provider_class.expects(:instances).returns [provider_class.new({ + :ensure => "1.2.5", + :name => "Django", + :provider => :pip, + })] + @provider.query.should == { - :ensure => "1.2.5", - :name => "Django", + :ensure => "1.2.5", + :name => "Django", :provider => :pip, } end - it "should return nil when pip is missing" do - @provider.expects(:command).with(:pip).raises( - Puppet::DevError.new("Pretend pip isn't installed.")) - @provider.query.should == nil - end - it "should return nil when the package is missing" do - @provider.expects(:command).with(:pip).returns("/fake/bin/pip") - p = stub("process") - p.expects(:each).yields("sdsfdssdhdfyjymdgfcjdfjxdrssf==0.0.0") - @provider.expects(:execpipe).with("/fake/bin/pip freeze").yields(p) + provider_class.expects(:instances).returns [] @provider.query.should == nil end @@ -83,12 +74,12 @@ describe provider_class do describe "latest" do it "should find a version number for Django" do - @resource.stubs(:[]).with(:name).returns "Django" + @resource[:name] = "Django" @provider.latest.should_not == nil end it "should not find a version number for sdsfdssdhdfyjymdgfcjdfjxdrssf" do - @resource.stubs(:[]).with(:name).returns "sdsfdssdhdfyjymdgfcjdfjxdrssf" + @resource[:name] = "sdsfdssdhdfyjymdgfcjdfjxdrssf" @provider.latest.should == nil end @@ -97,52 +88,46 @@ describe provider_class do describe "install" do before do - @resource.stubs(:[]).with(:name).returns("sdsfdssdhdfyjymdgfcjdfjxdrssf") + @resource[:name] = "sdsfdssdhdfyjymdgfcjdfjxdrssf" @url = "git+https://example.com/sdsfdssdhdfyjymdgfcjdfjxdrssf.git" end it "should install" do - @resource.stubs(:[]).with(:ensure).returns(:installed) - @resource.stubs(:[]).with(:source).returns(nil) - @provider.expects(:lazy_pip).with do |*args| - "install" == args[0] && "sdsfdssdhdfyjymdgfcjdfjxdrssf" == args[-1] - end.returns nil + @resource[:ensure] = :installed + @resource[:source] = nil + @provider.expects(:lazy_pip). + with("install", '-q', "sdsfdssdhdfyjymdgfcjdfjxdrssf") @provider.install end it "should install from SCM" do - @resource.stubs(:[]).with(:ensure).returns(:installed) - @resource.stubs(:[]).with(:source).returns(@url) - @provider.expects(:lazy_pip).with do |*args| - "#{@url}#egg=sdsfdssdhdfyjymdgfcjdfjxdrssf" == args[-1] - end.returns nil + @resource[:ensure] = :installed + @resource[:source] = @url + @provider.expects(:lazy_pip). + with("install", '-q', '-e', "#{@url}#egg=sdsfdssdhdfyjymdgfcjdfjxdrssf") @provider.install end - it "should install a particular revision" do - @resource.stubs(:[]).with(:ensure).returns("0123456") - @resource.stubs(:[]).with(:source).returns(@url) - @provider.expects(:lazy_pip).with do |*args| - "#{@url}@0123456#egg=sdsfdssdhdfyjymdgfcjdfjxdrssf" == args[-1] - end.returns nil + it "should install a particular SCM revision" do + @resource[:ensure] = "0123456" + @resource[:source] = @url + @provider.expects(:lazy_pip). + with("install", "-q", "-e", "#{@url}@0123456#egg=sdsfdssdhdfyjymdgfcjdfjxdrssf") @provider.install end it "should install a particular version" do - @resource.stubs(:[]).with(:ensure).returns("0.0.0") - @resource.stubs(:[]).with(:source).returns(nil) - @provider.expects(:lazy_pip).with do |*args| - "sdsfdssdhdfyjymdgfcjdfjxdrssf==0.0.0" == args[-1] - end.returns nil + @resource[:ensure] = "0.0.0" + @resource[:source] = nil + @provider.expects(:lazy_pip).with("install", "-q", "sdsfdssdhdfyjymdgfcjdfjxdrssf==0.0.0") @provider.install end it "should upgrade" do - @resource.stubs(:[]).with(:ensure).returns(:latest) - @resource.stubs(:[]).with(:source).returns(nil) - @provider.expects(:lazy_pip).with do |*args| - "--upgrade" == args[-2] && "sdsfdssdhdfyjymdgfcjdfjxdrssf" == args[-1] - end.returns nil + @resource[:ensure] = :latest + @resource[:source] = nil + @provider.expects(:lazy_pip). + with("install", "-q", "--upgrade", "sdsfdssdhdfyjymdgfcjdfjxdrssf") @provider.install end @@ -151,8 +136,9 @@ describe provider_class do describe "uninstall" do it "should uninstall" do - @resource.stubs(:[]).with(:name).returns("sdsfdssdhdfyjymdgfcjdfjxdrssf") - @provider.expects(:lazy_pip).returns(nil) + @resource[:name] = "sdsfdssdhdfyjymdgfcjdfjxdrssf" + @provider.expects(:lazy_pip). + with('uninstall', '-y', '-q', 'sdsfdssdhdfyjymdgfcjdfjxdrssf') @provider.uninstall end -- 1.7.3.1 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to puppet-dev@googlegroups.com. To unsubscribe from this group, send email to puppet-dev+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.