(#5432) Use AIX native commands to manage users and groups

More elegant Array|String => String conversion in AixObject.
Fixed some bugs and code/comments cleaned.

3 files changed, 100 insertions, 117 deletions

diff --git a/lib/puppet/provider/aixobject.rb b/lib/puppet/provider/aixobject.rb
index dcb3bff78..9506c67a2 100755
--- a/lib/puppet/provider/aixobject.rb
+++ b/lib/puppet/provider/aixobject.rb
@@ -1,18 +1,10 @@
 #
-# Common code for AIX providers
-#
+# Common code for AIX providers. This class implements basic structure for
+# AIX resources. 
 # Author::    Hector Rivas Gandara <keymon@gmail.com>
 #
-#  
 class Puppet::Provider::AixObject < Puppet::Provider
-  desc "User management for AIX! Users are managed with mkuser, rmuser, chuser, lsuser"
-
-  # Constants
-  # Loadable AIX I/A module for users and groups. By default we manage compat.
-  # TODO:: add a type parameter to change this
-  class << self 
-    attr_accessor :ia_module
-  end  
+  desc "Generic AIX resource provider"
 
   # The real provider must implement these functions.
   def lscmd(value=@resource[:name])
@@ -35,7 +27,6 @@ class Puppet::Provider::AixObject < Puppet::Provider
     raise Puppet::Error, "Method not defined #{@resource.class.name} #{@resource.name}: #{detail}"
   end
 
-
   # Valid attributes to be managed by this provider.
   # It is a list of hashes
   #  :aix_attr      AIX command attribute name
@@ -45,6 +36,8 @@ class Puppet::Provider::AixObject < Puppet::Provider
   class << self 
     attr_accessor :attribute_mapping
   end
+
+  # Mapping from Puppet property to AIX attribute.
   def self.attribute_mapping_to
     if ! @attribute_mapping_to
       @attribute_mapping_to = {}
@@ -57,6 +50,8 @@ class Puppet::Provider::AixObject < Puppet::Provider
     end
     @attribute_mapping_to
   end    
+
+  # Mapping from AIX attribute to Puppet property.
   def self.attribute_mapping_from
     if ! @attribute_mapping_from
       @attribute_mapping_from = {}
@@ -73,7 +68,7 @@ class Puppet::Provider::AixObject < Puppet::Provider
   # This functions translates a key and value using the given mapping.
   # Mapping can be nil (no translation) or a hash with this format
   # {:key => new_key, :method => translate_method}
-  # It returns a list [key, value]
+  # It returns a list with the pair [key, value]
   def translate_attr(key, value, mapping)
     return [key, value] unless mapping
     return nil unless mapping[key]
@@ -85,35 +80,12 @@ class Puppet::Provider::AixObject < Puppet::Provider
     end
     [mapping[key][:key], new_value]
   end
-
-  # Gets the given command line argument for the given key, value and mapping.
-  def get_arg(key, value, mapping)
-    arg = nil
-    if ret = self.translate_attr(key, val, mapping)
-      new_key = ret[0]
-      new_val = ret[1]
-      
-      # Arrays are separated by commas
-      if new_val.is_a? Array
-        value = new_val.join(",")
-      else
-        value = new_val.to_s
-      end
-      
-      # Get the needed argument
-      if mapping[key][:to_arg]
-        arg = method(mapping[key][:to_arg]).call(new_key, value)
-      else
-        arg = (new_key.to_s + "=" + value )
-      end
-    end
-    return arg
-  end
-  
   
-  # Reads and attribute.
-  # Here we implement the default behaviour.
-  # Subclasses must reimplement this.
+  # Loads an AIX attribute (key=value) and stores it in the given hash with
+  # puppet semantics. It translates the pair using the given mapping.
+  #
+  # This operation works with each property one by one,
+  # subclasses must reimplement this if more complex operations are needed
   def load_attribute(key, value, mapping, objectinfo)
     if mapping.nil?
       objectinfo[key] = value
@@ -129,6 +101,15 @@ class Puppet::Provider::AixObject < Puppet::Provider
     return objectinfo
   end
   
+  # Gets the given command line argument for the given key and value,
+  # using the given mapping to translate key and value.
+  # All the objectinfo hash (@resource or @property_hash) is passed.
+  #
+  # This operation works with each property one by one,
+  # and default behaviour is return the arguments as key=value pairs.
+  # Subclasses must reimplement this if more complex operations/arguments
+  # are needed
+  # 
   def get_arguments(key, value, mapping, objectinfo)
     if mapping.nil?
       new_key = key
@@ -146,11 +127,7 @@ class Puppet::Provider::AixObject < Puppet::Provider
     end
 
     # convert it to string
-    if new_value.is_a? Array
-      new_value = new_value.join(",")
-    else
-      new_value = new_value.to_s
-    end
+    new_value = Array(new_value).join(',')
 
     if new_key
       return [ "#{new_key}=#{new_value}" ] 
@@ -159,9 +136,10 @@ class Puppet::Provider::AixObject < Puppet::Provider
     end
   end
   
-  # Convert the provider properties to AIX command arguments (string)
-  # This function will translate each value/key and generate the argument.
-  # By default, arguments are created as aix_key=aix_value
+  # Convert the provider properties (hash) to AIX command arguments
+  # (list of strings)
+  # This function will translate each value/key and generate the argument using
+  # the get_arguments function.
   def hash2args(hash, mapping=self.class.attribute_mapping_to)
     return "" unless hash 
     arg_list = []
@@ -171,12 +149,14 @@ class Puppet::Provider::AixObject < Puppet::Provider
     arg_list
   end
 
-  # Parse AIX command attributes in a format of space separated of key=value
-  # pairs: "uid=100 groups=a,b,c"
-  # It returns and return provider hash.
+  # Parse AIX command attributes from the output of an AIX command, that
+  # which format is a list of space separated of key=value pairs:
+  # "uid=100 groups=a,b,c". 
+  # It returns an hash. 
   #
   # If a mapping is provided, the keys are translated as defined in the
-  # mapping hash. Only values included in mapping will be added
+  # mapping hash. And only values included in mapping will be added
+  #
   # NOTE: it will ignore the items not including '='
   def parse_attr_list(str, mapping=self.class.attribute_mapping_from)
     properties = {}
@@ -201,17 +181,15 @@ class Puppet::Provider::AixObject < Puppet::Provider
     properties.empty? ? nil : properties
   end
 
-  # Parse AIX colon separated list of attributes, using given list of keys
-  # to name the attributes. This function is useful to parse the output
-  # of commands like lsfs -c:
+  # Parse AIX command output in a colon separated list of attributes,
+  # This function is useful to parse the output of commands like lsfs -c:
   #   #MountPoint:Device:Vfs:Nodename:Type:Size:Options:AutoMount:Acct
   #   /:/dev/hd4:jfs2::bootfs:557056:rw:yes:no
   #   /home:/dev/hd1:jfs2:::2129920:rw:yes:no
   #   /usr:/dev/hd2:jfs2::bootfs:9797632:rw:yes:no
   #
   # If a mapping is provided, the keys are translated as defined in the
-  # mapping hash. Only values included in mapping will be added
-  # NOTE: it will ignore the items not including '='
+  # mapping hash. And only values included in mapping will be added
   def parse_colon_list(str, key_list, mapping=self.class.attribute_mapping_from)
     properties = {}
     attrs = []
@@ -227,8 +205,8 @@ class Puppet::Provider::AixObject < Puppet::Provider
     
   end
   
-  # Default parsing function for colon separated list or attributte list
-  # (key=val pairs). It will choose the method depending of the first line.
+  # Default parsing function for AIX commands.
+  # It will choose the method depending of the first line.
   # For the colon separated list it will:
   #  1. Get keys from first line.
   #  2. Parse next line.
@@ -242,7 +220,9 @@ class Puppet::Provider::AixObject < Puppet::Provider
     end
   end
 
-  # Retrieve what we can about our object
+  # Retrieve all the information of an existing resource.
+  # It will execute 'lscmd' command and parse the output, using the mapping
+  # 'attribute_mapping_from' to translate the keys and values.
   def getinfo(refresh = false)
     if @objectinfo.nil? or refresh == true
       # Execute lsuser, split all attributes and add them to a dict.
@@ -259,7 +239,8 @@ class Puppet::Provider::AixObject < Puppet::Provider
     @objectinfo
   end
 
-  # Retrieve what we can about our object, without translate the values.
+  # Like getinfo, but it will not use the mapping to translate the keys and values.
+  # It might be usefult to retrieve some raw information.
   def getosinfo(refresh = false)
     if @objectosinfo .nil? or refresh == true
       getinfo(refresh)
@@ -269,7 +250,8 @@ class Puppet::Provider::AixObject < Puppet::Provider
 
 
   # List all elements of given type. It works for colon separated commands and
-  # list commands. 
+  # list commands.
+  # It returns a list of names.
   def list_all
     names = []
     begin
@@ -301,6 +283,17 @@ class Puppet::Provider::AixObject < Puppet::Provider
     !!getinfo(true) # !! => converts to bool
   end
 
+  # Return all existing instances  
+  # The method for returning a list of provider instances.  Note that it returns
+  # providers, preferably with values already filled in, not resources.
+  def self.instances
+    objects=[]
+    self.list_all().each { |entry|
+      objects << new(:name => entry, :ensure => :present)
+    }
+    objects
+  end
+
   #- **ensure**
   #    The basic state that the object should be in.  Valid values are
   #    `present`, `absent`, `role`.
@@ -313,17 +306,7 @@ class Puppet::Provider::AixObject < Puppet::Provider
     end
   end
 
-  # Return all existing instances  
-  # The method for returning a list of provider instances.  Note that it returns
-  # providers, preferably with values already filled in, not resources.
-  def self.instances
-    objects=[]
-    self.list_all().each { |entry|
-      objects << new(:name => entry, :ensure => :present)
-    }
-    objects
-  end
-
+  # Create a new instance of the resource
   def create
     if exists?
       info "already exists"
@@ -338,6 +321,7 @@ class Puppet::Provider::AixObject < Puppet::Provider
     end
   end 
 
+  # Delete this instance of the resource
   def delete
     unless exists?
       info "already absent"
@@ -353,8 +337,8 @@ class Puppet::Provider::AixObject < Puppet::Provider
   end
 
   #--------------------------------
-  # Call this method when the object is initialized, 
-  # create getter/setter methods for each property our resource type supports.
+  # Call this method when the object is initialized.
+  # It creates getter/setter methods for each property our resource type supports.
   # If setter or getter already defined it will not be overwritten
   def self.mk_resource_methods
     [resource_type.validproperties, resource_type.parameters].flatten.each do |prop|
@@ -363,8 +347,7 @@ class Puppet::Provider::AixObject < Puppet::Provider
       define_method(prop.to_s + "=") { |*vals| set(prop, *vals) } unless public_method_defined?(prop.to_s + "=")
     end
   end
-  #
-
+  
   # Define the needed getters and setters as soon as we know the resource type
   def self.resource_type=(resource_type)
     super
@@ -404,8 +387,7 @@ class Puppet::Provider::AixObject < Puppet::Provider
   def initialize(resource)
     super
     @objectinfo = nil
-    # FIXME: Initiallize this properly.
-    self.class.ia_module="compat"
+    @objectosinfo = nil
   end  
 
 end
diff --git a/lib/puppet/provider/group/aix.rb b/lib/puppet/provider/group/aix.rb
index ceb889840..ecdef6070 100755
--- a/lib/puppet/provider/group/aix.rb
+++ b/lib/puppet/provider/group/aix.rb
@@ -9,7 +9,7 @@ require 'puppet/provider/aixobject'
 Puppet::Type.type(:group).provide :aix, :parent => Puppet::Provider::AixObject do
   desc "Group management for AIX! Users are managed with mkgroup, rmgroup, lsgroup, chgroup"
 
-    # This will the the default provider for this platform
+  # This will the the default provider for this platform
   defaultfor :operatingsystem => :aix
   confine :operatingsystem => :aix
 
@@ -35,17 +35,19 @@ Puppet::Type.type(:group).provide :aix, :parent => Puppet::Provider::AixObject d
   #  :aix_attr      AIX command attribute name
   #  :puppet_prop   Puppet propertie name
   #  :to            Method to adapt puppet property to aix command value. Optional.
-  #  :from            Method to adapt aix command value to puppet property. Optional
+  #  :from          Method to adapt aix command value to puppet property. Optional
   self.attribute_mapping = [
     #:name => :name,
     {:aix_attr => :id,       :puppet_prop => :gid },
     {:aix_attr => :users,    :puppet_prop => :members,
       :from => :users_from_attr},
+    {:aix_attr => :attributes, :puppet_prop => :attributes},
   ]
   
   #--------------
-  # Command lines
+  # Command definition
   
+  # Return the IA module arguments based on the resource param ia_load_module
   def get_ia_module_args
     if @resource[:ia_load_module]
       ["-R", @resource[:ia_load_module].to_s]
@@ -54,7 +56,6 @@ Puppet::Type.type(:group).provide :aix, :parent => Puppet::Provider::AixObject d
     end
   end
   
-  
   def lscmd(value=@resource[:name])
     [self.class.command(:list)] +
       self.get_ia_module_args +
@@ -76,12 +77,8 @@ Puppet::Type.type(:group).provide :aix, :parent => Puppet::Provider::AixObject d
       extra_attrs + [@resource[:name]]
   end
 
-  def modifycmd(hash = property_hash, translate=true)
-    if translate
-      args = self.hash2args(hash)
-    else 
-      args = self.hash2args(hash, nil)
-    end
+  def modifycmd(hash = property_hash)
+    args = self.hash2args(hash)
     return nil if args.empty?
     
     [self.class.command(:modify)] +
@@ -96,6 +93,8 @@ Puppet::Type.type(:group).provide :aix, :parent => Puppet::Provider::AixObject d
   end
 
 
+  #--------------
+  # Overwrite get_arguments to add the attributes arguments
   def get_arguments(key, value, mapping, objectinfo)
     # In the case of attributes, return a list of key=vlaue
     if key == :attributes
@@ -106,11 +105,6 @@ Puppet::Type.type(:group).provide :aix, :parent => Puppet::Provider::AixObject d
     super(key, value, mapping, objectinfo)
   end
 
-  # Force convert users it a list.
-  def users_from_attr(value)
-    (value.is_a? String) ? value.split(',') : value
-  end
-
   def filter_attributes(hash)
     # Return only not managed attributtes.
     hash.select {
@@ -128,7 +122,7 @@ Puppet::Type.type(:group).provide :aix, :parent => Puppet::Provider::AixObject d
   def attributes=(attr_hash)
     #self.class.validate(param, value)
     param = :attributes
-    cmd = modifycmd({param => filter_attributes(attr_hash)}, false)
+    cmd = modifycmd({param => filter_attributes(attr_hash)})
     if cmd 
       begin
         execute(cmd)
@@ -138,5 +132,10 @@ Puppet::Type.type(:group).provide :aix, :parent => Puppet::Provider::AixObject d
     end
   end
 
+  # Force convert users it a list.
+  def users_from_attr(value)
+    (value.is_a? String) ? value.split(',') : value
+  end
+
 
 end
diff --git a/lib/puppet/provider/user/aix.rb b/lib/puppet/provider/user/aix.rb
index c231da816..032d2b536 100755
--- a/lib/puppet/provider/user/aix.rb
+++ b/lib/puppet/provider/user/aix.rb
@@ -1,12 +1,11 @@
 #
-# User Puppet provider for AIX. It uses standar commands to manage users:
+# User Puppet provider for AIX. It uses standard commands to manage users:
 #  mkuser, rmuser, lsuser, chuser
 #
 # Notes:
 # - AIX users can have expiry date defined with minute granularity,
 #   but puppet does not allow it. There is a ticket open for that (#5431)
 # - AIX maximum password age is in WEEKs, not days
-# - I force the compat IA module. 
 #
 # See  http://projects.puppetlabs.com/projects/puppet/wiki/Development_Provider_Development
 # for more information
@@ -47,7 +46,7 @@ Puppet::Type.type(:user).provide :aix, :parent => Puppet::Provider::AixObject do
   #  value !~ /\s/
   #end
 
-  # User attributes to ignore
+  # User attributes to ignore from AIX output.
   def self.attribute_ignore
     []
   end
@@ -59,23 +58,26 @@ Puppet::Type.type(:user).provide :aix, :parent => Puppet::Provider::AixObject do
   #  :aix_attr      AIX command attribute name
   #  :puppet_prop   Puppet propertie name
   #  :to            Method to adapt puppet property to aix command value. Optional.
-  #  :from            Method to adapt aix command value to puppet property. Optional
+  #  :from          Method to adapt aix command value to puppet property. Optional
   self.attribute_mapping = [
     #:name => :name,
-    {:aix_attr => :pgrp,     :puppet_prop => :gid,
+    {:aix_attr => :pgrp,       :puppet_prop => :gid,
         :to => :gid_to_attr, :from => :gid_from_attr},
-    {:aix_attr => :id,       :puppet_prop => :uid},
-    {:aix_attr => :groups,   :puppet_prop => :groups},
-    {:aix_attr => :home,     :puppet_prop => :home},
-    {:aix_attr => :shell,    :puppet_prop => :shell},
-    {:aix_attr => :expires,  :puppet_prop => :expiry,
+    {:aix_attr => :id,         :puppet_prop => :uid},
+    {:aix_attr => :groups,     :puppet_prop => :groups},
+    {:aix_attr => :home,       :puppet_prop => :home},
+    {:aix_attr => :shell,      :puppet_prop => :shell},
+    {:aix_attr => :expires,    :puppet_prop => :expiry,
         :to => :expiry_to_attr, :from => :expiry_from_attr},
-    {:aix_attr => :maxage,   :puppet_prop => :password_max_age},
-    {:aix_attr => :minage,   :puppet_prop => :password_min_age},
+    {:aix_attr => :maxage,     :puppet_prop => :password_max_age},
+    {:aix_attr => :minage,     :puppet_prop => :password_min_age},
+    {:aix_attr => :attributes, :puppet_prop => :attributes},
   ]
   
   #--------------
-  # Command lines
+  # Command definition
+  
+  # Return the IA module arguments based on the resource param ia_load_module
   def get_ia_module_args
     if @resource[:ia_load_module]
       ["-R", @resource[:ia_load_module].to_s]
@@ -84,6 +86,7 @@ Puppet::Type.type(:user).provide :aix, :parent => Puppet::Provider::AixObject do
     end
   end
   
+  # List groups and Ids
   def lsgroupscmd(value=@resource[:name])
     [command(:lsgroup)] +
       self.get_ia_module_args +
@@ -108,13 +111,10 @@ Puppet::Type.type(:user).provide :aix, :parent => Puppet::Provider::AixObject do
       extra_attrs + [@resource[:name]]
   end
 
-  # Get modify command
-  def modifycmd(hash = property_hash, translate=true)
-    if translate
-      args = self.hash2args(hash)
-    else 
-      args = self.hash2args(hash, nil)
-    end
+  # Get modify command. Set translate=false if no mapping must be used.
+  # Needed for special properties like "attributes"
+  def modifycmd(hash = property_hash)
+    args = self.hash2args(hash)
     return nil if args.empty?
     
     [self.class.command(:modify)] + self.get_ia_module_args +
@@ -141,6 +141,7 @@ Puppet::Type.type(:user).provide :aix, :parent => Puppet::Provider::AixObject do
         unless value and value.is_a? Hash
       return value.select { |k,v| true }.map { |pair| pair.join("=") }
     end
+        
     super(key, value, mapping, objectinfo)
   end
   
@@ -179,6 +180,7 @@ Puppet::Type.type(:user).provide :aix, :parent => Puppet::Provider::AixObject do
     verify_group(value)
   end
 
+  # Get the group gid from its name
   def gid_from_attr(value)
     groupid_by_name(value)
   end
@@ -281,7 +283,7 @@ Puppet::Type.type(:user).provide :aix, :parent => Puppet::Provider::AixObject do
   def attributes=(attr_hash)
     #self.class.validate(param, value)
     param = :attributes
-    cmd = modifycmd({param => filter_attributes(attr_hash)}, false)
+    cmd = modifycmd({param => filter_attributes(attr_hash)})
     if cmd 
       begin
         execute(cmd)