On Thu, Mar 17, 2011 at 10:53 AM, Daniel Pittman <dan...@puppetlabs.com>wrote:
> On Thu, Mar 17, 2011 at 02:22, Dan Bode <bod...@gmail.com> wrote:
>
> > This function allows you to dynamically generate resources,
> > passing them as a hash to the create_resources function.
> >
> > This was originally written to be used together with an ENC.
> >
> > Resources can be programitally generated as yaml and passed to a class.
>
> So, this is a pretty fundamental transformation of Puppet, I think.
> We now have a declarative system, except where we use this sort of
> programmatic set of inputs. I would be happier if there was a regular
> way to specify a class using YAML, or JSON, and this was a "run-time"
> rather than "static" method to access that functionality.
>
I am not exactly what you mean. My use case is to transform from yaml to
resources.
> Er, and this isn't actually true: anything that can generate a hash,
> including writing it in the Puppet DSL, is the input. This suggests
> that it is limited, but we have really made a new mechanism that can
> entirely bypass the regular DSL in favour of a data-focused version of
> the same.
>
That sounds slightly terrifying, I am not sure if I support those plans :)
> […]
> > Now I can dynamically determine how webserver instances are deployed to
> nodes
> > by updating the YAML files.
>
> Alternately, I would be happy to see machine-generated DSL
you mean puppet language? I have heard of people doing this before and I am
not a big fan.
this was actually the approach that at least one of the customers that I
wrote this for wanted to take, I led them down this path b/c I think that
generating data that drives code is more powerful/flexible/easier to
maintain than code that generates code.
Reasons I don't like code that generates code:
1. The code for generating the puppet code becomes the expression of what
resources should be created,
2. You lose the benfit of the simplicity of the simple Puppet DSL
2. The Puppet parser does not have a well defined API, so it is not as easy
to interact with Puppet as it is with YAML
> from this
> input data. That would give you all the benefits of history tracking,
>
In both of the solutions that I have implemented this for, the generated
YAML is version controlled
> and wouldn't be enormously more complex than the current model,
unless I misunderstand, I disagree
> I
> would have expected. (Adding a shim between the master and the ENC
> that generated this inline would be easy enough, I think.)
>
>
> Anyway, practically we should ensure that the documentation for this
> is as extensive and serious as the Puppet DSL for defining resources,
> because it is totally going to have the same sort of problems and uses
> that the existing DSL does. (Actually, probably more, because we also
> need "why would you use this instead of the regular DSL, and "please
> don't work around the DSL with this")
>
>
You really got me thinking about this one. I wish I could put a 'for
advanced users only' sticker on it, it is clearly not for the Puppet novice.
I do not promote this to bypass the DSL, what as a way to keep your Puppet
code as simple as possible (moving the complexity to the process that
generates data to drive the simple config)
> > Signed-off-by: Dan Bode <bod...@gmail.com>
> > ---
> > lib/puppet/parser/functions/create_resources.rb | 47 +++++++
> > .../unit/parser/functions/create_resources_spec.rb | 135
> ++++++++++++++++++++
> > 2 files changed, 182 insertions(+), 0 deletions(-)
> > create mode 100644 lib/puppet/parser/functions/create_resources.rb
> > create mode 100755 spec/unit/parser/functions/create_resources_spec.rb
> >
> > diff --git a/lib/puppet/parser/functions/create_resources.rb
> b/lib/puppet/parser/functions/create_resources.rb
> > new file mode 100644
> > index 0000000..430f110
> > --- /dev/null
> > +++ b/lib/puppet/parser/functions/create_resources.rb
> > @@ -0,0 +1,47 @@
> > +Puppet::Parser::Functions::newfunction(:create_resources, :doc => '
> > +Converts a hash into a set of resources and adds them to the catalog.
> > +Takes two parameters:
> > + create_resource($type, $resources)
> > + Creates resources of type $type from the $resources hash. Assumes
> that
> > + hash is in the following form:
> > + {title=>{parameters}}
> > + This is currently tested for defined resources, classes, as well as
> native types
> > +') do |args|
> > + raise ArgumentError, ("create_resources(): wrong number of arguments
> (#{args.length}; must be 2)") if args.length != 2
> > + #raise ArgumentError, 'requires resource type and param hash' if
> args.size < 2
> > + # figure out what kind of resource we are
> > + type_of_resource = nil
> > + type_name = args[0].downcase
> > + if type_name == 'class'
> > + type_of_resource = :class
> > + else
> > + if resource = Puppet::Type.type(type_name.to_sym)
> > + type_of_resource = :type
> > + elsif resource = find_definition(type_name.downcase)
> > + type_of_resource = :define
> > + else
> > + raise ArgumentError, "could not create resource of unknown type
> #{type_name}"
> > + end
> > + end
> > + # iterate through the resources to create
> > + args[1].each do |title, params|
> > + raise ArgumentError, 'params should not contain title'
> if(params['title'])
> > + case type_of_resource
> > + when :type
> > + res = resource.hash2resource(params.merge(:title => title))
> > + catalog.add_resource(res)
> > + when :define
> > + p_resource = Puppet::Parser::Resource.new(type_name, title, :scope
> => self, :source => resource)
> > + params.merge(:name => title).each do |k,v|
> > + p_resource.set_parameter(k,v)
> > + end
> > + resource.instantiate_resource(self, p_resource)
> > + compiler.add_resource(self, p_resource)
> > + when :class
> > + klass = find_hostclass(title)
> > + raise ArgumentError, "could not find hostclass #{title}" unless
> klass
> > + klass.ensure_in_catalog(self, params)
> > + compiler.catalog.add_class([title])
> > + end
> > + end
> > +end
>
> The separation of logic about the "type" of resource, and the code
> that creates it, is pretty ugly. It would be much nicer if we could
> avoid having two flow control structures in there.
>
My first draft of the code only had one control structure, I didnt like it
b/c I had to figure out the type for every iteration of the loop, in this
case I traded performance over readability, without exactly understanding
the performance indications.
do you think that
> + iresource = Puppet::Type.type(type_name)
and
> + resource = find_definition(type_name.downcase)
is light weight enough that it doesn't matter if I execute it inside of the
loop?
> Since this is a core patch, it would be nice to replace the logic in
> the function with methods on the appropriate internal infrastructure
> (Puppet::Resource) to create an instance from a hash – which this
> could be a few lines using.
>
Would you mind providing an example? I would not be surprised if there was
an easier way to do this for all Puppet::Resource types (although I did try,
and would be a little confused about why ensure_in_catalog does not use the
simpler way)
> You are not doing any sanitization of input here, which you should be
> for external inputs: at the very least, ensure that the keys in the
> parameter hash are the appropriate basic types.
I am not sure what you mean, the keys are titles. The types themselves
provide input validation
> Ideally, we should
> defend against having rich types
ah, gotcha, restrict to string
> created on either side, so that only
> things that the DSL could create can be passed here.
>
> > diff --git a/spec/unit/parser/functions/create_resources_spec.rb
> b/spec/unit/parser/functions/create_resources_spec.rb
> > new file mode 100755
> > index 0000000..d4095b7
> > --- /dev/null
> > +++ b/spec/unit/parser/functions/create_resources_spec.rb
> > @@ -0,0 +1,135 @@
> > +require 'puppet'
> > +require File.dirname(__FILE__) + '/../../../spec_helper'
> > +
> > +describe 'function for dynamically creating resources' do
> > +
> > + def get_scope
> > + @topscope = Puppet::Parser::Scope.new
> > + # This is necessary so we don't try to use the compiler to discover
> our parent.
> > + @topscope.parent = nil
> > + @scope = Puppet::Parser::Scope.new
> > + @scope.compiler =
> Puppet::Parser::Compiler.new(Puppet::Node.new("floppy", :environment =>
> 'production'))
> > + @scope.parent = @topscope
> > + @compiler = @scope.compiler
> > + end
> > + before :each do
> > + get_scope
>
> It would be nice to use the 'let' functionality around the get_scope
> actions, rather than shoving stuff into member variables, both for
> reducing long-term memory use, and to allow demand-drawn construction
> of objects.
>
I can have a read about this, thanks for the tip
> > + Puppet::Parser::Functions.function(:create_resources)
> > + end
> > +
> > + it "should exist" do
> > + Puppet::Parser::Functions.function(:create_resources).should ==
> "function_create_resources"
> > + end
>
> This test is pretty much redundant: if it would fail, so would every
> other test, so we might as well eliminate it. (Not that I blame you:
> the other files next to it perform the same next-to-useless test.)
>
yeah, copy and paste, it would be great to not have bad tests in every
example :)
> Finally, there seem to be a set of similar tests in the rest of the
> code: that edges can be added, etc. It would be good to have only one
> copy of that code, with appropriate data input to let the code test
> it. After all, if we find a bug in one part of those behaviours we
> should really be checking it for every other top level type input,
> right?
>
I don't follow 100%, are you saying more than just check the edge creation
in a method?
> Thanks for the contribution,
>
thanks for the help, I am starting to get a handle on this Ruby development
stuff ;)
> Daniel
> --
> ⎋ Puppet Labs Developer – http://puppetlabs.com
> ✉ Daniel Pittman <dan...@puppetlabs.com>
> ✆ Contact me via gtalk, email, or phone: <%2B1%20%28877%29%20575-9775>+1
> (877) 575-9775
> ♲ Made with 100 percent post-consumer electrons
>
> --
> You received this message because you are subscribed to the Google Groups
> "Puppet Developers" group.
> To post to this group, send email to puppet-dev@googlegroups.com.
> To unsubscribe from this group, send email to
> puppet-dev+unsubscr...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/puppet-dev?hl=en.
>
>
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
