I agree about Rein's con, actually. I was trying to preserve the
existing behavior, which was using
"string1".match("string2")
which coerces string2 into a regexp. It was unclear to me why the
original author was using "match" instead of == , but it seemed
possible that they expected string2 to be a substring of string1.
Perhaps a more readable line would be something like
+ unless /#{Regexp.escape @resource[:name]}/ =~ matches[0]
but I was hoping to make clear that I was not actually changing the
behavior of the already unintuitive line.
This was my first attempt at playing with the Puppet codebase. Thanks
for the feedback.
~
Jesse Wolfe
> 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 Henrichshttp://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
-~----------~----~----~----~------~----~------~--~---
