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.
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.
[…]
> 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 from this
input data. That would give you all the benefits of history tracking,
and wouldn't be enormously more complex than the current model, 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")
> 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.
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.
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. Ideally, we should
defend against having rich types 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.
> + 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.)
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?
Thanks for the contribution,
Daniel
--
⎋ Puppet Labs Developer – http://puppetlabs.com
✉ Daniel Pittman <dan...@puppetlabs.com>
✆ Contact me via gtalk, email, or phone: +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.
