The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Refactor #5439

Puppet::Property::KeyValue should be more generic

Added by Stefan Schulte almost 4 years ago. Updated about 3 years ago.

Status:AcceptedStart date:12/01/2010
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-
Affected Puppet version: Branch:
Keywords:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This ticket may be automatically exported to the PUP project on JIRA using the button below:


Description

In my opinion puppet should provide a small set of good property classes that can be used by anyone who wants to write a custom type. While some types define their own classes (take a look in zone.rb), puppet/property looks like a good place for general ones. Unfortunately KeyValue has some really specific behaviour (its only used for the user_role_add to store attributes for /etc/user_attr).

Where to use KeyValue? Have a look at /etc/user_attr. This is one example line (taken from http://docs.sun.com/app/docs/doc/816-0219/6m6njqbd1?a=view)

root::::auths=solaris.*,solaris.grant;profiles=All;type=normal

Field separator is ‘:’ and all the keyvalue pairs in the last field is perfect for KeyValue. You can represent this last field with a hash attributes => {:auths => 'solaris.*,solaris.grant' , :profiles => 'All', :type => 'normal' }

KeyValue now also allows us to use minimal membership: If I want attributes => {:type => ‘role’}, puppet will change the type while leaving all the other attributes in tact (merging what the provider retrieves as IS and what the user defined as SHOULD)

So far I explained why I like the KeyValue. What I dont like is, its weired behaviour: When you query the should-value you destory your current value. Let’s say my provider retrieves {:key1 => 'value1' } as the is-value, and the user specified 'key2=value2' which translates to {:key2 => 'value2'}. When I call should on this property, I expect it to be {:key2 => 'value2'} if membership is set to inclusive and I expect {:key1 => 'value1', :key2 => 'value2'} if membership is set to minimum, But if you have membership inclusive the result will be {:key1 => nil, :key2 => 'value2'} AND your is-value is now {:key1 => nil} (if your provider doesnt duplicates the hash in attributes()). The modification on the original Hashobject is done in property/keyvalue.rb:process_current_hash. This behaviour is not obvious and I think incorrect in most cases.

I think the reason of setting every key to => nil that appears in IS but not in SHOULD is there because in user_role_add.rb the author tries to modify /etc/user_attr with usermod and usermod has no function to delete a key. For a few attributes it works when you pass “key=” to delete the key. (It’s not working for projects by the way, usermod -K project= user throws an error) so I tried to build a parsedfile provider for /etc/user_attr. But now I’m getting a hash that doesnt represent my real should)

Long story short: Puppet::Property::KeyValue Function process_current_hash should return an empty Hash if membership is set to :inclusive and if user_role_add depends on the current behaviour it should subclass and overwrite this method.

History

#1 Updated by James Turnbull over 3 years ago

  • Status changed from Unreviewed to Needs Decision
  • Assignee set to Nigel Kersten

#2 Updated by Nigel Kersten about 3 years ago

  • Status changed from Needs Decision to Accepted
  • Assignee deleted (Nigel Kersten)

Also available in: Atom PDF