Nico --
> This is my first patch for puppet (\o/). It aims to close the ticket
> #3304.
I don't know enough about pkgin to give you feedback on that, but I
can offer some more general suggestions on the patch with regards to
the puppet process and ruby coding technique. First, for submitting
patches:
* Use spaces, not tabs, for indentation. For the next few weeks the
standard is four space indentation; with the release of Rowlf we will
be switching to the more common two space indentation.
* Submit patches with the "rake email_patches" rake task, rather than
as an attachment; inline patches are much more likely to get read and
commented on.
* Include tests with the code; we're trying to maintain a policy of
not accepting any new code without tests. If you need examples to get
you started, look in the spec directory.
If you need help on any of these things, feel free to ask. This is in
general a very helpful and supportive group.
Also, while our existing code base is a rich source of examples it is
by no means as clear or concise as it could be. Consider the
"instances" method, which is structured very similarly to many other
providers:
1. Rather than initializing the hash at the top of the loop and then
having to reinitialize it after each use, you could just initialize it
before each use, but even better
2. Rather than initializing it and then setting the values you could
just create it with the values in it like so:
hash = { :name => match[1], :version =>
match[3], :description => match[5] }
3. Since the only use is in the line following you could get rid of
the hash variable and just pass it in to the new directly as:
packages << new( { :name => match[1], :version
=> match[3], :description => match[5] })
4. If the last (in this case only) argument to a method is a hash you
can omit the {}, giving you:
packages << new( :name => match[1], :version
=> match[3], :description => match[5] )
5. Rather than explicitly performing the match and getting the match
object in a local variable, you can just use =~ and the $n variables,
giving you:
if line =~ /(\S+)(-)(.*?)(\s+)(.*)/ then
packages << new( :name => $1, :version => $3,
:description => $5 )
end
6. Rather than doing an each and adding to the packages with << for
each matching line, we could do a select (to find the lines we're
interested in) and a collect (to do the news); however this pattern is
so common that there's a built in for it (called grep); using it we
have:
packages = pkglist.grep(/(\S+)(-)(.*?)(\s+)(.*)/) { new( :name
=> $1, :version => $3, :description => $5 ) }
7. By only putting ()s around the portions of the regular expression
we're interested in, we can eliminate the gaps in the numbering:
packages = pkglist.grep(/(\S+)-(.*?)\s+(.*)/) {new( :name =>
$1, :version => $2, :description => $3 ) }
Putting it together we have:
def self.instances
packages = []
# we rely on pkg_info to know which packages are installed
cmd="pkg_info -a"
begin
Puppet.debug "Running '#{cmd}'"
execpipe(cmd) { |pkglist|
packages = pkglist.grep(/(\S+)-(.*?)\s+(.*)/) {new(
:name => $1, :version => $2, :description => $3 ) }
}
rescue Puppet::ExecutionFailure
return nil
end
return packages
end
That's half the size of the original, and could be further simplified
by doing more in the same vein. The moral: use the existing code for
examples, but feel free to improve upon, as there is plenty of room
for improvement.
-- Markus
>From 9a0b4e3daf41dcd08dd2c6034b63534978c70e5b Mon Sep 17 00:00:00 2001
From: Nicolas Szalay <[email protected]>
Date: Fri, 26 Mar 2010 10:20:18 +0100
Subject: [PATCH 4838/4838] Added support for the pkgin provider
Signed-off-by: Nicolas Szalay <[email protected]>
---
lib/puppet/provider/package/pkgin.rb | 76 ++++++++++++++++++++++++++++++++++
1 files changed, 76 insertions(+), 0 deletions(-)
create mode 100644 lib/puppet/provider/package/pkgin.rb
diff --git a/lib/puppet/provider/package/pkgin.rb
b/lib/puppet/provider/package/pkgin.rb
new file mode 100644
index 0000000..159d3cf
--- /dev/null
+++ b/lib/puppet/provider/package/pkgin.rb
@@ -0,0 +1,76 @@
+require 'puppet/provider/package'
+
+# Puppet provider for iMil's pkgin (binary pkgsrc). Based on openbsd provider
+# This is not bug free, feel free to improve
+#
+# Nicolas Szalay <[email protected]>
+#
+Puppet::Type.type(:package).provide :pkgin, :parent =>
Puppet::Provider::Package do
+ include Puppet::Util::Execution
+ desc "pkgin provider"
+
+ # add typical pkgsrc dirs to the PATH
+ ENV["PATH"] =
ENV["PATH"]+":/opt/pkg/bin:/usr/pkg/bin/opt/pkg/sbin:/usr/pkg/sbin"
+
+ commands :pkgin => "pkgin"
+
+ def self.instances
+ packages = []
+ regex = %r{(\S+)(-)(.*?)(\s+)(.*)}
+ # we rely on pkg_info to know which packages are installed
+ cmd="pkg_info -a"
+
+ begin
+ Puppet.debug "Running '%s'" % cmd
+ execpipe(cmd) { |pkglist|
+ hash={}
+ pkglist.each { |line|
+ if (match=regex.match(line)) then
+ hash[:name]=match[1]
+ hash[:version]=match[3]
+ hash[:description]=match[5]
+
+ packages << new(hash)
+ hash={}
+ end
+ }
+ }
+ rescue Puppet::ExecutionFailure
+ return nil
+ end
+
+ return packages
+ end
+
+ def install
+ should = @resource[:ensure]
+ pkgin ["-y", "in", @resource[:name] ]
+ end
+
+ def uninstall
+ pkgin ["-y", "rm", @resource[:name] ]
+ end
+
+ def query
+ hash = {}
+ hash[:ensure] = :absent
+ regex=%r{(\S+)(-)(.*?)(\s+)(.*)}
+
+ begin
+ cmd = "pkgin ls"
+ execpipe(cmd) { |process|
+ process.each { |line|
+ if match = regex.match(line)
+ if match[1] == @resource[:name]
+ Puppet.debug " %s is
present" % @resource[:name]
+ hash[:ensure] = :present
+ end
+ end
+ }
+ }
+ end
+
+ return hash
+ end
+
+end
--
1.7.0
--
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.
