+1
Pros:
+ Good tests
+ Issue has been traced to the multiple systems it affects and fixed in each
+ Temptation towards refactorbation in the bug fix has been avoided
(even though the code is certainly asking for it)
Cons:
Only a minor one. In the following code
output = dpkg_deb "--show", @resource[:source]
matches = /^(\S+)\t(\S+)$/.match(output).captures
- unless matches[0].match(@resource[:name])
+ unless matches[0].match( Regexp.escape(@resource[:name]) )
warning "source doesn't contain named package, but %s" % matches[0]
end
the patched line matches the unescaped string against the escaped
resource name, which initially appears incorrect but does actually
work properly because of the way Ruby handles str.match(other_str).
This was unintuitive.
I wouldn't object to a follow-up patch that handles the code
duplication with a targeted refactoring, but this patch is quite
adequate.
On Thu, Oct 8, 2009 at 12:06 PM, Jesse Wolfe <[email protected]> wrote:
>
> When the manifest has:
> package { "gcc-c++": ensure => latest }
> An exception is raised:
> SyntaxError: nested *?+ in regexp
>
> This affects several providers:
> dpkg, rug, up2date, urpmi
>
> Fixed by escaping the package names with Regexp.escape
>
> Signed-off-by: Jesse Wolfe <[email protected]>
> ---
> lib/puppet/provider/package/dpkg.rb | 2 +-
> lib/puppet/provider/package/rug.rb | 2 +-
> lib/puppet/provider/package/up2date.rb | 2 +-
> lib/puppet/provider/package/urpmi.rb | 2 +-
> spec/unit/provider/package/dpkg.rb | 7 +++++++
> spec/unit/provider/package/rug.rb | 25 +++++++++++++++++++++++++
> spec/unit/provider/package/up2date.rb | 25 +++++++++++++++++++++++++
> spec/unit/provider/package/urpmi.rb | 25 +++++++++++++++++++++++++
> 8 files changed, 86 insertions(+), 4 deletions(-)
> create mode 100755 spec/unit/provider/package/rug.rb
> create mode 100755 spec/unit/provider/package/up2date.rb
> create mode 100755 spec/unit/provider/package/urpmi.rb
>
> diff --git a/lib/puppet/provider/package/dpkg.rb
> b/lib/puppet/provider/package/dpkg.rb
> index a4c3982..d6ec56a 100755
> --- a/lib/puppet/provider/package/dpkg.rb
> +++ b/lib/puppet/provider/package/dpkg.rb
> @@ -83,7 +83,7 @@ Puppet::Type.type(:package).provide :dpkg, :parent =>
> Puppet::Provider::Package
> def latest
> output = dpkg_deb "--show", @resource[:source]
> matches = /^(\S+)\t(\S+)$/.match(output).captures
> - unless matches[0].match(@resource[:name])
> + unless matches[0].match( Regexp.escape(@resource[:name]) )
> warning "source doesn't contain named package, but %s" %
> matches[0]
> end
> matches[1]
> diff --git a/lib/puppet/provider/package/rug.rb
> b/lib/puppet/provider/package/rug.rb
> index b68ec30..ca27cbb 100644
> --- a/lib/puppet/provider/package/rug.rb
> +++ b/lib/puppet/provider/package/rug.rb
> @@ -36,7 +36,7 @@ Puppet::Type.type(:package).provide :rug, :parent => :rpm do
> #rug can only get a list of *all* available packages?
> output = rug "list-updates"
>
> - if output =~ /#...@resource[:name]}\s*\|\s*([0-9\.\-]+)/
> + if output =~ /#{Regexp.escape @resource[:name]}\s*\|\s*([0-9\.\-]+)/
> return $1
> else
> # rug didn't find updates, pretend the current
> diff --git a/lib/puppet/provider/package/up2date.rb
> b/lib/puppet/provider/package/up2date.rb
> index d8a1265..284bbaf 100644
> --- a/lib/puppet/provider/package/up2date.rb
> +++ b/lib/puppet/provider/package/up2date.rb
> @@ -25,7 +25,7 @@ Puppet::Type.type(:package).provide :up2date, :parent =>
> :rpm, :source => :rpm d
> #up2date can only get a list of *all* available packages?
> output = up2date "--showall"
>
> - if output =~ /^...@resource[:name]}-(\d+.*)\.\w+/
> + if output =~ /^#{Regexp.escape @resource[:name]}-(\d+.*)\.\w+/
> return $1
> else
> # up2date didn't find updates, pretend the current
> diff --git a/lib/puppet/provider/package/urpmi.rb
> b/lib/puppet/provider/package/urpmi.rb
> index a958352..a79e962 100644
> --- a/lib/puppet/provider/package/urpmi.rb
> +++ b/lib/puppet/provider/package/urpmi.rb
> @@ -43,7 +43,7 @@ Puppet::Type.type(:package).provide :urpmi, :parent =>
> :rpm, :source => :rpm do
> def latest
> output = urpmq "-S", @resource[:name]
>
> - if output =~ /^...@resource[:name]}\s+:\s+.*\(\s+(\S+)\s+\)/
> + if output =~ /^#{Regexp.escape
> @resource[:name]}\s+:\s+.*\(\s+(\S+)\s+\)/
> return $1
> else
> # urpmi didn't find updates, pretend the current
> diff --git a/spec/unit/provider/package/dpkg.rb
> b/spec/unit/provider/package/dpkg.rb
> index 08aaca8..0c5c9b5 100755
> --- a/spec/unit/provider/package/dpkg.rb
> +++ b/spec/unit/provider/package/dpkg.rb
> @@ -149,6 +149,13 @@ describe provider do
> @provider.expects(:warning)
> @provider.latest
> end
> +
> + it "should cope with names containing ++" do
> + �...@resource = stub 'resource', :[] => "asdf++"
> + �...@provider = provider.new(@resource)
> + �[email protected](:dpkg_deb).returns "asdf++\t1.0"
> + �[email protected] == "1.0"
> + end
> end
>
> it "should use 'dpkg -r' to uninstall" do
> diff --git a/spec/unit/provider/package/rug.rb
> b/spec/unit/provider/package/rug.rb
> new file mode 100755
> index 0000000..3181e43
> --- /dev/null
> +++ b/spec/unit/provider/package/rug.rb
> @@ -0,0 +1,25 @@
> +#!/usr/bin/env ruby
> +
> +Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ?
> require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/spec_helper.rb") }
> +
> +provider = Puppet::Type.type(:package).provider(:rug)
> +
> +describe provider do
> + before do
> + �...@resource = stub 'resource', :[] => "asdf"
> + �...@provider = provider.new(@resource)
> + end
> +
> +
> + describe "when determining latest available version" do
> +
> + it "should cope with names containing ++" do
> + �...@resource = stub 'resource', :[] => "asdf++"
> + �...@provider = provider.new(@resource)
> + �[email protected](:rug).returns "asdf++ | 1.0"
> + �[email protected] == "1.0"
> + end
> +
> + end
> +
> +end
> diff --git a/spec/unit/provider/package/up2date.rb
> b/spec/unit/provider/package/up2date.rb
> new file mode 100755
> index 0000000..ec4bc67
> --- /dev/null
> +++ b/spec/unit/provider/package/up2date.rb
> @@ -0,0 +1,25 @@
> +#!/usr/bin/env ruby
> +
> +Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ?
> require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/spec_helper.rb") }
> +
> +provider = Puppet::Type.type(:package).provider(:up2date)
> +
> +describe provider do
> + before do
> + �...@resource = stub 'resource', :[] => "asdf"
> + �...@provider = provider.new(@resource)
> + end
> +
> +
> + describe "when determining latest available version" do
> +
> + it "should cope with names containing ++" do
> + �...@resource = stub 'resource', :[] => "asdf++"
> + �...@provider = provider.new(@resource)
> + �[email protected](:up2date).returns "asdf++-1.0.rpm"
> + �[email protected] == "1.0"
> + end
> +
> + end
> +
> +end
> diff --git a/spec/unit/provider/package/urpmi.rb
> b/spec/unit/provider/package/urpmi.rb
> new file mode 100755
> index 0000000..050ed63
> --- /dev/null
> +++ b/spec/unit/provider/package/urpmi.rb
> @@ -0,0 +1,25 @@
> +#!/usr/bin/env ruby
> +
> +Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ?
> require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/spec_helper.rb") }
> +
> +provider = Puppet::Type.type(:package).provider(:urpmi)
> +
> +describe provider do
> + before do
> + �...@resource = stub 'resource', :[] => "asdf"
> + �...@provider = provider.new(@resource)
> + end
> +
> +
> + describe "when determining latest available version" do
> +
> + it "should cope with names containing ++" do
> + �...@resource = stub 'resource', :[] => "asdf++"
> + �...@provider = provider.new(@resource)
> + �[email protected](:urpmq).returns "asdf++ : whatever ( 1.0 )"
> + �[email protected] == "1.0"
> + end
> +
> + end
> +
> +end
> --
> 1.6.3.3
>
>
> >
>
--
Rein Henrichs
http://reductivelabs.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
-~----------~----~----~----~------~----~------~--~---
