Please review pull request #398: Ticket/2.7.x/12106 pmt uninstall command ux improvements opened by (kelseyhightower)
Description:
Before this patch the uninstall action only uninstalled puppet modules by
name. The uninstallation of a module consists of removing a directory in
the module path that matches the name of the module. This does not take
into account the version of the module installed.
This patch changes the behaviour of the uninstall action with the
following features:
- Modules can be uninstalled by specific version
- Modules can be uninstalled by enviornment
- Output of the unistall command has been enhanced to provide a better UX
This patch also includes updated specs for the change in behaviour.
- Opened: Tue Jan 24 15:11:39 UTC 2012
- Based on: puppetlabs:2.7.x (d4cf751bc5b28361c744b12a44f80b5fa77907b2)
- Requested merge: kelseyhightower:ticket/2.7.x/12106_PMT_uninstall_command_ux_improvements (9a2897c981fe0a9983d78f61955a298437737f85)
Diff follows:
diff --git a/lib/puppet/face/module/uninstall.rb b/lib/puppet/face/module/uninstall.rb
index 19c983e..951b814 100644
--- a/lib/puppet/face/module/uninstall.rb
+++ b/lib/puppet/face/module/uninstall.rb
@@ -7,44 +7,85 @@
#{Puppet.settings[:modulepath].split(File::PATH_SEPARATOR).join(', ')}.
EOT
- returns "Array of strings representing paths of uninstalled files."
+ returns "Hash of module objects representing uninstalled modules and related errors."
examples <<-EOT
Uninstall a module from all directories in the modulepath:
$ puppet module uninstall ssh
- Removed /etc/puppet/modules/ssh
+ Removed /etc/puppet/modules/ssh (v1.0.0)
Uninstall a module from a specific directory:
- $ puppet module uninstall --target-directory /usr/share/puppet/modules ssh
- Removed /usr/share/puppet/modules/ssh
+ $ puppet module uninstall --modulepath /usr/share/puppet/modules ssh
+ Removed /usr/share/puppet/modules/ssh (v1.0.0)
+
+ Uninstall a module from a specific environment:
+
+ $ puppet module uninstall --env development
+ Removed /etc/puppet/environments/development/modules/ssh (v1.0.0)
+
+ Uninstall a specific version of a module:
+
+ $ puppet module uninstall --version 2.0.0 ssh
+ Removed /etc/puppet/modules/ssh (v2.0.0)
EOT
arguments "<name>"
- option "--target-directory=", "-t=" do
- default_to { Puppet.settings[:modulepath].split(File::PATH_SEPARATOR) }
- summary "The target directory to search from modules."
+ option "--env=" do
+ default_to { "production" }
+ summary "The target environment to search for modules."
+ description <<-EOT
+ The target environment to search for modules.
+ EOT
+ end
+
+ option "--version=" do
+ summary "The version of the module to uninstall"
+ description <<-EOT
+ The version of the module to uninstall. When using this option a module
+ that matches the specified version must be installed or an error is raised.
+ EOT
+ end
+
+ option "--modulepath=" do
+ summary "The target directory to search for modules."
description <<-EOT
The target directory to search for modules.
EOT
end
when_invoked do |name, options|
-
- if options[:target_directory].is_a?(Array)
- options[:target_directories] = options[:target_directory]
- else
- options[:target_directories] = [ options[:target_directory] ]
+ if options[:modulepath]
+ unless File.directory?(options[:modulepath])
+ raise ArgumentError, "Directory #{options[:modulepath]} does not exist"
+ end
end
- options.delete(:target_directory)
+
+ Puppet[:modulepath] = options[:modulepath] if options[:modulepath]
+ options[:name] = name
Puppet::Module::Tool::Applications::Uninstaller.run(name, options)
end
- when_rendering :console do |removed_modules|
- removed_modules.map { |path| "Removed #{path}" }.join('\n')
+ when_rendering :console do |return_value|
+ output = ''
+
+ return_value[:removed_mods].each do |mod|
+ output << "Removed #{mod.path} (v#{mod.version})\n"
+ end
+
+ return_value[:errors].map do |mod_name, errors|
+ if ! errors.empty?
+ header = "Could not uninstall module #{return_value[:options][:name]}"
+ header << " (v#{return_value[:options][:version]})" if return_value[:options][:version]
+ output << "#{header}:\n"
+ errors.map { |error| output << " #{error}\n" }
+ end
+ end
+
+ output
end
end
end
diff --git a/lib/puppet/module_tool/applications/uninstaller.rb b/lib/puppet/module_tool/applications/uninstaller.rb
index 9cd4d8b..94ff6c7 100644
--- a/lib/puppet/module_tool/applications/uninstaller.rb
+++ b/lib/puppet/module_tool/applications/uninstaller.rb
@@ -2,29 +2,55 @@ module Puppet::Module::Tool
module Applications
class Uninstaller < Application
- def initialize(name, options = {})
+ def initialize(name, options)
@name = name
- @target_directories = options[:target_directories]
- @removed_dirs = []
+ @options = options
+ @errors = Hash.new {|h, k| h[k] = []}
+ @removed_mods = []
+ @environment = Puppet::Node::Environment.new(options[:env])
end
def run
- uninstall
- Puppet.notice "#{@name} is not installed" if @removed_dirs.empty?
- @removed_dirs
+ if module_installed?
+ uninstall
+ else
+ @errors[@name] << "Module #{@name} is not installed"
+ end
+ { :removed_mods => @removed_mods, :errors => @errors, :options => @options }
end
private
+ def version_match?(mod)
+ if @options[:version]
+ mod.version == @options[:version]
+ else
+ true
+ end
+ end
+
+ def module_installed?
+ @environment.module(@name)
+ end
+
+ def has_changes?
+ Puppet::Module::Tool::Applications::Checksummer.run(@module.path)
+ end
+
def uninstall
# TODO: #11803 Check for broken dependencies before uninstalling modules.
- #
- # Search each path in the target directories for the specified module
- # and delete the directory.
- @target_directories.each do |target|
- if File.directory? target
- module_path = File.join(target, @name)
- @removed_dirs << FileUtils.rm_rf(module_path).first if File.directory?(module_path)
+ @environment.modules_by_path.each do |path, modules|
+ modules.each do |mod|
+ if mod.name == @name
+ unless version_match?(mod)
+ @errors[@name] << "Installed version of #{mod.name} (v#{mod.version}) does not match version range"
+ end
+
+ if @errors[@name].empty?
+ FileUtils.rm_rf(mod.path)
+ @removed_mods << mod
+ end
+ end
end
end
end
diff --git a/spec/unit/face/module/uninstall_spec.rb b/spec/unit/face/module/uninstall_spec.rb
index c145b1b..a238cab 100644
--- a/spec/unit/face/module/uninstall_spec.rb
+++ b/spec/unit/face/module/uninstall_spec.rb
@@ -22,9 +22,35 @@
end
end
- it "should accept the --target-directory option" do
- options[:target_directory] = "/foo/puppet/modules"
- expected_options = { :target_directories => ["/foo/puppet/modules"] }
+ it "should accept the --env option" do
+ options[:env] = "development"
+ expected_options = {
+ :env => 'development',
+ :name => 'puppetlabs-apache'
+ }
+ Puppet::Module::Tool::Applications::Uninstaller.expects(:run).with("puppetlabs-apache", expected_options).once
+ subject.uninstall("puppetlabs-apache", options)
+ end
+
+ it "should accept the --modulepath option" do
+ options[:modulepath] = "/foo/puppet/modules"
+ expected_options = {
+ :modulepath => '/foo/puppet/modules',
+ :env => 'production',
+ :name => 'puppetlabs-apache',
+ }
+ File.expects(:directory?).with("/foo/puppet/modules").returns(true)
+ Puppet::Module::Tool::Applications::Uninstaller.expects(:run).with("puppetlabs-apache", expected_options).once
+ subject.uninstall("puppetlabs-apache", options)
+ end
+
+ it "should accept the --version option" do
+ options[:version] = "1.0.0"
+ expected_options = {
+ :version => '1.0.0',
+ :env => 'production',
+ :name => 'puppetlabs-apache',
+ }
Puppet::Module::Tool::Applications::Uninstaller.expects(:run).with("puppetlabs-apache", expected_options).once
subject.uninstall("puppetlabs-apache", options)
end
@@ -35,7 +61,7 @@
its(:summary) { should =~ /uninstall.*module/im }
its(:description) { should =~ /uninstall.*module/im }
- its(:returns) { should =~ /array of strings/i }
+ its(:returns) { should =~ /hash of module objects.*/im }
its(:examples) { should_not be_empty }
%w{ license copyright summary description returns examples }.each do |doc|
diff --git a/spec/unit/module_tool/uninstaller_spec.rb b/spec/unit/module_tool/uninstaller_spec.rb
index abf2db0..c4185d3 100644
--- a/spec/unit/module_tool/uninstaller_spec.rb
+++ b/spec/unit/module_tool/uninstaller_spec.rb
@@ -1,44 +1,124 @@
require 'spec_helper'
require 'puppet/module_tool'
require 'tmpdir'
+require 'json'
describe Puppet::Module::Tool::Applications::Uninstaller do
include PuppetSpec::Files
- describe "instances" do
- let(:tmp_module_path1) { tmpdir("uninstaller_module_path1") }
- let(:tmp_module_path2) { tmpdir("uninstaller_module_path2") }
- let(:options) do
- { :target_directories => [ tmp_module_path1, tmp_module_path2 ] }
- end
+ def mkmod(name, path, metadata=nil)
+ modpath = File.join(path, name)
+ FileUtils.mkdir_p(modpath)
- it "should return an empty list if the module is not installed" do
- described_class.new('foo', options).run.should == []
+ # For some tests we need the metadata to be present, mainly
+ # when testing against specific versions of a module.
+ if metadata:
+ File.open(File.join(modpath, 'metadata.json'), 'w') do |f|
+ f.write(metadata.to_json)
+ end
end
- it "should uninstall an installed module" do
- foo_module_path = File.join(tmp_module_path1, 'foo')
- Dir.mkdir(foo_module_path)
- described_class.new('foo', options).run.should == [ foo_module_path ]
- end
+ modpath
+ end
- it "should only uninstall the requested module" do
- foo_module_path = File.join(tmp_module_path1, 'foo')
- bar_module_path = File.join(tmp_module_path1, 'bar')
- Dir.mkdir(foo_module_path)
- Dir.mkdir(bar_module_path)
- described_class.new('foo', options).run.should == [ foo_module_path ]
+ describe "the behavior of the instances" do
+
+ before do
+ @uninstaller = Puppet::Module::Tool::Applications::Uninstaller
+ FileUtils.mkdir_p(modpath1)
+ FileUtils.mkdir_p(modpath2)
+ fake_env.modulepath = [modpath1, modpath2]
end
+
+ let(:modpath1) { File.join(tmpdir("uninstaller"), "modpath1") }
+ let(:modpath2) { File.join(tmpdir("uninstaller"), "modpath2") }
+ let(:fake_env) { Puppet::Node::Environment.new('fake_env') }
+ let(:options) { {:env => "fake_env"} }
- it "should uninstall the module from all target directories" do
- foo1_module_path = File.join(tmp_module_path1, 'foo')
- foo2_module_path = File.join(tmp_module_path2, 'foo')
- Dir.mkdir(foo1_module_path)
- Dir.mkdir(foo2_module_path)
- described_class.new('foo', options).run.should == [ foo1_module_path, foo2_module_path ]
+ context "when the module is not installed" do
+ it "should return an empty list" do
+ results = @uninstaller.new('fakemod_not_installed', options).run
+ results[:removed_mods].should == []
+ end
end
- #11803
- it "should check for broken dependencies"
+ context "when the module is installed" do
+ it "should uninstall the module" do
+ foo = mkmod("foo", modpath1)
+
+ results = @uninstaller.new("foo", options).run
+ results[:removed_mods].should == [
+ Puppet::Module.new('foo', :environment => fake_env, :path => foo)
+ ]
+ end
+
+ it "should only uninstall the requested module" do
+ foo = mkmod("foo", modpath1)
+
+ results = @uninstaller.new("foo", options).run
+ results[:removed_mods].should == [
+ Puppet::Module.new("foo", :environment => fake_env, :path => foo)
+ ]
+ end
+
+ it "should uninstall the module from every path in the modpath" do
+ foo1 = mkmod('foo', modpath1)
+ foo2 = mkmod('foo', modpath2)
+
+ results = @uninstaller.new('foo', options).run
+ results[:removed_mods].length.should == 2
+ results[:removed_mods].should include(
+ Puppet::Module.new('foo', :environment => fake_env, :path => foo1),
+ Puppet::Module.new('foo', :environment => fake_env, :path => foo2)
+ )
+ end
+
+ context "when options[:version] is specified" do
+ let(:metadata) do
+ {
+ "author" => "",
+ "name" => "foo",
+ "version" => "1.0.0",
+ "source" => "http://dummyurl",
+ "license" => "Apache2"
+ }
+ end
+
+ it "should uninstall the module if the version matches" do
+ foo = mkmod('foo', modpath1, metadata)
+
+ options[:version] = "1.0.0"
+
+ results = @uninstaller.new("foo", options).run
+ results[:removed_mods].length.should == 1
+ results[:removed_mods].first.name.should == "foo"
+ results[:removed_mods].first.version.should == "1.0.0"
+ end
+
+ it "should not uninstall the module if the version does not match" do
+ foo = mkmod("foo", modpath1, metadata)
+
+ options[:version] = "2.0.0"
+
+ results = @uninstaller.new("foo", options).run
+ results[:removed_mods].should == []
+ end
+
+ context "when the module metadata is missing" do
+ it "should not uninstall the module" do
+ foo = mkmod("foo", modpath1)
+
+ options[:version] = "2.0.0"
+
+ results = @uninstaller.new("foo", options).run
+ results[:removed_mods].should == []
+ end
+ end
+ end
+
+ # This test is pending work in #11803 to which will add
+ # dependency resolution.
+ it "should check for broken dependencies"
+ end
end
end
-- 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.
