On Fri, Aug 14, 2009 at 5:41 PM, Luke Kanies<[email protected]> wrote: > > On Aug 14, 2009, at 1:45 PM, Nigel Kersten wrote: > >> >> >> Signed-off-by: Nigel Kersten <[email protected]> >> --- >> lib/puppet/provider/service/debian.rb | 33 ++++++++----- >> spec/unit/provider/service/debian.rb | 83 ++++++++++++++++++++++++ >> +++++++++ >> 2 files changed, 103 insertions(+), 13 deletions(-) >> create mode 100755 spec/unit/provider/service/debian.rb >> >> diff --git a/lib/puppet/provider/service/debian.rb b/lib/puppet/ >> provider/service/debian.rb >> index 8527ae0..658b463 100755 >> --- a/lib/puppet/provider/service/debian.rb >> +++ b/lib/puppet/provider/service/debian.rb >> @@ -4,11 +4,16 @@ >> Puppet::Type.type(:service).provide :debian, :parent => :init do >> desc "Debian's form of ``init``-style management. >> >> The only difference is that this supports service enabling and >> disabling >> - via ``update-rc.d``. >> + via ``update-rc.d`` and determines enabled status via ``invoke- >> rc.d``. > > This is a behaviour change and should be marked so on the ticket. No > idea how many people it will affect.
Yes is is a change, I'll add that. It means that the service provider now works with insserv installed, whereas it hasn't up till now due to doing the kind of hacky output parsing. > >> >> " >> >> - commands :update => "/usr/sbin/update-rc.d" >> + commands :update_rc => "/usr/sbin/update-rc.d" >> + # note this isn't being used as a command until >> + # http://projects.reductivelabs.com/issues/2538 >> + # is resolved. >> + commands :invoke_rc => "/usr/sbin/invoke-rc.d" >> + >> defaultfor :operatingsystem => [:debian, :ubuntu] >> >> def self.defpath >> @@ -17,24 +22,26 @@ >> Puppet::Type.type(:service).provide :debian, :parent => :init do >> >> # Remove the symlinks >> def disable >> - update "-f", @resource[:name], "remove" >> - update @resource[:name], "stop", "00", "1", "2", "3", "4", >> "5", "6", "." >> + update_rc "-f", @resource[:name], "remove" >> + update_rc @resource[:name], "stop", "00", "1", "2", "3", >> "4", "5", "6", "." >> end >> >> def enabled? >> - output = update "-n", "-f", @resource[:name], "remove" >> - >> - # If it's enabled, then it will print output showing >> removal of >> - # links. >> - if output =~ /etc\/rc[\dS].d\/S|not installed/ >> - return :true >> + # TODO: Replace system() call when Puppet::Util.execute >> gives us a way >> + # to determine exit status. >> http://projects.reductivelabs.com/issues/2538 >> + system("/usr/sbin/invoke-rc.d --query #...@resource[:name]} >> start") > > Even if you're using system(), you should pass it an array. That way > you get a direct exec instead of a shell interpretation. With this > code, someone could have 'foo; rm -rf /etc; echo' as the service name > and it would do what you don't want. > > Not that we never make this mistake in the code, but I try to make it > as rarely as possible (and it's why execute() prefers an array). Ah. I was feeling uncomfortable about this, but didn't realize system() could take an array. will do and resend. > >> >> + # 104 is the exit status when you query start an enabled >> service. >> + # See x-man-page://invoke-rc.d >> + if $?.exitstatus == 104 >> + return :true >> else >> - return :false >> + return :false >> end >> end >> >> def enable >> - update "-f", @resource[:name], "remove" >> - update @resource[:name], "defaults" >> + update_rc "-f", @resource[:name], "remove" >> + update_rc @resource[:name], "defaults" >> end >> end >> diff --git a/spec/unit/provider/service/debian.rb b/spec/unit/ >> provider/service/debian.rb >> new file mode 100755 >> index 0000000..0c31c01 >> --- /dev/null >> +++ b/spec/unit/provider/service/debian.rb >> @@ -0,0 +1,83 @@ >> +#!/usr/bin/env ruby >> +# >> +# Unit testing for the debian service provider >> +# >> + >> +require File.dirname(__FILE__) + '/../../../spec_helper' >> + >> +provider_class = Puppet::Type.type(:service).provider(:debian) >> + >> +describe provider_class do >> + >> + before(:each) do >> + # Create a mock resource >> + �...@resource = stub 'resource' >> + >> + �...@provider = provider_class.new >> + >> + # A catch all; no parameters set >> + �[email protected](:[]).returns(nil) >> + >> + # But set name, source and path >> + �[email protected](:[]).with(:name).returns "myservice" >> + �[email protected](:[]).with(:ensure).returns :enabled >> + �[email protected](:ref).returns "Service[myservice]" >> + >> + �[email protected] = @resource >> + >> + �[email protected](:command).with(:update_rc).returns >> "update_rc" >> + �[email protected](:command).with(:invoke_rc).returns >> "invoke_rc" >> + >> + �[email protected](:update_rc) >> + �[email protected](:invoke_rc) >> + end >> + >> + it "should have an enabled? method" do >> + �[email protected] respond_to(:enabled?) >> + end >> + >> + it "should have an enable method" do >> + �[email protected] respond_to(:enable) >> + end >> + >> + it "should have a disable method" do >> + �[email protected] respond_to(:disable) >> + end >> + >> + describe "when enabling" do >> + it "should call update-rc.d twice" do >> + �[email protected](:update_rc).twice >> + �[email protected] >> + end >> + end >> + >> + describe "when disabling" do >> + it "should call update-rc.d twice" do >> + �[email protected](:update_rc).twice >> + �[email protected] >> + end >> + end > > These aren't the most meaningful of tests, but I'd at least just say > 'it should use update-rc.d' rather than 'it should call it twice'. > >> >> + describe "when checking whether it is enabled" do >> + it "should call Kernel.system() with the appropriate >> parameters" do >> + �[email protected](:system).with("/usr/sbin/invoke-rc.d >> --query #...@resource[:name]} start").once >> + �[email protected]? >> + end >> + >> + it "should return true when invoke-rc.d exits with 104 >> status" do >> + �[email protected](:system) >> + $?.stubs(:exitstatus).returns(104) >> + �[email protected]?.should == :true >> + end >> + >> + # pick a range of non-104 numbers, strings and booleans to >> test with. >> + [-100, -1, 0, 1, 100, "foo", "", :true, :false].each do | >> exitstatus| >> + it "should return false when invoke-rc.d exits with >> #{exitstatus} status" do >> + �[email protected](:system) >> + $?.stubs(:exitstatus).returns(exitstatus) >> + �[email protected]?.should == :false >> + end >> + end >> + end >> + >> + end >> -- >> 1.6.4 >> >> >> > > > > -- > A censor is a man who knows more than he thinks you ought to. > -- Granville Hicks > --------------------------------------------------------------------- > Luke Kanies | http://reductivelabs.com | http://madstop.com > > > > > -- Nigel Kersten [email protected] System Administrator Google, Inc. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en -~----------~----~----~----~------~----~------~--~---
