On Aug 14, 2009, at 6:42 PM, Nigel Kersten wrote: > > 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.
I didn't mean it was bad -- just that it should be logged as a behaviour change so it gets marked appropriately in the release notes. > >> >>> >>> " >>> >>> - 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 >>> + @resource.stubs(:[]).returns(nil) >>> + >>> + # But set name, source and path >>> + @resource.stubs(:[]).with(:name).returns "myservice" >>> + @resource.stubs(:[]).with(:ensure).returns :enabled >>> + @resource.stubs(:ref).returns "Service[myservice]" >>> + >>> + @provider.resource = @resource >>> + >>> + @provider.stubs(:command).with(:update_rc).returns >>> "update_rc" >>> + @provider.stubs(:command).with(:invoke_rc).returns >>> "invoke_rc" >>> + >>> + @provider.stubs(:update_rc) >>> + @provider.stubs(:invoke_rc) >>> + end >>> + >>> + it "should have an enabled? method" do >>> + @provider.should respond_to(:enabled?) >>> + end >>> + >>> + it "should have an enable method" do >>> + @provider.should respond_to(:enable) >>> + end >>> + >>> + it "should have a disable method" do >>> + @provider.should respond_to(:disable) >>> + end >>> + >>> + describe "when enabling" do >>> + it "should call update-rc.d twice" do >>> + @provider.expects(:update_rc).twice >>> + @provider.enable >>> + end >>> + end >>> + >>> + describe "when disabling" do >>> + it "should call update-rc.d twice" do >>> + @provider.expects(:update_rc).twice >>> + @provider.disable >>> + 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 >>> + @provider.expects(:system).with("/usr/sbin/invoke-rc.d >>> --query #...@resource[:name]} start").once >>> + @provider.enabled? >>> + end >>> + >>> + it "should return true when invoke-rc.d exits with 104 >>> status" do >>> + @provider.stubs(:system) >>> + $?.stubs(:exitstatus).returns(104) >>> + @provider.enabled?.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 >>> + @provider.stubs(:system) >>> + $?.stubs(:exitstatus).returns(exitstatus) >>> + @provider.enabled?.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 don't learn anything the second time you're kicked by a mule. -- Anonymous Texan --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
