diff options
author | Lamont Granquist <lamont@scriptkiddie.org> | 2015-11-20 16:21:29 -0800 |
---|---|---|
committer | Lamont Granquist <lamont@scriptkiddie.org> | 2015-11-20 16:21:29 -0800 |
commit | 2fe875ce8d38631bf9b5975ff3f3cf5b532be2bd (patch) | |
tree | f3333bb3411b73443442697170e7ecd967a21125 | |
parent | 8d1257010fcdd413653af931cb495066d492830f (diff) | |
parent | 1b95e4dd04e66a4889e0fe352f0bd0bd64ca3beb (diff) | |
download | chef-2fe875ce8d38631bf9b5975ff3f3cf5b532be2bd.tar.gz |
Merge pull request #4185 from chef/lcg/dpkg-refactor
dpkg provider cleanup
-rw-r--r-- | CHANGELOG.md | 1 | ||||
-rw-r--r-- | lib/chef/provider/package/dpkg.rb | 141 | ||||
-rw-r--r-- | spec/unit/provider/package/dpkg_spec.rb | 223 |
3 files changed, 220 insertions, 145 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index ed861ea2a3..24d01075df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ * [**Patrick Connolly**](https://github.com/patcon) [pr#3529](https://github.com/chef/chef/pull/3529) Allow user@hostname format for knife-bootstrap +* [pr#4185](https://github.com/chef/chef/pull/4185) dpkg provider cleanup * [pr#4165](https://github.com/chef/chef/pull/4165) Multipackage internal API improvements * [pr#4081](https://github.com/chef/chef/pull/4081) RFC-037: add `chef_version` and `ohai_version` metadata * [pr#3530](https://github.com/chef/chef/pull/3530) Allow using --sudo option with user's home folder in knife bootstrap diff --git a/lib/chef/provider/package/dpkg.rb b/lib/chef/provider/package/dpkg.rb index 67e9b903c6..2de6226bb9 100644 --- a/lib/chef/provider/package/dpkg.rb +++ b/lib/chef/provider/package/dpkg.rb @@ -34,83 +34,61 @@ class Chef def define_resource_requirements super - requirements.assert(:install) do |a| - a.assertion{ not @new_resource.source.nil? } - a.failure_message Chef::Exceptions::Package, "Source for package #{@new_resource.name} required for action install" + + requirements.assert(:install, :upgrade) do |a| + a.assertion { !new_resource.source.nil? } + a.failure_message Chef::Exceptions::Package, "#{new_resource} the source property is required for action :install or :upgrade" end - # TODO this was originally written for any action in which .source is provided - # but would it make more sense to only look at source if the action is :install? - requirements.assert(:all_actions) do |a| - a.assertion { @source_exists } - a.failure_message Chef::Exceptions::Package, "Package #{@new_resource.name} not found: #{@new_resource.source}" - a.whyrun "Assuming it would have been previously downloaded." + requirements.assert(:install, :upgrade) do |a| + a.assertion { source_file_exist? } + a.failure_message Chef::Exceptions::Package, "#{new_resource} source file does not exist: #{new_resource.source}" + a.whyrun "Assuming it would have been previously created." end end def load_current_resource - @source_exists = true - @current_resource = Chef::Resource::Package.new(@new_resource.name) - @current_resource.package_name(@new_resource.package_name) - - if @new_resource.source - @source_exists = ::File.exists?(@new_resource.source) - if @source_exists - # Get information from the package if supplied - Chef::Log.debug("#{@new_resource} checking dpkg status") - status = shell_out_with_timeout("dpkg-deb -W #{@new_resource.source}") - pkginfo = status.stdout.split("\t") - unless pkginfo.empty? - @current_resource.package_name(pkginfo[0]) - @candidate_version = pkginfo[1].strip - end - else - # Source provided but not valid means we can't safely do further processing - return - end + @current_resource = Chef::Resource::Package.new(new_resource.name) + current_resource.package_name(new_resource.package_name) + + if source_file_exist? + @candidate_version = get_candidate_version + current_resource.package_name(get_package_name) + # if the source file exists then our package_name is right + current_resource.version(get_current_version) + elsif !installing? + # we can't do this if we're installing with no source, because our package_name + # is probably not right. + # + # if we're removing or purging we don't use source, and our package_name must + # be right so we can do this. + # + # we don't error here on the dpkg command since we'll handle the exception or + # the why-run message in define_resource_requirements. + current_resource.version(get_current_version) end - # Check to see if it is installed - package_installed = nil - Chef::Log.debug("#{@new_resource} checking install state") - status = shell_out_with_timeout("dpkg -s #{@current_resource.package_name}") - status.stdout.each_line do |line| - case line - when DPKG_INSTALLED - package_installed = true - when DPKG_VERSION - if package_installed - Chef::Log.debug("#{@new_resource} current version is #{$1}") - @current_resource.version($1) - end - end - end - - unless status.exitstatus == 0 || status.exitstatus == 1 - raise Chef::Exceptions::Package, "dpkg failed - #{status.inspect}!" - end - - @current_resource + current_resource end def install_package(name, version) - Chef::Log.info("#{@new_resource} installing #{@new_resource.source}") + Chef::Log.info("#{new_resource} installing #{new_resource.source}") run_noninteractive( - "dpkg -i#{expand_options(@new_resource.options)} #{@new_resource.source}" + "dpkg -i#{expand_options(new_resource.options)} #{new_resource.source}" ) end def remove_package(name, version) - Chef::Log.info("#{@new_resource} removing #{@new_resource.package_name}") + Chef::Log.info("#{new_resource} removing #{new_resource.package_name}") run_noninteractive( - "dpkg -r#{expand_options(@new_resource.options)} #{@new_resource.package_name}" + "dpkg -r#{expand_options(new_resource.options)} #{new_resource.package_name}" ) end def purge_package(name, version) - Chef::Log.info("#{@new_resource} purging #{@new_resource.package_name}") + Chef::Log.info("#{new_resource} purging #{new_resource.package_name}") run_noninteractive( - "dpkg -P#{expand_options(@new_resource.options)} #{@new_resource.package_name}" + "dpkg -P#{expand_options(new_resource.options)} #{new_resource.package_name}" ) end @@ -119,22 +97,65 @@ class Chef end def preseed_package(preseed_file) - Chef::Log.info("#{@new_resource} pre-seeding package installation instructions") + Chef::Log.info("#{new_resource} pre-seeding package installation instructions") run_noninteractive("debconf-set-selections #{preseed_file}") end def reconfig_package(name, version) - Chef::Log.info("#{@new_resource} reconfiguring") + Chef::Log.info("#{new_resource} reconfiguring") run_noninteractive("dpkg-reconfigure #{name}") end + private + + def get_current_version + Chef::Log.debug("#{new_resource} checking install state") + status = shell_out_with_timeout("dpkg -s #{current_resource.package_name}") + package_installed = false + status.stdout.each_line do |line| + case line + when DPKG_INSTALLED + package_installed = true + when DPKG_VERSION + if package_installed + Chef::Log.debug("#{new_resource} current version is #{$1}") + return $1 + end + end + end + return nil + end + # Runs command via shell_out_with_timeout with magic environment to disable # interactive prompts. Command is run with default localization rather # than forcing locale to "C", so command output may not be stable. - # - # FIXME: This should be "LC_ALL" => "en_US.UTF-8" in order to stabilize the output and get UTF-8 def run_noninteractive(command) - shell_out_with_timeout!(command, :env => { "DEBIAN_FRONTEND" => "noninteractive", "LC_ALL" => nil }) + shell_out_with_timeout!(command, :env => { "DEBIAN_FRONTEND" => "noninteractive" }) + end + + def source_file_exist? + new_resource.source && ::File.exist?(new_resource.source) + end + + def pkginfo + @pkginfo ||= + begin + Chef::Log.debug("#{new_resource} checking dpkg status") + status = shell_out_with_timeout!("dpkg-deb -W #{new_resource.source}") + status.stdout.split("\t") + end + end + + def get_candidate_version + pkginfo[1].strip unless pkginfo.empty? + end + + def get_package_name + pkginfo[0] unless pkginfo.empty? + end + + def installing? + [:install, :upgrade].include?(action) end end diff --git a/spec/unit/provider/package/dpkg_spec.rb b/spec/unit/provider/package/dpkg_spec.rb index b868128147..2ded8313c0 100644 --- a/spec/unit/provider/package/dpkg_spec.rb +++ b/spec/unit/provider/package/dpkg_spec.rb @@ -19,42 +19,109 @@ require 'spec_helper' describe Chef::Provider::Package::Dpkg do + let(:node) { Chef::Node.new } + let(:events) { Chef::EventDispatch::Dispatcher.new } + let(:run_context) { Chef::RunContext.new(node, {}, events) } + let(:package) { "wget" } + let(:source) { "/tmp/wget_1.11.4-1ubuntu1_amd64.deb" } + let(:new_resource) do + new_resource = Chef::Resource::Package.new(package) + new_resource.source source + new_resource + end + let(:provider) { Chef::Provider::Package::Dpkg.new(new_resource, run_context) } + + let(:dpkg_deb_version) { "1.11.4" } + let(:dpkg_deb_status) { status = double(:stdout => "#{package}\t#{dpkg_deb_version}", :exitstatus => 0) } + let(:dpkg_s_version) { "1.11.4-1ubuntu1" } + let(:dpkg_s_status) do + stdout = <<-DPKG_S +Package: #{package} +Status: install ok installed +Priority: important +Section: web +Installed-Size: 1944 +Maintainer: Ubuntu Core developers <ubuntu-devel-discuss@lists.ubuntu.com> +Architecture: amd64 +Version: #{dpkg_s_version} +Config-Version: #{dpkg_s_version} +Depends: libc6 (>= 2.8~20080505), libssl0.9.8 (>= 0.9.8f-5) +Conflicts: wget-ssl + DPKG_S + status = double(:stdout => stdout, :exitstatus => 1) + end + before(:each) do - @node = Chef::Node.new - @events = Chef::EventDispatch::Dispatcher.new - @run_context = Chef::RunContext.new(@node, {}, @events) - @new_resource = Chef::Resource::Package.new("wget") - @new_resource.source "/tmp/wget_1.11.4-1ubuntu1_amd64.deb" + allow(provider).to receive(:shell_out!).with("dpkg-deb -W #{new_resource.source}", timeout: 900).and_return(dpkg_deb_status) + allow(provider).to receive(:shell_out).with("dpkg -s #{package}", timeout: 900).and_return(dpkg_s_status) + allow(::File).to receive(:exist?).with(source).and_return(true) + end + + describe "#define_resource_requirements" do + it "should raise an exception if a source is supplied but not found when :install" do + allow(::File).to receive(:exist?).with(source).and_return(false) + expect { provider.run_action(:install) }.to raise_error(Chef::Exceptions::Package) + end + + it "should raise an exception if a source is supplied but not found when :upgrade" do + allow(::File).to receive(:exist?).with(source).and_return(false) + expect { provider.run_action(:upgrade) }.to raise_error(Chef::Exceptions::Package) + end + + # FIXME? we're saying we ignore source, but should supplying source on :remove or :purge be an actual error? + it "should not raise an exception if a source is supplied but not found when :remove" do + allow(::File).to receive(:exist?).with(source).and_return(false) + expect(provider).to receive(:action_remove) + expect { provider.run_action(:remove) }.not_to raise_error + end + + it "should not raise an exception if a source is supplied but not found when :purge" do + allow(::File).to receive(:exist?).with(source).and_return(false) + expect(provider).to receive(:action_purge) + expect { provider.run_action(:purge) }.not_to raise_error + end - @provider = Chef::Provider::Package::Dpkg.new(@new_resource, @run_context) + it "should raise an exception if a source is nil when :install" do + new_resource.source nil + allow(::File).to receive(:exist?).with(source).and_return(false) + expect { provider.run_action(:install) }.to raise_error(Chef::Exceptions::Package) + end - @status = double(:stdout => "", :exitstatus => 0) - allow(@provider).to receive(:shell_out).and_return(@status) + it "should raise an exception if a source is nil when :upgrade" do + new_resource.source nil + allow(::File).to receive(:exist?).with(source).and_return(false) + expect { provider.run_action(:upgrade) }.to raise_error(Chef::Exceptions::Package) + end - allow(::File).to receive(:exists?).and_return(true) + it "should not raise an exception if a source is nil when :remove" do + new_resource.source nil + allow(::File).to receive(:exist?).with(source).and_return(false) + expect(provider).to receive(:action_remove) + expect { provider.run_action(:remove) }.not_to raise_error + end + + it "should not raise an exception if a source is nil when :purge" do + new_resource.source nil + allow(::File).to receive(:exist?).with(source).and_return(false) + expect(provider).to receive(:action_purge) + expect { provider.run_action(:purge) }.not_to raise_error + end end describe "when loading the current resource state" do it "should create a current resource with the name of the new_resource" do - @provider.load_current_resource - expect(@provider.current_resource.package_name).to eq("wget") - end - - it "should raise an exception if a source is supplied but not found" do - @provider.load_current_resource - @provider.define_resource_requirements - allow(::File).to receive(:exists?).and_return(false) - expect { @provider.run_action(:install) }.to raise_error(Chef::Exceptions::Package) + provider.load_current_resource + expect(provider.current_resource.package_name).to eq("wget") end describe 'gets the source package version from dpkg-deb' do def check_version(version) - @status = double(:stdout => "wget\t#{version}", :exitstatus => 0) - allow(@provider).to receive(:shell_out).with("dpkg-deb -W #{@new_resource.source}", timeout: 900).and_return(@status) - @provider.load_current_resource - expect(@provider.current_resource.package_name).to eq("wget") - expect(@provider.candidate_version).to eq(version) + status = double(:stdout => "wget\t#{version}", :exitstatus => 0) + expect(provider).to receive(:shell_out!).with("dpkg-deb -W #{new_resource.source}", timeout: 900).and_return(status) + provider.load_current_resource + expect(provider.current_resource.package_name).to eq("wget") + expect(provider.candidate_version).to eq(version) end it 'if short version provided' do @@ -74,129 +141,115 @@ describe Chef::Provider::Package::Dpkg do end end - it "gets the source package name from dpkg-deb correctly when the package name has `-', `+' or `.' characters" do - stdout = "f.o.o-pkg++2\t1.11.4-1ubuntu1" - status = double(:stdout => stdout, :exitstatus => 1) - allow(@provider).to receive(:shell_out).and_return(status) - @provider.load_current_resource - expect(@provider.current_resource.package_name).to eq("f.o.o-pkg++2") + describe "when the package name has `-', `+' or `.' characters" do + let(:package) { "f.o.o-pkg++2" } + + it "gets the source package name from dpkg-deb correctly" do + provider.load_current_resource + expect(provider.current_resource.package_name).to eq("f.o.o-pkg++2") + end end - it "gets the source package version from dpkg-deb correctly when the package version has `~', `-', `+' or `.' characters" do - stdout = "b.a.r-pkg++1\t1.2.3+3141592-1ubuntu1~lucid" - status = double(:stdout => stdout, :exitstatus => 1) - allow(@provider).to receive(:shell_out).and_return(status) - @provider.load_current_resource - expect(@provider.candidate_version).to eq('1.2.3+3141592-1ubuntu1~lucid') + describe "when the package version has `~', `-', `+' or `.' characters" do + let(:package) { "b.a.r-pkg++1" } + let(:dpkg_deb_version) { "1.2.3+3141592-1ubuntu1~lucid" } + let(:dpkg_s_version) { "1.2.3+3141592-1ubuntu1~lucid" } + + it "gets the source package version from dpkg-deb correctly when the package version has `~', `-', `+' or `.' characters" do + provider.load_current_resource + expect(provider.candidate_version).to eq('1.2.3+3141592-1ubuntu1~lucid') + end end it "should raise an exception if the source is not set but we are installing" do - @new_resource = Chef::Resource::Package.new("wget") - @provider.new_resource = @new_resource - @provider.load_current_resource - @provider.define_resource_requirements - expect { @provider.run_action(:install)}.to raise_error(Chef::Exceptions::Package) + new_resource = Chef::Resource::Package.new("wget") + provider.new_resource = new_resource + provider.load_current_resource + provider.define_resource_requirements + expect { provider.run_action(:install)}.to raise_error(Chef::Exceptions::Package) end it "should return the current version installed if found by dpkg" do - stdout = <<-DPKG_S -Package: wget -Status: install ok installed -Priority: important -Section: web -Installed-Size: 1944 -Maintainer: Ubuntu Core developers <ubuntu-devel-discuss@lists.ubuntu.com> -Architecture: amd64 -Version: 1.11.4-1ubuntu1 -Config-Version: 1.11.4-1ubuntu1 -Depends: libc6 (>= 2.8~20080505), libssl0.9.8 (>= 0.9.8f-5) -Conflicts: wget-ssl -DPKG_S - status = double(:stdout => stdout, :exitstatus => 1) - allow(@provider).to receive(:shell_out).with("dpkg -s wget", timeout: 900).and_return(status) - - @provider.load_current_resource - expect(@provider.current_resource.version).to eq("1.11.4-1ubuntu1") + provider.load_current_resource + expect(provider.current_resource.version).to eq("1.11.4-1ubuntu1") end it "should raise an exception if dpkg fails to run" do status = double(:stdout => "", :exitstatus => -1) - allow(@provider).to receive(:shell_out).and_return(status) - expect { @provider.load_current_resource }.to raise_error(Chef::Exceptions::Package) + expect(provider).to receive(:shell_out_with_timeout!).with("dpkg-deb -W /tmp/wget_1.11.4-1ubuntu1_amd64.deb").and_raise(Mixlib::ShellOut::ShellCommandFailed) + expect { provider.load_current_resource }.to raise_error(Mixlib::ShellOut::ShellCommandFailed) end end describe Chef::Provider::Package::Dpkg, "install and upgrade" do it "should run dpkg -i with the package source" do - expect(@provider).to receive(:run_noninteractive).with( + expect(provider).to receive(:run_noninteractive).with( "dpkg -i /tmp/wget_1.11.4-1ubuntu1_amd64.deb" ) - @provider.install_package("wget", "1.11.4-1ubuntu1") + provider.install_package("wget", "1.11.4-1ubuntu1") end it "should run dpkg -i if the package is a path and the source is nil" do - @new_resource = Chef::Resource::Package.new("/tmp/wget_1.11.4-1ubuntu1_amd64.deb") - @provider = Chef::Provider::Package::Dpkg.new(@new_resource, @run_context) - expect(@provider).to receive(:run_noninteractive).with( + new_resource.name = "/tmp/wget_1.11.4-1ubuntu1_amd64.deb" + expect(provider).to receive(:run_noninteractive).with( "dpkg -i /tmp/wget_1.11.4-1ubuntu1_amd64.deb" ) - @provider.install_package("/tmp/wget_1.11.4-1ubuntu1_amd64.deb", "1.11.4-1ubuntu1") + provider.install_package("/tmp/wget_1.11.4-1ubuntu1_amd64.deb", "1.11.4-1ubuntu1") end it "should run dpkg -i if the package is a path and the source is nil for an upgrade" do - @new_resource = Chef::Resource::Package.new("/tmp/wget_1.11.4-1ubuntu1_amd64.deb") - @provider = Chef::Provider::Package::Dpkg.new(@new_resource, @run_context) - expect(@provider).to receive(:run_noninteractive).with( + new_resource.name = "/tmp/wget_1.11.4-1ubuntu1_amd64.deb" + expect(provider).to receive(:run_noninteractive).with( "dpkg -i /tmp/wget_1.11.4-1ubuntu1_amd64.deb" ) - @provider.upgrade_package("/tmp/wget_1.11.4-1ubuntu1_amd64.deb", "1.11.4-1ubuntu1") + provider.upgrade_package("/tmp/wget_1.11.4-1ubuntu1_amd64.deb", "1.11.4-1ubuntu1") end it "should run dpkg -i with the package source and options if specified" do - expect(@provider).to receive(:run_noninteractive).with( + expect(provider).to receive(:run_noninteractive).with( "dpkg -i --force-yes /tmp/wget_1.11.4-1ubuntu1_amd64.deb" ) - allow(@new_resource).to receive(:options).and_return("--force-yes") + allow(new_resource).to receive(:options).and_return("--force-yes") - @provider.install_package("wget", "1.11.4-1ubuntu1") + provider.install_package("wget", "1.11.4-1ubuntu1") end it "should upgrade by running install_package" do - expect(@provider).to receive(:install_package).with("wget", "1.11.4-1ubuntu1") - @provider.upgrade_package("wget", "1.11.4-1ubuntu1") + expect(provider).to receive(:install_package).with("wget", "1.11.4-1ubuntu1") + provider.upgrade_package("wget", "1.11.4-1ubuntu1") end end describe Chef::Provider::Package::Dpkg, "remove and purge" do it "should run dpkg -r to remove the package" do - expect(@provider).to receive(:run_noninteractive).with( + expect(provider).to receive(:run_noninteractive).with( "dpkg -r wget" ) - @provider.remove_package("wget", "1.11.4-1ubuntu1") + provider.remove_package("wget", "1.11.4-1ubuntu1") end it "should run dpkg -r to remove the package with options if specified" do - expect(@provider).to receive(:run_noninteractive).with( + expect(provider).to receive(:run_noninteractive).with( "dpkg -r --force-yes wget" ) - allow(@new_resource).to receive(:options).and_return("--force-yes") + allow(new_resource).to receive(:options).and_return("--force-yes") - @provider.remove_package("wget", "1.11.4-1ubuntu1") + provider.remove_package("wget", "1.11.4-1ubuntu1") end it "should run dpkg -P to purge the package" do - expect(@provider).to receive(:run_noninteractive).with( + expect(provider).to receive(:run_noninteractive).with( "dpkg -P wget" ) - @provider.purge_package("wget", "1.11.4-1ubuntu1") + provider.purge_package("wget", "1.11.4-1ubuntu1") end it "should run dpkg -P to purge the package with options if specified" do - expect(@provider).to receive(:run_noninteractive).with( + expect(provider).to receive(:run_noninteractive).with( "dpkg -P --force-yes wget" ) - allow(@new_resource).to receive(:options).and_return("--force-yes") + allow(new_resource).to receive(:options).and_return("--force-yes") - @provider.purge_package("wget", "1.11.4-1ubuntu1") + provider.purge_package("wget", "1.11.4-1ubuntu1") end end end |