Merge pull request puppetlabs#1643 from jeffweiss/20442_fix_aix_exten…

…ded_attributes

(#20442)[PE-161] fix AIX extended attributes

lib/puppet/provider/aixobject.rb

@@ -7,24 +7,20 @@ class Puppet::Provider::AixObject < Puppet::Provider
   desc "Generic AIX resource provider"
 
   # The real provider must implement these functions.
-  def lscmd(value=@resource[:name])
-    raise Puppet::Error, "Method not defined #{@resource.class.name} #{@resource.name}: #{detail}"
+  def lscmd( _value = @resource[:name] )
+    raise Puppet::Error, "Method not defined #{@resource.class.name} #{@resource.name}: Base AixObject provider doesn't implement lscmd"
   end
 
-  def lscmd(value=@resource[:name])
-    raise Puppet::Error, "Method not defined #{@resource.class.name} #{@resource.name}: #{detail}"
+  def addcmd( _extra_attrs = [] )
+    raise Puppet::Error, "Method not defined #{@resource.class.name} #{@resource.name}: Base AixObject provider doesn't implement addcmd"
   end
 
-  def addcmd(extra_attrs = [])
-    raise Puppet::Error, "Method not defined #{@resource.class.name} #{@resource.name}: #{detail}"
-  end
-
-  def modifycmd(attributes_hash)
-    raise Puppet::Error, "Method not defined #{@resource.class.name} #{@resource.name}: #{detail}"
+  def modifycmd( _attributes_hash = {} )
+    raise Puppet::Error, "Method not defined #{@resource.class.name} #{@resource.name}: Base AixObject provider doesn't implement modifycmd"
   end
 
   def deletecmd
-    raise Puppet::Error, "Method not defined #{@resource.class.name} #{@resource.name}: #{detail}"
+    raise Puppet::Error, "Method not defined #{@resource.class.name} #{@resource.name}: Base AixObject provider doesn't implement deletecmd"
   end
 
   # Valid attributes to be managed by this provider.
@@ -161,19 +157,21 @@ def hash2args(hash, mapping=self.class.attribute_mapping_to)
   def parse_attr_list(str, mapping=self.class.attribute_mapping_from)
     properties = {}
     attrs = []
-    if !str or (attrs = str.split()).empty?
+    if str.nil? or (attrs = str.split()).empty?
       return nil
     end
 
     attrs.each { |i|
       if i.include? "=" # Ignore if it does not include '='
         (key_str, val) = i.split('=')
         # Check the key
-        if !key_str or key_str.empty?
+        if key_str.nil? or key_str.empty?
           info "Empty key in string 'i'?"
           continue
         end
+        key_str.strip!
         key = key_str.to_sym
+        val.strip! if val
 
         properties = self.load_attribute(key, val, mapping, properties)
       end
@@ -193,7 +191,7 @@ def parse_attr_list(str, mapping=self.class.attribute_mapping_from)
   def parse_colon_list(str, key_list, mapping=self.class.attribute_mapping_from)
     properties = {}
     attrs = []
-    if !str or (attrs = str.split(':')).empty?
+    if str.nil? or (attrs = str.split(':')).empty?
       return nil
     end
 
@@ -227,9 +225,9 @@ def getinfo(refresh = false)
       # Execute lsuser, split all attributes and add them to a dict.
       begin
         output = execute(self.lscmd)
-        @objectinfo = self.parse_command_output(execute(self.lscmd))
+        @objectinfo = self.parse_command_output(output)
         # All attributtes without translation
-        @objectosinfo = self.parse_command_output(execute(self.lscmd), nil)
+        @objectosinfo = self.parse_command_output(output, nil)
       rescue Puppet::ExecutionFailure => detail
         # Print error if needed. FIXME: Do not check the user here.
         Puppet.debug "aix.getinfo(): Could not find #{@resource.class.name} #{@resource.name}: #{detail}"
@@ -241,10 +239,10 @@ def getinfo(refresh = false)
   # 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
+    if @objectosinfo.nil? or refresh == true
       getinfo(refresh)
     end
-    @objectosinfo
+    @objectosinfo || Hash.new
   end
 
 
@@ -290,7 +288,7 @@ def exists?
   # providers, preferably with values already filled in, not resources.
   def self.instances
     objects=[]
-    self.list_all.each { |entry|
+    list_all.each { |entry|
       objects << new(:name => entry, :ensure => :present)
     }
     objects

lib/puppet/provider/user/aix.rb

@@ -48,7 +48,7 @@
 
   # User attributes to ignore from AIX output.
   def self.attribute_ignore
-    []
+    ["name"]
   end
 
   # AIX attributes to properties mapping.
@@ -60,19 +60,20 @@ def self.attribute_ignore
   #  :to            Method to adapt puppet property to aix command value. Optional.
   #  :from          Method to adapt aix command value to puppet property. Optional
   self.attribute_mapping = [
-    #:name => :name,
     {:aix_attr => :pgrp,       :puppet_prop => :gid,
-        :to => :gid_to_attr, :from => :gid_from_attr},
+                                :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,
-        :to => :expiry_to_attr, :from => :expiry_from_attr},
+                                :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 => :attributes, :puppet_prop => :attributes},
-    {:aix_attr => :gecos,      :puppet_prop => :comment},
+    { :aix_attr => :gecos,      :puppet_prop => :comment },
   ]
 
   #--------------
@@ -140,7 +141,7 @@ def get_arguments(key, value, mapping, objectinfo)
     if key == :attributes
       raise Puppet::Error, "Attributes must be a list of pairs key=value on #{@resource.class.name}[#{@resource.name}]" \
         unless value and value.is_a? Hash
-      return value.select { |k,v| true }.map { |pair| pair.join("=") }
+      return value.map { |k,v| k.to_s.strip + "=" + v.to_s.strip}
     end
 
     super(key, value, mapping, objectinfo)
@@ -271,18 +272,30 @@ def password=(value)
     end
   end
 
+  def managed_attribute_keys(hash)
+    managed_attributes ||= @resource.original_parameters[:attributes] || hash.keys.map{|k| k.to_s}
+    managed_attributes.map {|attr| key, value = attr.split("="); key.strip.to_sym}
+  end
+
+  def should_include?(key, managed_keys)
+    !self.class.attribute_mapping_from.include?(key) and
+            !self.class.attribute_ignore.include?(key) and
+            managed_keys.include?(key)
+  end
+
   def filter_attributes(hash)
-    # Return only not managed attributtes.
-    hash.select {
-        |k,v| !self.class.attribute_mapping_from.include?(k) and
-                !self.class.attribute_ignore.include?(k)
+    # Return only managed attributtes.
+    managed_keys = managed_attribute_keys(hash)
+    results = hash.select {
+        |k,v| should_include?(k, managed_keys)
       }.inject({}) {
         |hash, array| hash[array[0]] = array[1]; hash
       }
+    results
   end
 
   def attributes
-    filter_attributes(getosinfo(refresh = false))
+    filter_attributes(getosinfo(false))
   end
 
   def attributes=(attr_hash)

spec/unit/provider/aixobject_spec.rb

@@ -0,0 +1,101 @@
+require 'spec_helper'
+require 'puppet/provider/aixobject'
+
+describe Puppet::Provider::AixObject do
+  let(:resource) do
+    Puppet::Type.type(:user).new(
+      :name   => 'test_aix_user',
+      :ensure => :present
+    )
+  end
+
+  let(:provider) do
+    provider = Puppet::Provider::AixObject.new resource
+  end
+
+  describe "base provider methods" do
+    [ :lscmd,
+      :addcmd,
+      :modifycmd,
+      :deletecmd
+    ].each do |method|
+      it "should raise an error when unimplemented method #{method} called" do
+        lambda do
+          provider.send(method)
+        end.should raise_error(Puppet::Error, /not defined/)
+      end
+    end
+  end
+
+  describe "attribute mapping methods" do
+    let(:mapping) do
+      [
+        { :aix_attr => :test_aix_property,
+          :puppet_prop => :test_puppet_property,
+          :to => :test_convert_to_aix_method,
+          :from => :test_convert_to_puppet_method
+        }
+      ]
+    end
+
+    before(:each) do
+      provider.class.attribute_mapping = mapping
+    end
+
+    describe ".attribute_mapping_to" do
+      before(:each) do
+         if provider.class.instance_variable_defined? :@attribute_mapping_to
+           provider.class.send(:remove_instance_variable, :@attribute_mapping_to)
+         end
+      end
+
+      it "should create a hash where the key is the puppet property and the value is a hash with the aix property and the conversion method" do
+        hash = provider.class.attribute_mapping_to
+        hash.should have_key :test_puppet_property
+        sub_hash = hash[:test_puppet_property]
+        sub_hash.should have_key :key
+        sub_hash.should have_key :method
+        sub_hash[:key].should == :test_aix_property
+        sub_hash[:method].should == :test_convert_to_aix_method
+      end
+
+      it "should cache results between calls" do
+        provider.class.expects(:attribute_mapping).returns(mapping).once
+        provider.class.attribute_mapping_to
+        provider.class.attribute_mapping_to
+      end
+    end
+
+    describe ".attribute_mapping_from" do
+      before(:each) do
+        if provider.class.instance_variable_defined? :@attribute_mapping_from
+          provider.class.send(:remove_instance_variable, :@attribute_mapping_from)
+        end
+      end
+
+      it "should create a hash where the key is the aix property and the value is a hash with the puppet property and the conversion method" do
+        hash = provider.class.attribute_mapping_from
+        hash.should have_key :test_aix_property
+        sub_hash = hash[:test_aix_property]
+        sub_hash.should have_key :key
+        sub_hash.should have_key :method
+        sub_hash[:key].should == :test_puppet_property
+        sub_hash[:method].should == :test_convert_to_puppet_method
+      end
+
+      it "should cache results between calls" do
+        provider.class.expects(:attribute_mapping).returns(mapping).once
+        provider.class.attribute_mapping_from
+        provider.class.attribute_mapping_from
+      end
+    end
+  end
+
+  describe "#getinfo" do
+    it "should only execute the system command once" do
+      provider.stubs(:lscmd).returns "ls"
+      provider.expects(:execute).returns("bob=frank").once
+      provider.getinfo(true)
+    end
+  end
+end

spec/unit/provider/user/aix_spec.rb

@@ -39,4 +39,93 @@
     provider_class.list_all.should == ['root', 'guest']
   end
 
+  describe "#managed_attribute_keys" do
+    let(:existing_attributes) do
+      { :account_locked => 'false',
+        :admin => 'false',
+        :login => 'true',
+        'su' => 'true'
+      }
+    end
+
+    before(:each) do
+      original_parameters = { :attributes => attribute_array }
+      @resource.stubs(:original_parameters).returns(original_parameters)
+    end
+
+    describe "invoked via manifest" do
+      let(:attribute_array) { ["rlogin=false", "login =true"] }
+
+      it "should return only the keys of the attribute key=value pair from manifest" do
+        keys = @provider.managed_attribute_keys(existing_attributes)
+        keys.should be_include(:rlogin)
+        keys.should be_include(:login)
+        keys.should_not be_include(:su)
+      end
+
+      it "should strip spaces from symbols" do
+        keys = @provider.managed_attribute_keys(existing_attributes)
+        keys.should be_include(:login)
+        keys.should_not be_include(:"login ")
+      end
+
+      it "should have the same count as that from the manifest" do
+        keys = @provider.managed_attribute_keys(existing_attributes)
+        keys.size.should == attribute_array.size
+      end
+
+      it "should convert the keys to symbols" do
+        keys = @provider.managed_attribute_keys(existing_attributes)
+        all_symbols = keys.all? {|k| k.is_a? Symbol}
+        all_symbols.should be_true
+      end
+    end
+
+    describe "invoked via RAL" do
+      let(:attribute_array) { nil }
+
+      it "should return the keys in supplied hash" do
+        keys = @provider.managed_attribute_keys(existing_attributes)
+        keys.should_not be_include(:rlogin)
+        keys.should be_include(:login)
+        keys.should be_include(:su)
+      end
+
+      it "should convert the keys to symbols" do
+        keys = @provider.managed_attribute_keys(existing_attributes)
+        all_symbols = keys.all? {|k| k.is_a? Symbol}
+        all_symbols.should be_true
+      end
+    end
+  end
+
+  describe "#should_include?" do
+    it "should exclude keys translated into something else" do
+      managed_keys = [:rlogin]
+      @provider.class.attribute_mapping_from.stubs(:include?).with(:rlogin).returns(true)
+      @provider.class.stubs(:attribute_ignore).returns([])
+      @provider.should_include?(:rlogin, managed_keys).should be_false
+    end
+
+    it "should exclude keys explicitly ignored" do
+      managed_keys = [:rlogin]
+      @provider.class.attribute_mapping_from.stubs(:include?).with(:rlogin).returns(false)
+      @provider.class.stubs(:attribute_ignore).returns([:rlogin])
+      @provider.should_include?(:rlogin, managed_keys).should be_false
+    end
+
+    it "should exclude keys not specified in manifest" do
+      managed_keys = [:su]
+      @provider.class.attribute_mapping_from.stubs(:include?).with(:rlogin).returns(false)
+      @provider.class.stubs(:attribute_ignore).returns([])
+      @provider.should_include?(:rlogin, managed_keys).should be_false
+    end
+
+    it "should include keys specified in manifest if not translated or ignored" do
+      managed_keys = [:rlogin]
+      @provider.class.attribute_mapping_from.stubs(:include?).with(:rlogin).returns(false)
+      @provider.class.stubs(:attribute_ignore).returns([])
+      @provider.should_include?(:rlogin, managed_keys).should be_true
+    end
+  end
 end