[PATCH/puppet 6/7] (#7629) changed name to title since types with composite namevars may not have a :name parmeter.

[Dan Bode]

Signed-off-by: Dan Bode <d...@puppetlabs.com>
---
Local-branch: issue/master/7629
lib/puppet/type.rb | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index dc4764e..e62a764 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -893,8 +893,7 @@ class Type
next
end
provider_instances[instance.name] = instance
-
- new(:name => instance.name, :provider => instance, :audit => :all)
+ new(:title => namevar_join(instance.property_hash), :provider => instance, :audit => :all)
end
end.flatten.compact
end
--
1.6.5.1

[Dan Bode]

- it was previously hardcoded to use :name param
which does not work for composite namevars.

Signed-off-by: Dan Bode <d...@puppetlabs.com>
---
Local-branch: issue/master/7629

lib/puppet/provider.rb | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/puppet/provider.rb b/lib/puppet/provider.rb
index 4456feb..9cccfaf 100644
--- a/lib/puppet/provider.rb
+++ b/lib/puppet/provider.rb
@@ -258,7 +258,7 @@ class Puppet::Provider
end

def name
- if n = @property_hash[:name]
+ if n = self.class.resource_type.namevar_join(@property_hash)
return n
elsif self.resource
resource.name
--
1.6.5.1

[Dan Bode]

- previously, it was only matching based on the
name parameter. This caused a few problems.
- should not have to specify a name param
- it does not make as much sense with composite
namevars
- for composite namevars, it needs to match
records to resources based on all of the
namevars.

Signed-off-by: Dan Bode <d...@puppetlabs.com>
---
Local-branch: issue/master/7629

lib/puppet/provider/parsedfile.rb | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/lib/puppet/provider/parsedfile.rb b/lib/puppet/provider/parsedfile.rb
index 75a215f..522930e 100755
--- a/lib/puppet/provider/parsedfile.rb
+++ b/lib/puppet/provider/parsedfile.rb
@@ -193,7 +193,12 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
# Skip things like comments and blank lines
next if skip_record?(record)

- if name = record[:name] and resource = resources[name]
+ # collect all of the key attribute values from the record to create the key
+ key = resource_type.key_attributes.sort_by{ |k| k.to_s }.collect do |attr|
+ record[attr]
+ end
+ key = key.to_s if key.size == 1
+ if resource = resources[key]
resource.provider = new(record)
elsif respond_to?(:match)
if resource = match(record, matchers)
--
1.6.5.1

[Dan Bode]

- for composite namevars to support purging
and 'puppet resource' quiery all, it needs to
be possible to map from the composite namevars
to a single id.

- the basic idea is that authors of types need to
implement this method if they want to support
query all or purging.

Signed-off-by: Dan Bode <d...@puppetlabs.com>
---
Local-branch: issue/master/7629

lib/puppet/type.rb | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 1dbf124..dc4764e 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -219,6 +219,14 @@ class Type
end
end

+ def self.namevar_join(hash)
+ case key_attributes.length
+ when 0,1; hash[:name]
+ else
+ Puppet.warning('you should specify a joiner when there are two of more key attributes')
+ end
+ end
+
def uniqueness_key
self.class.key_attributes.sort_by { |attribute_name| attribute_name.to_s }.map{ |attribute_name| self[attribute_name] }
end
--
1.6.5.1

[Dan Bode]

This is an experimental set of patches that I wrote for composite namevars
to work with parsedfile and to support purging.

This is not ready to be merged, I would just like to get some input on the
following:
- does this work (for people who have pending composite namevar work)
- will this break anything? although it has passed the unit test,
it touches Puppet in scary places that I mostly, but do not fully
understand
- is the design agreeable enough for everyone?

[Dan Bode]

For types with composite namevars, hash2resource
should use namevar_join to populate tilte with
namevar.

Signed-off-by: Dan Bode <d...@puppetlabs.com>
---
Local-branch: issue/master/7629

lib/puppet/type.rb | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index e62a764..305042d 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -916,8 +916,11 @@ class Type

title = hash.delete(:title)
title ||= hash[:name]
- title ||= hash[key_attributes.first] if key_attributes.length == 1
-
+ title ||= if key_attributes.length == 1
+ hash[key_attributes.first]
+ else
+ namevar_join(hash)
+ end
raise Puppet::Error, "Title or name must be provided" unless title

# Now create our resource.
--
1.6.5.1

[Dan Bode]

- this ensures that resources are unique based on
composite namevars.
- I need to review why this is required..

Signed-off-by: Dan Bode <d...@puppetlabs.com>
---
Local-branch: issue/master/7629

lib/puppet/transaction.rb | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 3728a2f..9e40d44 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -226,7 +226,9 @@ class Puppet::Transaction
@catalog.vertices.each do |resource|
if provider = resource.provider and provider.class.respond_to?(:prefetch)
prefetchers[provider.class] ||= {}
- prefetchers[provider.class][resource.name] = resource
+ unique_key = resource.uniqueness_key
+ unique_key = unique_key.to_s if unique_key.size == 1
+ prefetchers[provider.class][unique_key] = resource
end
end

--
1.6.5.1

[Dan Bode]

- Introduction of composite namevars places
extra restrictions on resource title format
(in order to guarantee uniqueness
it should be based on all namevars)
- Puppet was not explicitly handling the case
where the title is invalid

Signed-off-by: Dan Bode <d...@puppetlabs.com>
---
Local-branch: issue/master/7629

lib/puppet/resource.rb | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lib/puppet/resource.rb b/lib/puppet/resource.rb
index 59e387d..2426ed2 100644
--- a/lib/puppet/resource.rb
+++ b/lib/puppet/resource.rb
@@ -426,6 +426,7 @@ class Puppet::Resource
return h
end
}
+ raise ArgumentError, "invalid resource #{self.to_s} does not match any title patterns"
else
return { :name => title.to_s }
end
--
1.6.5.1

[Dan Bode]

MQR - maybe title should be used for uniqueness here?
 

      end
    end

--
1.6.5.1

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to puppe...@googlegroups.com.
To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.

[Dan Bode]

On Mon, May 23, 2011 at 11:45 AM, Dan Bode <d...@puppetlabs.com> wrote:

- for composite namevars to support purging
 and 'puppet resource' quiery all, it needs to
 be possible to map from the composite namevars
 to a single id.

- the basic idea is that authors of types need to
 implement this method if they want to support
 query all or purging.

Signed-off-by: Dan Bode <d...@puppetlabs.com>
---
Local-branch: issue/master/7629
 lib/puppet/type.rb |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 1dbf124..dc4764e 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -219,6 +219,14 @@ class Type
    end
  end

+  def self.namevar_join(hash)
+    case key_attributes.length
+    when 0,1; hash[:name]

replaced with:

     case key_attributes.length
-    when 0,1; hash[:name]
+    when 0; hash[:name]
+    when 1; hash[key_attributes.first]
     else

 

+    else
+      Puppet.warning('you should specify a joiner when there are two of more key attributes')
+    end
+  end
+
  def uniqueness_key
    self.class.key_attributes.sort_by { |attribute_name| attribute_name.to_s }.map{ |attribute_name| self[attribute_name] }
  end
--
1.6.5.1

[Dan Bode]

On Mon, May 23, 2011 at 11:45 AM, Dan Bode <d...@puppetlabs.com> wrote:

For types with composite namevars, hash2resource
should use namevar_join to populate tilte with
namevar.

Signed-off-by: Dan Bode <d...@puppetlabs.com>
---
Local-branch: issue/master/7629
 lib/puppet/type.rb |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index e62a764..305042d 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -916,8 +916,11 @@ class Type

    title = hash.delete(:title)
    title ||= hash[:name]
-    title ||= hash[key_attributes.first] if key_attributes.length == 1
-
+    title ||= if key_attributes.length == 1
+      hash[key_attributes.first]
+    else
+      namevar_join(hash)
+    end

-    title = hash.delete(:title)
-    title ||= hash[:name]
-    title ||= if key_attributes.length == 1
-      hash[key_attributes.first]
-    else
-      namevar_join(hash)
-    end
+    title = hash.delete(:title) || namevar_join(hash)

 

    raise Puppet::Error, "Title or name must be provided" unless title

    # Now create our resource.
--
1.6.5.1

[Stefan Schulte]

I applied your patches to my port type and implemented the namevar_join
method:

def self.namevar_join(hash)
"#{hash[:name]}/#{hash[:protocol]}"
end

two comments:

1) resource[:name] seems to never be empty
I have the following validate method in my type:

validate do
unless @parameters[:name] and @parameters[:protocol]
raise Puppet::Error, "Attributes 'name' and 'protocol' are mandatory"
end
end

But the error is never raised when name is not set
proc{ @class.new(:protocol => :tcp) }.should raise_error(Puppet::Error)

The name is just an empty string now. I havent looked into it but you
may generate a title from the namevars (:title => '/tcp') and then use
title_patterns to generate the name from the title again?

2) puppet resource doesnt work
The integration tests pass now (so prefetching seems to work) and I also
did a few tests with the following sample manifest (setting one resource
to absent and one to present, changing portnumbers etc)

port { 'foo/tcp':
number => 10000,
ensure => absent,
}
port { 'foo/udp':
number => 11000,
ensure => present,
}

However »puppet resource mount port« does currently NOT work

# puppet resource port|grep telnet
warning: Port telnet found in both parsed and parsed; skipping the parsed version
warning: Port rtelnet found in both parsed and parsed; skipping the parsed version
warning: Port telnets found in both parsed and parsed; skipping the parsed version
warning: Port ktelnet found in both parsed and parsed; skipping the parsed version
port { 'ktelnet/tcp':
description => 'remote login a la telnet',
port { 'rtelnet/tcp':
port { 'telnet/tcp':
port { 'telnets/tcp':
description => 'telnet protocol over TLS/SSL',

expected output: same number of resources with udp as protocol

-Stefan

[Dan Bode]

:name is special (and hardcoded) in several parts of the Puppet code. Could you try using some other name that :name and see if it resolves those issues?

[Stefan Schulte]

On Mon, May 23, 2011 at 02:39:40PM -0700, Dan Bode wrote:
> :name is special (and hardcoded) in several parts of the Puppet code. Could
> you try using some other name that :name and see if it resolves those
> issues?
>

Doesnt resolve the issue. The test did pass before I applied your
patch. Here are the modified namevar_join and title_patterns methods
(replaced :name with :path)

def self.namevar_join(hash)
"#{hash[:path]}/#{hash[:protocol]}"
end

def self.title_patterns
[
# we have two title_patterns "name" and "name:protocol". We won't use
# one pattern (that will eventually set :protocol to nil) because we
# want to use a default value for :protocol. And that does only work
# when :protocol is not put in the parameter hash while initialising
[
/^(.*)\/(tcp|udp)$/, # Set name and protocol
[
# We don't need a lot of post-parsing
[ :path, lambda{|x| x} ],
[ :protocol, lambda{ |x| x.intern } ]
]
],
[
/^(.*)$/,
[
[ :path, lambda{|x| x} ]
]
]
]
end

validate do
unless @parameters[:path] and @parameters[:protocol]

raise Puppet::Error, "Attributes 'name' and 'protocol' are mandatory"
end
end

* Specifying just :path raises an error if I remove the default on
:protocol (:protocol is missing)
* Specifying just :protocol should raise an error (:path is missing) but
does not because class.new(:protocol => :tcp)[:path] is "" instead

-Stefan

[Dan Bode]

can you point me to a branch I can pull from?

[Luke Kanies]

One of the major concerns I have is that there aren't any tests for any of it.

It's a very complicatd area of functionality, and not having clarity on desired behavior in the form of tests makes it much harder to maintain over time.

Also, the majority of these patches, possibly all of them, should be squashed into one patch. The only one that might deserve separate treatment is the parsedfile patch, but I'm not even convinced about that one. With the spread of patches, it's kind of hard to see what the actual design is, and it's especially hard, regardless of the number of patches, to see what behavior we had and what this changes it to.

Can you provide a summary of that somewhere in one of the commits?

And finally, the commit subjects are a bit long, and, um, weirdly capitalized. It'd be nice to see that cleaner.

--
A computer lets you make more mistakes faster than any invention in
human history--with the possible exceptions of handguns and tequila.
-- Mitch Ratcliffe
---------------------------------------------------------------------
Luke Kanies -|- http://puppetlabs.com -|- http://about.me/lak

[Dan Bode]

On Mon, May 23, 2011 at 10:45 PM, Luke Kanies <lu...@puppetlabs.com> wrote:

On May 23, 2011, at 11:45 AM, Dan Bode wrote:

> This is an experimental set of patches that I wrote for composite namevars
> to work with parsedfile and to support purging.
>
> This is not ready to be merged, I would just like to get some input on the
> following:
>  - does this work (for people who have pending composite namevar work)
>  - will this break anything? although it has passed the unit test,
>    it touches Puppet in scary places that I mostly, but do not fully
>    understand
>  - is the design agreeable enough for everyone?

One of the major concerns I have is that there aren't any tests for any of it. 

It's a very complicatd area of functionality, and not having clarity on desired behavior in the form of tests makes it much harder to maintain over time.

as the commit message says, this is not ready to be merged. At this point, I have just hacked on the code until it supported my use cases and did a little bit of clean-up. I still have plans on writing unit tests.

As of right now, the type that I wrote this for has been my test (that as well as puppet resource) I wasn't even thinking about rather or not that type should be in Puppet. Perhaps this can provide a view into what I am trying to accomplish.

    https://github.com/bodepd/puppet-limits-ruby

I really only posted this patch, b/c I was hoping on having one or two people who had expressed interest in the functionality have a look at it. In the future, I could either send emails to those people privately or post messages to the list that point at a branch.
 

Also, the majority of these patches, possibly all of them, should be squashed into one patch.

ok
 

 The only one that might deserve separate treatment is the parsedfile patch,

probably the raise exception if no pattern matches could be its own commit as well, I can squash the other ones into a single commit.
 

but I'm not even convinced about that one.  With the spread of patches, it's kind of hard to see what the actual design is, and it's especially hard, regardless of the number of patches, to see what behavior we had and what this changes it to.

that is one thing that I am having trouble with. I can easily hack on the code for it to support my use cases,
 

Can you provide a summary of that somewhere in one of the commits?

And finally, the commit subjects are a bit long, and, um, weirdly capitalized.  It'd be nice to see that cleaner. 

--
A computer lets you make more mistakes faster than any invention in
human history--with the possible exceptions of handguns and tequila.
               -- Mitch Ratcliffe
---------------------------------------------------------------------
Luke Kanies  -|-   http://puppetlabs.com   -|-   http://about.me/lak

[Dan Bode]

HI Stefan,

I modified and merged your ports code with the latest version of my patch (which has changed a little from yesterday) comments below:

It could be that I fixed you use cases with some of my changes this morning.

I have still identified that this is causing other things in puppet (namely puppet resouce) to break, I am still looking into those.

https://github.com/bodepd/puppet/tree/mastercompositenamevar

On Mon, May 23, 2011 at 2:25 PM, Stefan Schulte <stefan....@taunusstein.net> wrote:

On Mon, May 23, 2011 at 11:45:45AM -0700, Dan Bode wrote:
> This is an experimental set of patches that I wrote for composite namevars
> to work with parsedfile and to support purging.
>
> This is not ready to be merged, I would just like to get some input on the
> following:
>   - does this work (for people who have pending composite namevar work)
>   - will this break anything? although it has passed the unit test,
>     it touches Puppet in scary places that I mostly, but do not fully
>     understand
>   - is the design agreeable enough for everyone?
>

I applied your patches to my port type and implemented the namevar_join
method:

   def self.namevar_join(hash)
     "#{hash[:name]}/#{hash[:protocol]}"
   end

two comments:

1) resource[:name] seems to never be empty
I have the following validate method in my type:

   validate do
     unless @parameters[:name] and @parameters[:protocol]
       raise Puppet::Error, "Attributes 'name' and 'protocol' are mandatory"
     end
   end

b/c of the title_pattern matching, it is not possible for :path to be empty:

         /^(.*)$/,
         [
           [ :path, lambda{|x| x} ]
         ]

 

But the error is never raised when name is not set

   proc{ @class.new(:protocol => :tcp) }.should raise_error(Puppet::Error)

how could this test ever happen in the real world?

 

The name is just an empty string now. I havent looked into it but you
may generate a title from the namevars (:title => '/tcp') and then use
title_patterns to generate the name from the title again?

not sure I follow
 

2) puppet resource doesnt work
The integration tests pass now (so prefetching seems to work) and I also
did a few tests with the following sample manifest (setting one resource
to absent and one to present, changing portnumbers etc)

   port { 'foo/tcp':
     number => 10000,
     ensure => absent,
   }
   port { 'foo/udp':
     number => 11000,
     ensure => present,
   }
 

However »puppet resource mount port« does currently NOT work

this works for me
 

# puppet resource port|grep telnet
warning: Port telnet found in both parsed and parsed; skipping the parsed version
warning: Port rtelnet found in both parsed and parsed; skipping the parsed version
warning: Port telnets found in both parsed and parsed; skipping the parsed version
warning: Port ktelnet found in both parsed and parsed; skipping the parsed version
port { 'ktelnet/tcp':
 description => 'remote login a la telnet',
port { 'rtelnet/tcp':
port { 'telnet/tcp':
port { 'telnets/tcp':
 description => 'telnet protocol over TLS/SSL',

expected output: same number of resources with udp as protocol

this worked for my tests

 

-Stefan

[Stefan Schulte]

On Tue, May 24, 2011 at 12:12:20AM +0200, Stefan Schulte wrote:
> On Mon, May 23, 2011 at 02:39:40PM -0700, Dan Bode wrote:
> > :name is special (and hardcoded) in several parts of the Puppet code. Could
> > you try using some other name that :name and see if it resolves those
> > issues?
> >
>
> Doesnt resolve the issue. The test did pass before I applied your
> patch. Here are the modified namevar_join and title_patterns methods
> (replaced :name with :path)

I have to correct myself because I havent done the renaming everywhere:
Renaming makes everything a lot worse. I guess it's because puppet still
does a lot of lookups with resource[:name]. Now in type.rb we do the following:

def [](name) # name = :name
name = attr_alias(name)

### in validattr :name is always considered to be a valid attribute ###
fail("Invalid parameter #{name}(#{name.inspect})") unless self.class.validattr?(name)

### This is always false because name_var returns false if we have more than one namevar ###
if name == :name && nv = name_var
name = nv
end

### name is still :name because we havent found a namevar ###

if obj = @parameters[name] ### this is nil
# Note that if this is a property, then the value is the "should" value,
# not the current value.
obj.value
else
return nil
end
end

Heres a branch that includes your patches (without the renaming of the
:name parameter)

https://github.com/stschulte/puppet/tree/feature/next/5660_with_7629

Just run the following specs

# rspec -c -f d spec/unit/type/port_spec.rb
# rspec -c -f d spec/unit/provider/port/parsed_spec.rb
# rspec -c -f d spec/integration/provider/port_spec.rb
# puppet resource port

-Stefan