composite namevars (part 1) - purging

[Dan Bode]

Hi all,

I have been working on getting composite namevars to work with parsedfile.

It actually seems to get a little sticky for purging, below I am outlining how it works, I was just curious if anyone has spent time thinking about this use case before and how it should work:

As far as I can tell, the title_patterns can only map in one direction title->parameters

(more on why they may have to map the other way in a second :) ).

When we call self.instances during purging, the following series of events occur (with parsed file)

  - generate each record
        - this is able to generate a record that includes the params that compose the composite namevar
  - this record (which is a hash) is then converted into a resource
  - when converted into a resources, it calls hash2resource, which picks a title if one has not been specified:

   910      title = hash.delete(:title)
   911      title ||= hash[:name]
   912      title ||= hash[key_attributes.first] if key_attributes.length == 1

   - it is impossible for this code to generate a valid title for a composite title (since it doesn't know how to map backwards from the params out of the has to the composite title)

now that we have a title that is invalid

  - we call to_hash when we generate the original parameters

@original_parameters = resource.to_hash

   - to_hash tries to parse the title (using def parse_title), which leads to failures b/c the title is invalid

I can get around this by adding the following pattern:

      # this pattern is required for purging
      [ /^(.*)$/,
        [
          [ :name, lambda{|x| x} ]
        ]
      ]

which is added just for purging

[markus]

> It actually seems to get a little sticky for purging, below I am
> outlining how it works, I was just curious if anyone has spent time
> thinking about this use case before and how it should work:
>
> As far as I can tell, the title_patterns can only map in one direction
> title->parameters

That's correct. Since it's made up of two non-invertible operations
(matching a regular expression & mapping through lambdas) there's no
automatically generated inverse operation. We (Stephan and I) discussed
providing support for a converse operation (something like a micro-erb
template that would produce a title based on the other attributes) but
since we couldn't come up with compelling use case at the time we
decided it was out of scope.

> (more on why they may have to map the other way in a second :) ).
>
> When we call self.instances during purging, the following series of
> events occur (with parsed file)
>
> - generate each record
> - this is able to generate a record that includes the params
> that compose the composite namevar
> - this record (which is a hash) is then converted into a resource
> - when converted into a resources, it calls hash2resource, which
> picks a title if one has not been specified:
>
> 910 title = hash.delete(:title)
> 911 title ||= hash[:name]
> 912 title ||= hash[key_attributes.first] if
> key_attributes.length == 1
>
> - it is impossible for this code to generate a valid title for a
> composite title (since it doesn't know how to map backwards from the
> params out of the has to the composite title)
>
> now that we have a title that is invalid

You, on the other hand, have a pretty good one right there. :)

>
> - we call to_hash when we generate the original parameters
>
> @original_parameters = resource.to_hash
>
> - to_hash tries to parse the title (using def parse_title), which
> leads to failures b/c the title is invalid
>
> I can get around this by adding the following pattern:
>
> # this pattern is required for purging
> [ /^(.*)$/,
> [
> [ :name, lambda{|x| x} ]
> ]
> ]
>
> which is added just for purging

Hmmm. I'm not seeing how that actually fixes it rather than just hiding
it by setting the name to the title so it can later set the title to the
name. I think what's really needed is to have hash2resource be smarter
about assigning a title...which may be what you're saying as well.

-- Markus

[Dan Bode]

On Sun, May 22, 2011 at 4:42 PM, markus <mar...@reality.com> wrote:

> It actually seems to get a little sticky for purging, below I am
> outlining how it works, I was just curious if anyone has spent time
> thinking about this use case before and how it should work:
>
> As far as I can tell, the title_patterns can only map in one direction
> title->parameters

That's correct.  Since it's made up of two non-invertible operations
(matching a regular expression & mapping through lambdas) there's no
automatically generated inverse operation.  We (Stephan and I) discussed
providing support for a converse operation (something like a micro-erb
template that would produce a title based on the other attributes) but
since we couldn't come up with compelling use case at the time we
decided it was out of scope.

I am going to be working on a patch for a joiner, I have some other use cases besides the ones listed below:

- puppet resource (will not print all resources b/c of name collisions)

- I was also getting lots of name collisions when I was plugins:

warning: Limits dan found in both parsed and parsed; skipping the parsed version

 

I was not saying that it fixed it, I was saying here is a dirty hack I have to do that seems to fix it.

After further progressing into the problem, I have realized that it just masked my one issue so that I could encounter other issues :)
 

-- Markus


--
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.

[markus]

So what if we replaced this:

910 title = hash.delete(:title)
911 title ||= hash[:name]
912 title ||= hash[key_attributes.first] if key_attributes.length == 1

With something that took an array of arrays of strings and symbols and
"reconstitutes" the title from the first one that matches:

title = hash.delete(:title)
title ||= patterns.
collect { |p| p.collect {|x| (x.is_a? Symbol) ? hash[x] : x}}.
find { |pattern| pattern.all? { |x| x } }.
join('')

And then the default (to get the same behaviour as before) would be:

[
[:name],

[key_attributes.first] if key_attributes.length == 1

].compact

...but could (on a type-by-type basis) include things like:

[:username,"@",:host,".",:domain]

....or

[:protocol,":",:port]

...or whatever to build parsible titles as needed.

-- Markus

[Dan Bode]

I just finished something very similar:


  def self.title_join(hash)
    "#{hash[:domain]}/#{hash[:type]}/#{hash[:item]}"
  end

where the default case was:

+  def self.title_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 a
+    end
+  end

It will be a little while before I can get this code cleaned up into a patch, it does not quite work yet...

-- Markus

[markus]

> I just finished something very similar:
>
>
> def self.title_join(hash)
> "#{hash[:domain]}/#{hash[:type]}/#{hash[:item]}"
> end
>
> where the default case was:
>
> + def self.title_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 a
> + end
> + end
>
> It will be a little while before I can get this code cleaned up into a
> patch, it does not quite work yet...

Yeah, that looks like a good way to handle it & should work fine,
assuming things are all in the right places and called correctly, etc.
Shout if you get stuck & need help debugging it.

-- Markus

[Stefan Schulte]

On Sun, May 22, 2011 at 05:56:29PM -0700, Dan Bode wrote:
> I just finished something very similar:
>
>
> def self.title_join(hash)
> "#{hash[:domain]}/#{hash[:type]}/#{hash[:item]}"
> end
>
> where the default case was:
>
> + def self.title_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 a
> + end
> + end
>

Hm, does this work in case I have one keyattribute that is not "name"
(like the fileresource where the keyattribute is "path")?

If not, here is a second suggestion

def self.title_join(hash)
# extract all parameters that are keyattributes to build defaulttitle
passed_keyattributes = hash.values_at(*self.key_attributes)
if passed_keyattributes.include?(nil)
nil # or raise an error?
else
passed_keyattributes.join('/')
end
end

This way you only have to overwrite the method if you're unhappy with
the default joiner "/".

-Stefan

[markus]

> Hm, does this work in case I have one keyattribute that is not "name"
> (like the fileresource where the keyattribute is "path")?

No, but neither does the existing code. :(

> If not, here is a second suggestion
>
> def self.title_join(hash)
> # extract all parameters that are keyattributes to build defaulttitle
> passed_keyattributes = hash.values_at(*self.key_attributes)
> if passed_keyattributes.include?(nil)
> nil # or raise an error?
> else
> passed_keyattributes.join('/')
> end
> end
>
> This way you only have to overwrite the method if you're unhappy with
> the default joiner "/".

The problem there is that the set of key attributes isn't really what
we're after.

Suppose our resources were coordinates and our patterns were:

[[/#{num},#{num}/, [[:x,identity],[:y,identity]]],
[/r:#{num} *theta:#{num}/, [[:r,identity],[:theta,identity]]]]

Depending on how we did things under the hood, we'd likely never get all
four key attributes at once, but if we ever did the generated name would
be order indeterminate gibberish.

For this and similar reasons I like Dan's "fail at design/testing time
if we can't infer it" approach better here.

-- Markus