[PATCH/puppet 1/1] Fix #2389 - Enhance Puppet DSL with Hashes

[Brice Figureau]

Hi,

I had a couple of spare hours and decided to hack on this topic.
I don't think the work is completely finished (especially it lacks
hash access string interpolation which is much more complex than it
appears, advice needed here).

I'm looking for reviews, comments and testing as usual.
Thanks,

Brice

Original commit msg:
This bring a new container syntax to the Puppet DSL: hashes.

Hashes are defined like Ruby Hash:
{ key1 => val1, ... }

Hash keys are strings, but hash values can be any possible right
values admitted in Puppet DSL (ie function call, variables access...)

Currently it is possible:

1) to assign hashes to variable
$myhash = { key1 => "myval", key2 => $b }

2) to access hash members (recursively) from a variable containing
a hash (works for arrays and mix of arrays and hashes too):

$myhash = { key => { subkey => "b" }}
notice($myhash[key][subjey]]

3) to use hash member access as resource title

4) to use hash in default definition parameter or resource parameter if
the type supports it (known for the moment).

It is not possible to string interpolate an hash access. If it proves
to be an issue it can be added or work-arounded with a string concatenation
operator easily.

It is not possible to use an hash as a resource title. This might be
possible once we support compound resource title.

Unlike the proposed syntax in the ticket it is not possible to assign
individual hash member (mostly to respect write once nature of variable
in puppet).

Signed-off-by: Brice Figureau <brice-...@daysofwonder.com>
---
lib/puppet/parser/ast.rb | 1 +
lib/puppet/parser/ast/asthash.rb | 34 +
lib/puppet/parser/ast/leaf.rb | 21 +
lib/puppet/parser/grammar.ra | 48 +
lib/puppet/parser/parser.rb | 2001 +++++++++++++++++++++-----------------
lib/puppet/parser/scope.rb | 5 +-
spec/unit/parser/ast/asthash.rb | 64 ++
spec/unit/parser/ast/leaf.rb | 83 ++
spec/unit/parser/scope.rb | 10 +
test/data/snippets/hash.pp | 33 +
test/language/snippets.rb | 7 +
11 files changed, 1389 insertions(+), 918 deletions(-)
create mode 100644 lib/puppet/parser/ast/asthash.rb
create mode 100644 spec/unit/parser/ast/asthash.rb
create mode 100644 test/data/snippets/hash.pp

diff --git a/lib/puppet/parser/ast.rb b/lib/puppet/parser/ast.rb
index ad8af74..dab1091 100644
--- a/lib/puppet/parser/ast.rb
+++ b/lib/puppet/parser/ast.rb
@@ -92,6 +92,7 @@ end
# And include all of the AST subclasses.
require 'puppet/parser/ast/arithmetic_operator'
require 'puppet/parser/ast/astarray'
+require 'puppet/parser/ast/asthash'
require 'puppet/parser/ast/branch'
require 'puppet/parser/ast/boolean_operator'
require 'puppet/parser/ast/caseopt'
diff --git a/lib/puppet/parser/ast/asthash.rb b/lib/puppet/parser/ast/asthash.rb
new file mode 100644
index 0000000..2860657
--- /dev/null
+++ b/lib/puppet/parser/ast/asthash.rb
@@ -0,0 +1,34 @@
+require 'puppet/parser/ast/leaf'
+
+class Puppet::Parser::AST
+ class ASTHash < Leaf
+ include Enumerable
+
+ def [](index)
+ end
+
+ # Evaluate our children.
+ def evaluate(scope)
+ items = {}
+
+ @value.each_pair do |k,v|
+ items.merge!({ k => v.safeevaluate(scope) })
+ end
+
+ return items
+ end
+
+ def merge(hash)
+ case hash
+ when ASTHash
+ @value = @value.merge(hash.value)
+ when Hash
+ @value = @value.merge(hash)
+ end
+ end
+
+ def to_s
+ "{" + @value.collect { |v| v.collect { |a| a.to_s }.join(' => ') }.join(', ') + "}"
+ end
+ end
+end
diff --git a/lib/puppet/parser/ast/leaf.rb b/lib/puppet/parser/ast/leaf.rb
index 153120a..9b3496f 100644
--- a/lib/puppet/parser/ast/leaf.rb
+++ b/lib/puppet/parser/ast/leaf.rb
@@ -152,6 +152,27 @@ class Puppet::Parser::AST
end
end

+ class HashOrArrayAccess < AST::Leaf
+ attr_accessor :variable, :key
+
+ def evaluate(scope)
+ container = variable.respond_to?(:evaluate) ? variable.safeevaluate(scope) : variable
+ object = (container.is_a?(Hash) or container.is_a?(Array)) ? container : scope.lookupvar(container)
+
+ accesskey = key.respond_to?(:evaluate) ? key.safeevaluate(scope) : key
+
+ unless object.is_a?(Hash) or object.is_a?(Array)
+ raise Puppet::ParseError, "#{variable} is not an hash or array when accessing it with #{accesskey}"
+ end
+
+ return object[accesskey]
+ end
+
+ def to_s
+ "\$#{variable.to_s}[#{key.to_s}]"
+ end
+ end
+
class Regex < AST::Leaf
def initialize(hash)
super
diff --git a/lib/puppet/parser/grammar.ra b/lib/puppet/parser/grammar.ra
index 4c74211..6b7b31b 100644
--- a/lib/puppet/parser/grammar.ra
+++ b/lib/puppet/parser/grammar.ra
@@ -131,6 +131,7 @@ namestring: name
| funcrvalue
| selector
| quotedtext
+ | hasharrayaccesses
| CLASSNAME {
result = ast AST::Name, :value => val[0][:value]
}
@@ -325,6 +326,7 @@ resourcename: quotedtext
| selector
| variable
| array
+ | hasharrayaccesses

assignment: VARIABLE EQUALS expression {
if val[0][:value] =~ /::/
@@ -403,6 +405,8 @@ rvalue: quotedtext
| selector
| variable
| array
+ | hash
+ | hasharrayaccesses
| resourceref
| funcrvalue
| undef
@@ -781,6 +785,50 @@ regex: REGEX {
result = ast AST::Regex, :value => val[0][:value]
}

+hash: LBRACE hashpairs RBRACE {
+ if val[1].instance_of?(AST::ASTHash)
+ result = val[1]
+ else
+ result = ast AST::ASTHash, { :value => val[1] }
+ end
+}
+ | LBRACE hashpairs COMMA RBRACE {
+ if val[1].instance_of?(AST::ASTHash)
+ result = val[1]
+ else
+ result = ast AST::ASTHash, { :value => val[1] }
+ end
+} | LBRACE RBRACE {
+ result = ast AST::ASTHash
+}
+
+hashpairs: hashpair
+ | hashpairs COMMA hashpair {
+ if val[0].instance_of?(AST::ASTHash)
+ result = val[0].merge(val[2])
+ else
+ result = ast AST::ASTHash, :value => val[0]
+ result.merge(val[2])
+ end
+}
+
+hashpair: key FARROW rvalue {
+ result = ast AST::ASTHash, { :value => { val[0] => val[2] } }
+}
+
+key: NAME { result = val[0][:value] }
+ | SQTEXT { result = val[0][:value] }
+ | DQTEXT { result = val[0][:value] }
+
+hasharrayaccess: VARIABLE LBRACK rvalue RBRACK {
+ result = ast AST::HashOrArrayAccess, :variable => val[0][:value], :key => val[2]
+}
+
+hasharrayaccesses: hasharrayaccess
+ | hasharrayaccess LBRACK rvalue RBRACK {
+ result = ast AST::HashOrArrayAccess, :variable => val[0], :key => val[2]
+}
+
end
---- header ----
require 'puppet'
diff --git a/lib/puppet/parser/parser.rb b/lib/puppet/parser/parser.rb
index 376b818..826b58c 100644
--- a/lib/puppet/parser/parser.rb
+++ b/lib/puppet/parser/parser.rb
[patch elided]
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index d6d6630..e27640f 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -317,8 +317,11 @@ class Puppet::Parser::Scope
# lookup the value in the scope if it exists and insert the var
table[name] = lookupvar(name)
# concatenate if string, append if array, nothing for other types
- if value.is_a?(Array)
+ case value
+ when Array
table[name] += value
+ when Hash
+ table[name].merge!(value)
else
table[name] << value
end
diff --git a/spec/unit/parser/ast/asthash.rb b/spec/unit/parser/ast/asthash.rb
new file mode 100644
index 0000000..4c700fe
--- /dev/null
+++ b/spec/unit/parser/ast/asthash.rb
@@ -0,0 +1,64 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../../spec_helper'
+
+describe Puppet::Parser::AST::ASTHash do
+ before :each do
+ @scope = Puppet::Parser::Scope.new()
+ end
+
+ it "should have a [] accessor" do
+ hash = Puppet::Parser::AST::ASTHash.new(:value => {})
+ hash.should respond_to(:[])
+ end
+
+ it "should have a merge functionality" do
+ hash = Puppet::Parser::AST::ASTHash.new(:value => {})
+ hash.should respond_to(:merge)
+ end
+
+ it "should be able to merge 2 AST hashes" do
+ hash = Puppet::Parser::AST::ASTHash.new(:value => { "a" => "b" })
+
+ hash.merge(Puppet::Parser::AST::ASTHash.new(:value => {"c" => "d"}))
+
+ hash.value.should == { "a" => "b", "c" => "d" }
+ end
+
+ it "should be able to merge with a ruby Hash" do
+ hash = Puppet::Parser::AST::ASTHash.new(:value => { "a" => "b" })
+
+ hash.merge({"c" => "d"})
+
+ hash.value.should == { "a" => "b", "c" => "d" }
+ end
+
+ it "should evaluate each hash value" do
+ key1 = stub "key1"
+ value1 = stub "value1"
+ key2 = stub "key2"
+ value2 = stub "value2"
+
+ value1.expects(:safeevaluate).with(@scope).returns("b")
+ value2.expects(:safeevaluate).with(@scope).returns("d")
+
+ operator = Puppet::Parser::AST::ASTHash.new(:value => { key1 => value1, key2 => value2})
+ operator.evaluate(@scope)
+ end
+
+ it "should return an evaluated hash" do
+ key1 = stub "key1"
+ value1 = stub "value1", :safeevaluate => "b"
+ key2 = stub "key2"
+ value2 = stub "value2", :safeevaluate => "d"
+
+ operator = Puppet::Parser::AST::ASTHash.new(:value => { key1 => value1, key2 => value2})
+ operator.evaluate(@scope).should == { key1 => "b", key2 => "d" }
+ end
+
+ it "should return a valid string with to_s" do
+ hash = Puppet::Parser::AST::ASTHash.new(:value => { "a" => "b", "c" => "d" })
+
+ hash.to_s.should == '{a => b, c => d}'
+ end
+end
diff --git a/spec/unit/parser/ast/leaf.rb b/spec/unit/parser/ast/leaf.rb
index e968150..48e0e1f 100755
--- a/spec/unit/parser/ast/leaf.rb
+++ b/spec/unit/parser/ast/leaf.rb
@@ -72,6 +72,89 @@ describe Puppet::Parser::AST::String do
end
end

+describe Puppet::Parser::AST::HashOrArrayAccess do
+ before :each do
+ @scope = stub 'scope'
+ end
+
+ it "should evaluate the variable part if necessary" do
+ @scope.stubs(:lookupvar).with("a").returns(["b"])
+
+ variable = stub 'variable', :evaluate => "a"
+ access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => variable, :key => 0 )
+
+ variable.expects(:safeevaluate).with(@scope).returns("a")
+
+ access.evaluate(@scope).should == "b"
+ end
+
+ it "should evaluate the access key part if necessary" do
+ @scope.stubs(:lookupvar).with("a").returns(["b"])
+
+ index = stub 'index', :evaluate => 0
+ access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => index )
+
+ index.expects(:safeevaluate).with(@scope).returns(0)
+
+ access.evaluate(@scope).should == "b"
+ end
+
+ it "should be able to return an array member" do
+ @scope.stubs(:lookupvar).with("a").returns(["val1", "val2", "val3"])
+
+ access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => 1 )
+
+ access.evaluate(@scope).should == "val2"
+ end
+
+ it "should be able to return an hash value" do
+ @scope.stubs(:lookupvar).with("a").returns({ "key1" => "val1", "key2" => "val2", "key3" => "val3" })
+
+ access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key2" )
+
+ access.evaluate(@scope).should == "val2"
+ end
+
+ it "should raise an error if the variable lookup didn't return an hash or an array" do
+ @scope.stubs(:lookupvar).with("a").returns("I'm a string")
+
+ access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key2" )
+
+ lambda { access.evaluate(@scope) }.should raise_error
+ end
+
+ it "should raise an error if the variable wasn't in the scope" do
+ @scope.stubs(:lookupvar).with("a").returns(nil)
+
+ access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key2" )
+
+ lambda { access.evaluate(@scope) }.should raise_error
+ end
+
+ it "should return a correct string representation" do
+ access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key2" )
+ access.to_s.should == '$a[key2]'
+ end
+
+ it "should work with recursive hash access" do
+ @scope.stubs(:lookupvar).with("a").returns({ "key" => { "subkey" => "b" }})
+
+ access1 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key")
+ access2 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => access1, :key => "subkey")
+
+ access2.evaluate(@scope).should == 'b'
+ end
+
+ it "should work with interleaved array and hash access" do
+ @scope.stubs(:lookupvar).with("a").returns({ "key" => [ "a" , "b" ]})
+
+ access1 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key")
+ access2 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => access1, :key => 1)
+
+ access2.evaluate(@scope).should == 'b'
+ end
+end
+
describe Puppet::Parser::AST::Regex do
before :each do
@scope = stub 'scope'
diff --git a/spec/unit/parser/scope.rb b/spec/unit/parser/scope.rb
index 0859ead..66222c9 100755
--- a/spec/unit/parser/scope.rb
+++ b/spec/unit/parser/scope.rb
@@ -29,6 +29,11 @@ describe Puppet::Parser::Scope do
@scope.lookupvar("var").should == "yep"
end

+ it "should be able to look up hashes" do
+ @scope.setvar("var", {"a" => "b"})
+ @scope.lookupvar("var").should == {"a" => "b"}
+ end
+
it "should be able to look up variables in parent scopes" do
@topscope.setvar("var", "parentval")
@scope.lookupvar("var").should == "parentval"
@@ -132,6 +137,11 @@ describe Puppet::Parser::Scope do
@scope.lookupvar("var").should == [4,2]
end

+ it "it should store the merged hash {a => b, c => d}" do
+ @topscope.setvar("var",{"a" => "b"}, :append => false)
+ @scope.setvar("var",{"c" => "d"}, :append => true)
+ @scope.lookupvar("var").should == {"a" => "b", "c" => "d"}
+ end
end

describe "when calling number?" do
diff --git a/test/data/snippets/hash.pp b/test/data/snippets/hash.pp
new file mode 100644
index 0000000..d332498
--- /dev/null
+++ b/test/data/snippets/hash.pp
@@ -0,0 +1,33 @@
+
+$hash = { "file" => "/tmp/myhashfile1" }
+
+file {
+ $hash["file"]:
+ ensure => file, content => "content";
+}
+
+$hash2 = { "a" => { key => "/tmp/myhashfile2" }}
+
+file {
+ $hash2["a"][key]:
+ ensure => file, content => "content";
+}
+
+define test($a = { "b" => "c" }) {
+ file {
+ $a["b"]:
+ ensure => file, content => "content"
+ }
+}
+
+test {
+ "test":
+ a => { "b" => "/tmp/myhashfile3" }
+}
+
+$hash3 = { mykey => "/tmp/myhashfile4" }
+$key = "mykey"
+
+file {
+ $hash3[$key]: ensure => file, content => "content"
+}
diff --git a/test/language/snippets.rb b/test/language/snippets.rb
index 5c7805c..bfd0e53 100755
--- a/test/language/snippets.rb
+++ b/test/language/snippets.rb
@@ -486,6 +486,13 @@ class TestSnippets < Test::Unit::TestCase
assert_file("/tmp/testiftest","if test");
end

+ def snippet_hash
+ assert_file("/tmp/myhashfile1","hash test 1");
+ assert_file("/tmp/myhashfile2","hash test 2");
+ assert_file("/tmp/myhashfile3","hash test 3");
+ assert_file("/tmp/myhashfile4","hash test 4");
+ end
+
# Iterate across each of the snippets and create a test.
Dir.entries(snippetdir).sort.each { |file|
next if file =~ /^\./
--
1.6.5.2

[Markus Roberts]

Brice --

Nice.  I have some concerns about the potential for abuse, but that's always the case with language features powerful enough to be interesting/useful.

I had a couple of spare hours and decided to hack on this topic.
I don't think the work is completely finished (especially it lacks
hash access string interpolation which is much more complex than it
appears, advice needed here).

Can you elaborate?

> Unlike the proposed syntax in the ticket it is not possible to assign

individual hash member (mostly to respect write once nature of variable
in puppet).

Hmmm.  I can see the reasoning, but I'm not sure if restricting it to assigning values to previously unused keys wouldn't be a better analog. 

@@ -317,8 +317,11 @@ class Puppet::Parser::Scope

            # lookup the value in the scope if it exists and insert the var
            table[name] = lookupvar(name)
            # concatenate if string, append if array, nothing for other types
-            if value.is_a?(Array)
+            case value
+            when Array
                table[name] += value
+            when Hash
+                table[name].merge!(value)
            else
                table[name] << value
            end

What will this do with something like:

    $my_hash += "foo"

Is it an error or does it add a { :value => "foo"} to $my_hash?

I'll download and play with it in a bit, & return with more thoughts.

-- Markus

[Brice Figureau]

On 10/11/09 19:01, Markus Roberts wrote:
> Brice --
>
> Nice. I have some concerns about the potential for abuse, but that's always
> the case with language features powerful enough to be interesting/useful.

Sure. Maybe we can prevent those to occur. Do you have any examples?

> I had a couple of spare hours and decided to hack on this topic.
>> I don't think the work is completely finished (especially it lacks
>> hash access string interpolation which is much more complex than it
>> appears, advice needed here).
>>
>
> Can you elaborate?

I meant this:

notice("this is a hash access: ${hash[$mykey]}")

or even:
notice("this is a hash access: ${hash[key][subkey][subsubkey][1]}")

I see 2 possibilities:
* have a string concatenation operator (+ or .) to be able to write:

"this is a a hash access:" + $hash[$key] + " and this is cool"

* let string interpolation know about hash access.

>> Unlike the proposed syntax in the ticket it is not possible to assign
>
>> individual hash member (mostly to respect write once nature of variable
>> in puppet).
>>
>
> Hmmm. I can see the reasoning, but I'm not sure if restricting it to
> assigning values to previously unused keys wouldn't be a better analog.

The thing is that currently the hash is backed by a ruby Hash. If we
want to prevent modifying already present values, we need to wrap this
in one of our own object.
I didn't think about that in the first instance so I decided to first
release the patch as is to gather comments, and then if we need this
feature then it'll be time to refactor this.

> @@ -317,8 +317,11 @@ class Puppet::Parser::Scope
>
>> # lookup the value in the scope if it exists and insert the var
>> table[name] = lookupvar(name)
>> # concatenate if string, append if array, nothing for other
>> types
>> - if value.is_a?(Array)
>> + case value
>> + when Array
>> table[name] += value
>> + when Hash
>> + table[name].merge!(value)
>> else
>> table[name]<< value
>> end
>>
>
> What will this do with something like:
>
> $my_hash += "foo"
>
> Is it an error or does it add a { :value => "foo"} to $my_hash?

It should be an error. BTW, merge("foo") will produce an Argument Error.
So maybe I should check the operand first.

> I'll download and play with it in a bit,& return with more thoughts.

The code is in tickets/master/2389 in my github repository.
Thanks for your time,
--
Brice Figureau
My Blog: http://www.masterzen.fr/

[Markus Roberts]

> > Nice.  I have some concerns about the potential for abuse, but that's always

> the case with language features powerful enough to be interesting/useful.

Sure. Maybe we can prevent those to occur. Do you have any examples?

Well, one thought is that people might start using it as a back-channel for node/class information.  Worst case would be all nodes are default and include one class that's a mess of spaghetti controlled by nested hashes strategically keyed off the node's hostname or some such.  I'm not suggesting that anyone would do this, but by introducing the feature we create the possibility.

Another is that nested hashes don't have to have the same structure throughout, and it can sometimes be tricky to debug when access fails at some distant point.

A third is that immutable hashes are less useful but mutable hashes are potentially a path for stateful / order dependent logic to creep in.

Just to be clear, I'm not objecting to the feature so much as trying to think through the consequences.

>> it lacks hash access string interpolation
>> which is much more complex than it
>> appears, advice needed here).

 

> Can you elaborate?

I meant this:

notice("this is a hash access: ${hash[$mykey]}")

or even:
notice("this is a hash access: ${hash[key][subkey][subsubkey][1]}")

I see 2 possibilities:
 * have a string concatenation operator (+ or .) to be able to write:

"this is a a hash access:" + $hash[$key] + " and this is cool"

 * let string interpolation know about hash access.

As yes, strinterp is dealing with the string post-parsing and doesn't know about structured values.  Adding one level of hash-awareness in strinterp would be trivial, but to make it "correct" (e.g. deal with things like h1[h2["k1"]["k2]]["k3"]) would require a refactor.

A third option would be to do something like what ruby does and parse the interpolations (so that a string parses as an array of literals and expressions which, when evaluated and concatenated, give the final value.

 

>> Unlike the proposed syntax in the ticket it is not possible to assign
>
>> individual hash member (mostly to respect write once nature of variable
>> in puppet).
>>
>
> Hmmm.  I can see the reasoning, but I'm not sure if restricting it to
> assigning values to previously unused keys wouldn't be a better analog.

The thing is that currently the hash is backed by a ruby Hash. If we
want to prevent modifying already present values, we need to wrap this
in one of our own object.

Well, we control the updates, so we could just raise an exception if they had any keys in common before calling merge.

>
>>              # lookup the value in the scope if it exists and insert the var
>>              table[name] = lookupvar(name)
>>              # concatenate if string, append if array, nothing for other
>> types
>> -            if value.is_a?(Array)
>> +            case value
>> +            when Array
>>                  table[name] += value
>> +            when Hash
>> +                table[name].merge!(value)
>>              else
>>                  table[name]<<  value
>>              end
>>
>
> What will this do with something like:
>
>      $my_hash += "foo"
>
> Is it an error or does it add a { :value =>  "foo"} to $my_hash?

> @@ -317,8 +317,11 @@ class Puppet::Parser::Scope
It should be an error. BTW, merge("foo") will produce an Argument Error.
So maybe I should check the operand first.

That's what I was wondering about.  I didn't know if this was supposed to be calling "merge" (no bang) on our object and getting a :value key, or hit the ruby hash directly & possibly fail.
 

> I'll download and play with it in a bit,&  return with more thoughts.

The code is in tickets/master/2389 in my github repository.
Thanks for your time,

*smile* And thank you for yours.  This is a tidy bit of work, and much more entertaining to think about than the random bugs I've been chasing down.

-- Markus

[Luke Kanies]

On Nov 10, 2009, at 7:57 AM, Brice Figureau wrote:

> Hashes are defined like Ruby Hash:
> { key1 => val1, ... }
>
> Hash keys are strings, but hash values can be any possible right
> values admitted in Puppet DSL (ie function call, variables access...)
>
> Currently it is possible:
>
> 1) to assign hashes to variable
> $myhash = { key1 => "myval", key2 => $b }
>
> 2) to access hash members (recursively) from a variable containing
> a hash (works for arrays and mix of arrays and hashes too):
>
> $myhash = { key => { subkey => "b" }}
> notice($myhash[key][subjey]]
>
> 3) to use hash member access as resource title
>
> 4) to use hash in default definition parameter or resource parameter
> if
> the type supports it (known for the moment).
>
> It is not possible to string interpolate an hash access. If it proves
> to be an issue it can be added or work-arounded with a string
> concatenation
> operator easily.
>
> It is not possible to use an hash as a resource title. This might be
> possible once we support compound resource title.

If we're going to add hash functionality, I think that at the same
time we should add the ability to treat resources like hashes. That
is, the following syntax should work:

file { "/foo": owner => luke }

$owner = File["/foo"][owner]

This treats the resource types like a hash, with the resource title as
the indexer, which makes sense and helps tie together this otherwise
somewhat confusing syntax with the new hash syntax.

If we don't add this, then I think the resource reference syntax and
the hash access syntax will be fantastically confusing.

Does that seem reasonable.

--
The great aim of education is not knowledge but action.
-- Herbert Spencer
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

[Markus Roberts]

If we're going to add hash functionality, I think that at the same
time we should add the ability to treat resources like hashes.  That
is, the following syntax should work:

file { "/foo": owner => luke }

$owner = File["/foo"][owner]

This treats the resource types like a hash, with the resource title as
the indexer, which makes sense and helps tie together this otherwise
somewhat confusing syntax with the new hash syntax.

If we don't add this, then I think the resource reference syntax and
the hash access syntax will be fantastically confusing.

Does that seem reasonable.

On the face of it, yes, it seems very reasonable.  And off hand I'm not seeing any huge hurdles to implementing it.

Do you have thoughts on the best way to handle string interpolation?  I'm leaning towards making the parser interpolation aware rather than hacking a micro-parser into the string interpolation routine (or rather, extending the one that's there) but I'm not in love with any of the suggestions.

[Luke Kanies]

I kinda missed this. What do you mean here?

--
A classic is something that everybody wants to have read and nobody
wants to read. -- Mark Twain

[Markus Roberts]

> > Do you have thoughts on the best way to handle string

> interpolation?  I'm leaning towards making the parser interpolation
> aware rather than hacking a micro-parser into the string
> interpolation routine (or rather, extending the one that's there)
> but I'm not in love with any of the suggestions.

I kinda missed this.  What do you mean here?

Brice had noted earlier on the thread that string interpolation was messier than he had expected going in because (as I also found when I looked into it)  we aren't parsing strings like ruby does, we're passing them through verbatim and have a poor man's parse (a regexp) that interpolates values at time of use.

We floated several possible solutions, none great, earlier on this thread.

My current favorite would be to just parse string interpolation along with everything else (i.e. in the parser, where god intended it) and pull out the regexp games at use-time rather than trying to get fancier there in a doomed effort to avoid dealing with it up front.

[Nigel Kersten]

reasonable? It sounds bloody brilliant to me. 

 

--
The great aim of education is not knowledge but action.
    -- Herbert Spencer
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

--
nigel

[Luke Kanies]

Ah, right.

I tried to do the string interpolation at parse time but I couldn't
figure out how to do it and couldn't find a good example. If you can
make it work, I'm all for it.

--
The difference between scientists and engineers is that when
engineers screw up, people die. -- Professor Orthlieb

[Brice Figureau]

On Wed, 2009-11-11 at 15:06 -0800, Luke Kanies wrote:
> On Nov 10, 2009, at 7:57 AM, Brice Figureau wrote:
>
> > Hashes are defined like Ruby Hash:
> > { key1 => val1, ... }
> >
> > Hash keys are strings, but hash values can be any possible right
> > values admitted in Puppet DSL (ie function call, variables access...)
> >
> > Currently it is possible:
> >
> > 1) to assign hashes to variable
> > $myhash = { key1 => "myval", key2 => $b }
> >
> > 2) to access hash members (recursively) from a variable containing
> > a hash (works for arrays and mix of arrays and hashes too):
> >
> > $myhash = { key => { subkey => "b" }}
> > notice($myhash[key][subjey]]
> >
> > 3) to use hash member access as resource title
> >
> > 4) to use hash in default definition parameter or resource parameter
> > if
> > the type supports it (known for the moment).
> >
> > It is not possible to string interpolate an hash access. If it proves
> > to be an issue it can be added or work-arounded with a string
> > concatenation
> > operator easily.
> >
> > It is not possible to use an hash as a resource title. This might be
> > possible once we support compound resource title.
>
>
> If we're going to add hash functionality,

If we're not going to add the functionality, please say it now :-)
I know you're usually reluctant to transform the puppet DSL into a
poor-man's ruby, and I understand why well.
If you think we're going too far, let me know.

> I think that at the same
> time we should add the ability to treat resources like hashes. That
> is, the following syntax should work:
>
> file { "/foo": owner => luke }
>
> $owner = File["/foo"][owner]
>
> This treats the resource types like a hash, with the resource title as
> the indexer, which makes sense and helps tie together this otherwise
> somewhat confusing syntax with the new hash syntax.

Is Your concern the comparable syntax of resource references and hash
access?
Maybe we can write hash access differently, ala $hash{key} (kind of
perlish) or even instead of providing a syntax use a parser function?

> If we don't add this, then I think the resource reference syntax and
> the hash access syntax will be fantastically confusing.
>
> Does that seem reasonable.

Yes, it does. And I think this should be moderatly easy to add.
And of course this is only a view of the resource, there's no "write"
access to the resource parameter, correct?
--
Brice Figureau
Follow the latest Puppet Community evolutions on www.planetpuppet.org!

[Luke Kanies]

I'm comfortable adding it, although I think people are going to want
supporting functionality in the language -- e.g., something to easily
turn a hash into a resource. This also provides the functionality we
need to add richer data structures to Facter, which is good.

>> I think that at the same
>> time we should add the ability to treat resources like hashes. That
>> is, the following syntax should work:
>>
>> file { "/foo": owner => luke }
>>
>> $owner = File["/foo"][owner]
>>
>> This treats the resource types like a hash, with the resource title
>> as
>> the indexer, which makes sense and helps tie together this otherwise
>> somewhat confusing syntax with the new hash syntax.
>
> Is Your concern the comparable syntax of resource references and hash
> access?
> Maybe we can write hash access differently, ala $hash{key} (kind of
> perlish) or even instead of providing a syntax use a parser function?

We're definitely not using a different indexing bracket. :)

>> If we don't add this, then I think the resource reference syntax and
>> the hash access syntax will be fantastically confusing.
>>
>> Does that seem reasonable.
>
> Yes, it does. And I think this should be moderatly easy to add.
> And of course this is only a view of the resource, there's no "write"
> access to the resource parameter, correct?

I don't see why not, really, as long as it goes through the standard
parameter setting methods - then the overriding rules will still be
followed, so it's just syntactic sugar for an override.

--
Most people are born and years later die without really having lived
at all. They play it safe and tiptoe through life with no aspiration
other than to arrive at death safely. -- Tony Campolo, "Carpe Diem"

[Brice Figureau]

I started working on this, but I'm encountering an issue.
I started with the read version, ie:

$owner = File["/foo"][owner]

What I did is to create a new AST leaf which from a resource reference
aims to extract one parameter.

The issue is that at evaluation time the catalog might not know yet the
resource. So calling scope.compiler.find_resource( resource_ref ) will
certainly fail.

Basically this is the same issue as the defined parser function.

So the question is: should I care?
And if yes, what would be the best way to handle this?

Note: the write version doesn't have the issue since it will use the
override system which has the property to be deferred until we know all
the resources.

[Markus Roberts]

> >>> I think that at the same

>>> time we should add the ability to treat resources like hashes.  That
>>> is, the following syntax should work:
>>>
>>> file { "/foo": owner =>  luke }
>>>
>>> $owner = File["/foo"][owner]
>>>
>>> This treats the resource types like a hash, with the resource title
>>> as
>>> the indexer, which makes sense and helps tie together this otherwise
>>> somewhat confusing syntax with the new hash syntax.

 

>> Yes, it does. And I think this should be moderatly easy to add.

>> And of course this is only a view of the resource, there's no "write"
>> access to the resource parameter, correct?
>
> I don't see why not, really, as long as it goes through the standard
> parameter setting methods - then the overriding rules will still be
> followed, so it's just syntactic sugar for an override.

I started working on this, but I'm encountering an issue.
I started with the read version, ie:

$owner = File["/foo"][owner]

What I did is to create a new AST leaf which from a resource reference
aims to extract one parameter.

The issue is that at evaluation time the catalog might not know yet the
resource. So calling scope.compiler.find_resource( resource_ref ) will
certainly fail.

So some options off the top of my head (no sorting/filtering for quality):

* It could fail (you aren't allowed to do that, the resource must exist)
* It could evaluate to undef
* It could be a noop
* It could produce a future (http://en.wikipedia.org/wiki/Futures_and_promises)
* We could give up
* We could create a dependency or otherwise force the ordering
* We could suspend/defer (aka poor man's future)
* Do multiple passes until they all resolve
* Something else I haven't thought of

I won't say "I hope that helps" because the likelyhood isn't that great.  Ranking the options depends a lot on the shape of the envisioned use cases, which I'm not up to speed on.

-- Markus

[Luke Kanies]

Given the way the system works now, the only real choice is to fail -
someone's got an implicit dependency that they haven't declared.

It's just like someone trying to refer to the contents of a hash that
hasn't been set.

--
The time to repair the roof is when the sun is shining.
-- John F. Kennedy

[Markus Roberts]

Well, yes, but since the hash semantics are being defined at the same time...

A few other thoughts:

* You could argue that both access to an undefined parameter/property and reference to an unassigned hash element should mirror the semantics of undefined $vars, e.g. they are undef which (as noted on a sibling thread) is sort of like empty string.
* You could (and I just might be inclined to) argue either that reference to the properties of a resource was not allowed without an explicit dependency on that resource (even if the ordering by chance worked out) or that it was itself an implicit declaration of dependency and should be fully treated as one were.
* Are there any issues we're ignoring we respect to default values, overrides, and plussignment (e.g. ways that, depending on what we decide here, the results could be mysteriously order dependent)?
* What exactly are the use cases?  Everything I've managed to think up so far would be simpler to express with a variable or hash, rather than routing the value through another resource--though I can vaguely see how t might reflect the semantics a little better to refer directly to a value you wanted to synchronize with rather than referring to the value that was used to set it.
* Would it make sense to allow this mechanism to refer to unmanaged properties (such as the mode or owner of a file)?

-- Markus

[David Schmitt]

I have a harder question for you:

Given these these two classes:

class base { file { "/etc": mode => 0777 } }
class base::fixed inherits base { File["/etc"] { mode => 0755 } }

What is the $value in the following case:

include base
$value = File['/etc'][mode]
include base::fixed

The puppet dsl is designed to allow parse-time evaluation of assignments
by disallowing modification of already assigned variables. This makes
the language easier to understand, write and evaluate. Being able to
reference mutating values breaks many valuable assumptions. See also the
confusion/strangeness surrounding other parse-order dependent features
like defined() and tagged().

It'd be great if you could abolish the parse-order dependency
altogether. As long as all variable/value reference chains form a
directed acylcic graph, they could be evaluated after finishing the
parse, regardless of order.

I am aware though, that this would be *WAY* beyond the scope of simply
adding hashes to the dsl.

Regards, DavidS

[Luke Kanies]

On Nov 16, 2009, at 3:20 AM, David Schmitt wrote:

> I have a harder question for you:
>
> Given these these two classes:
>
> class base { file { "/etc": mode => 0777 } }
> class base::fixed inherits base { File["/etc"] { mode => 0755 } }
>
> What is the $value in the following case:
>
> include base
> $value = File['/etc'][mode]
> include base::fixed
>
>
> The puppet dsl is designed to allow parse-time evaluation of
> assignments
> by disallowing modification of already assigned variables. This makes
> the language easier to understand, write and evaluate. Being able to
> reference mutating values breaks many valuable assumptions. See also
> the
> confusion/strangeness surrounding other parse-order dependent features
> like defined() and tagged().
>
> It'd be great if you could abolish the parse-order dependency
> altogether. As long as all variable/value reference chains form a
> directed acylcic graph, they could be evaluated after finishing the
> parse, regardless of order.

Could you elaborate on this?

> I am aware though, that this would be *WAY* beyond the scope of simply
> adding hashes to the dsl.

This isn't so much beyond the scope of adding hashes (which it is) as
it is beyond my ability to know how the heck to do it.

I'm all for the this functionality, I've just no real idea how to do
it. The best I can think of is to do something like store a reference
to a variable and a scope during compilation, and then only deref them
when we're converting the catalog at the last minute.

I expect we'll be trying to hack our way to a solution to this at some
point in the next few months, but until someone has some kind of mad
inspiration, it's mostly going to be wandering around in the dark.

--
Hegel was right when he said that we learn from history that man can
never learn anything from history. -- George Bernard Shaw

[Luke Kanies]

This seems right.

> * You could (and I just might be inclined to) argue either that
> reference to the properties of a resource was not allowed without an
> explicit dependency on that resource (even if the ordering by chance
> worked out) or that it was itself an implicit declaration of
> dependency and should be fully treated as one were.

In this case it's just a dependency during compilation, which isn't
the same as a run-time dependency, and isn't as useful - if you can
find the resource, you don't need to declare the dependency, and if
you can't, you don't know what class you need to evaluate to get it.
You're in a bit of a chicken/egg situation.

> * Are there any issues we're ignoring we respect to default values,
> overrides, and plussignment (e.g. ways that, depending on what we
> decide here, the results could be mysteriously order dependent)?

Probably. :/

> * What exactly are the use cases? Everything I've managed to think
> up so far would be simpler to express with a variable or hash,
> rather than routing the value through another resource--though I can
> vaguely see how t might reflect the semantics a little better to
> refer directly to a value you wanted to synchronize with rather than
> referring to the value that was used to set it.

I think the main use case is that it intuitively should exist, and it
unifies the syntax that we're talking about adding. It would be
weirder if it didn't exist.

As to how people will (ab)use it, I'm not really sure, but I'm quite
confident that people will quickly use it everywhere.

> * Would it make sense to allow this mechanism to refer to unmanaged
> properties (such as the mode or owner of a file)?

Yes, but how is this different from your first point?

--
Tradition is what you resort to when you don't have the time or the
money to do it right. -- Kurt Herbert Alder

[Markus Roberts]

> > It'd be great if you could abolish the parse-order dependency

> altogether. As long as all variable/value reference chains form a
> directed acylcic graph, they could be evaluated after finishing the
> parse, regardless of order.

Could you elaborate on this?

> The best I can think of is to do something like store a reference
> to a variable and a scope during compilation, and then only deref
> them when we're converting the catalog at the last minute.

That's basically it; the more general form is to use the futures (aka promises) mentioned earlier in this thread.  Instead of stashing a (variable,scope) reference you create a placeholder object which either holds a value or the tree to evaluate to get that value.

After parsing is done (and the full forest is known) you fulfill all futures that contain values, than iteratively fulfill all futures which only depend on fulfilled values.  If it's a DAG, you're set.  If there are loops, you'll be left with unfulfillable nodes and it's an error.
 

-- Markus

[David Schmitt]

Yes. You make it sound so easy :-) I'm worried because this also affects
function evaluation including include() and template(), both of which
might alter the manifest significantly, including autoloading new stuff.

Not saying it can't be done, just that it is really a sweeping change
through many parts of the language.

Regards, DavidS

[Teyo Tyree]

This would no doubt change the behavior of function evaluation, but in return bring a higher level of consistency by removing some of the odd ordering/scope abuses that manifest developers sometimes employ.  No doubt it will require some folks to refactor their Puppet code.

Not saying it can't be done, just that it is really a sweeping change
through many parts of the language.

I wouldn't call the change sweeping as much as a targeted extraction of one of the more confusing and inconsistent aspects of the Puppet language.

Regards, DavidS

-teyo

[Luke Kanies]

Is this something you could reasonably implement without a lot of work?

--
It's not to control, but to protect the citizens of Singapore. In our
society, you can state your views, but they have to be correct.
-- Ernie Hai, co-ordinator of the Singapore Government
Internet Project

[Luke Kanies]

I think this is somewhat of a concern, but I really do think that the
vast majority of cases will just work correctly more easily.

As Markus has mentioned earlier, I think very people are intentionally
using the existing buggy behaviour, so I don't really think it'll be a
problem.

--
I don't deserve this award, but I have arthritis and I don't deserve
that either. -- Jack Benny

[Markus Roberts]

> > That's basically it; the more general form is to use the futures

> (aka promises) mentioned earlier in this thread.  Instead of
> stashing a (variable,scope) reference you create a placeholder
> object which either holds a value or the tree to evaluate to get
> that value.
>
> After parsing is done (and the full forest is known) you fulfill all
> futures that contain values, than iteratively fulfill all futures
> which only depend on fulfilled values.  If it's a DAG, you're set.
> If there are loops, you'll be left with unfulfillable nodes and it's
> an error.

Is this something you could reasonably implement without a lot of work?

In the very near future I hope to be able to either answer that in the affirmative or quietly drop the subject and metaphorically walk away from it whistling, with my hands in my pockets and a look of studied indifference on my face.

[Brice Figureau]

lol :-)

If you need help in this task, let me know. It's something I'm
interested in (not that I have lot of time to devote to Puppet dev right
now, but I can help implement things or review patches or simply discuss
implementation if you feel the need).

[Markus]

These two messages were sent yesterday afternoon and about 1:00 AM last
night (both times PST). They seem to have bounced/dropped from the
list, so I'm resending them from my non-gmail account.


Begin forwarded message:
> From: Markus Roberts <Mar...@reality.com>
> Date: November 19, 2009 7:28:26 AM PST
> To: puppe...@googlegroups.com
> Subject: Re: [Puppet-dev] Re: [PATCH/puppet 1/1] Fix #2389 - Enhance
> Puppet DSL with Hashes
>
>
> I've done two quick sketches so far, neither very promising.
>
> For the first I tried to build a placeholder value to return from
> Scope#lookupvar and tie to Scope#setvar -- that floundered because I
> (despite knowing better) got suckered in by the AST node classes
> (really expression nodes) and wound up not getting called until too
> late.
>
> Next, I tried adding a shim layer between the grammar's ast() node
> creation function and the actual ast nodes. That let me defer
> creation of the so-called AST nodes until the scope was known, but
> when I got to define I got tangled.
>
> I'm going to take a quick look as raising scope assignment from
> evaluate to something closer to the (real) AST layer, so that the
> scope graph is known by the time you get to evaluate. If you think of
> this phase as "instantiation" for most nodes it's just passing down
> the scope but for definitions (and, grep tells me, hostclass) it is
> actually building the scope tree--before any "evaluation" takes
> place. This may well be doomed too, but I won't know till I try.
>
> -- Markus
>
>
Begin forwarded message:
> From: Markus Roberts <Mar...@reality.com>
> Date: November 19, 2009 7:29:22 AM PST
> To: puppe...@googlegroups.com
> Subject: Re: [Puppet-dev] Re: [PATCH/puppet 1/1] Fix #2389 - Enhance
> Puppet DSL with Hashes
>
>
>
> Third time lucky. After two abortive attempts I've managed to get a
> rough sketch of what futures-based evaluation might look like in
> puppet. As a concrete example, consider this overly simplistic &
> contrived test case:
>
> $early = "this prints"
> notice($early)
>
> notice($late)
> $late = "this should too"
>
> In the present system the first notice works but the second just
> prints an empty string because $late hasn't been assigned a value
> yet. This is because statements are executed sequentially and
> variables are looked up in the symbol table as needed--if the value is
> there, fine; if not, it's :undef (or pulls the value from the
> enclosing scope).
>
> What I did was roughly:
>
> • Add a layer of objects between the grammar and the present AST nodes
> (by rewriting the ast method in parser_utils). These objects just
> represent the structure of the code and have no puppet-semantics, thus
> corresponding more closely to traditional AST nodes.
> • Added a Futures class to scopes, replacing the symbol tables with a
> system for managing them (in a given scope, a given variable has
> exactly one Future).
> class Future
> define_accessors :source,:resolved?,:scope,:name
> def initialize(scope,name)
> @scope,@name = scope,name
> end
> def value
> if resolved? then @value
> elsif source then resolved!; @value =
> source.evaluate(scope)
> elsif scope.parent then scope.parent.future_for(name).value
> else :undef
> end
> end
> end
>
> class No_future < Future
> def initialize(scope,name,message)
> super(scope,name)
> warning message
> end
> def value
> :undefined
> end
> end
>
> def future_for(name)
> if name =~ /(.*)::([^:]+)/
> klassname,varname = $1,$2
> if not (klass = find_hostclass(klassname))
> No_future.new(self,name,
> "Could not look up qualified variable '#{name}';"+
> " class #{klassname} could not be found"
> )
> elsif not (kscope = compiler.class_scope(klass))
> No_future.new(self,name,
> "Could not look up qualified variable '#{name}';"+
> " class #{klassname} has not been evaluated"
> )
> else
> kscope.future_for(varname)
> end
> else
> @futures[name] ||= Future.new(self,name)
> end
> end
> • Added an "instantiation" step in which the new AST structure
> produces a structure of "expression nodes" (really the old AST
> nodes--I didn't even rename them, though if we go this route they
> definitely should be renamed).
> • Modified the Variable and VarDef nodes to use Futures instead of
> Scope#lookupvar() and Scope#setvar(); variable references just use
> Future.value, while VarDefs set the future's source (provide there
> isn't already a source).
> • Disabled the lazy evaluation disabling as done by include so that
> includes are evaluated lazily (this may be a sticking point)
> • When this structure is evaluated, the first reference to $late in
> the example above causes the corresponding VarDef to be executed
> before continuing with the notify.
> It works, somewhat, but it breaks a lot of tests (mostly because they
> depended heavily on implementation details). There are also a raft of
> edge cases and details to clean up. However, I'm going to stop here
> for the moment because I've gotten what I came for:
>
> Yes, Luke, I think it's doable, if we want it.
>
> -- Markus
>
> P.S. The code's the software equivalent of a wire wrap, but if anyone
> wants to see it it's up at
>
> http://github.com/MarkusQ/puppet/tree/feature/0.25.x/futures
>
> P.P.S. This is all very experimental; do NOT try using this code for
> anything but exploration of the ideas.
>
>
----------------------- I'm a lumberjack and I'm OK

[Luke Kanies]

On Nov 19, 2009, at 7:57 AM, Markus wrote:

> These two messages were sent yesterday afternoon and about 1:00 AM
> last
> night (both times PST). They seem to have bounced/dropped from the
> list, so I'm resending them from my non-gmail account.

Especially after conversation today, seems like we've got an
implementation-worthy prototype. Once we get my extracted-definitions
branch done, we can start on this.

I'd really like to get this into 1.0 if at all possible.

--
There is no expedient to which a man will not go to avoid the labor
of thinking. --Thomas A. Edison