[PATCH/puppet 1/1] Fix #2818 - scope variable assigned with undef are not "undef"
Brice Figureau
I'm sending this patch to start a discussion on the subject.
Notably, what I want to determine is how undef assigned variables compare
to undefined variable (those never assigned).
Please review and discuss,
Brice
Original commit msg:
The following manifest doesn't work:
$foo = undef
case $foo {
undef: { notice("undef") }
default: { notice("defined") }
}
This is because "undef" scope variable are returned as an empty
string.
This patch introduces a subtile behavior change which might produce
some issues in existing manifests.
Note, unassigned variable usage still returns an empty string "" as
before (which is I think what people would expect), so:
case $bar {
undef: { notice("undef") }
default: { notice("defined") }
}
won't print "undef".
Signed-off-by: Brice Figureau <brice-...@daysofwonder.com>
---
lib/puppet/parser/ast/leaf.rb | 5 ++++-
spec/unit/parser/ast/comparison_operator.rb | 4 ++--
spec/unit/parser/ast/leaf.rb | 17 +++++++++++++++++
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/lib/puppet/parser/ast/leaf.rb b/lib/puppet/parser/ast/leaf.rb
index 153120a..e1157e8 100644
--- a/lib/puppet/parser/ast/leaf.rb
+++ b/lib/puppet/parser/ast/leaf.rb
@@ -147,7 +147,10 @@ class Puppet::Parser::AST
# not include syntactical constructs, like '$' and '{}').
def evaluate(scope)
parsewrap do
- return scope.lookupvar(@value)
+ if (var = scope.lookupvar(@value, false)) == :undefined
+ var = ""
+ end
+ var
end
end
end
diff --git a/spec/unit/parser/ast/comparison_operator.rb b/spec/unit/parser/ast/comparison_operator.rb
index 2631179..9740243 100755
--- a/spec/unit/parser/ast/comparison_operator.rb
+++ b/spec/unit/parser/ast/comparison_operator.rb
@@ -71,8 +71,8 @@ describe Puppet::Parser::AST::ComparisonOperator do
one = Puppet::Parser::AST::Variable.new( :value => "one" )
two = Puppet::Parser::AST::Variable.new( :value => "two" )
- @scope.expects(:lookupvar).with("one").returns(1)
- @scope.expects(:lookupvar).with("two").returns(2)
+ @scope.expects(:lookupvar).with("one", false).returns(1)
+ @scope.expects(:lookupvar).with("two", false).returns(2)
operator = Puppet::Parser::AST::ComparisonOperator.new :lval => one, :operator => "<", :rval => two
operator.evaluate(@scope).should == true
diff --git a/spec/unit/parser/ast/leaf.rb b/spec/unit/parser/ast/leaf.rb
index e968150..1d002d6 100755
--- a/spec/unit/parser/ast/leaf.rb
+++ b/spec/unit/parser/ast/leaf.rb
@@ -149,6 +149,23 @@ describe Puppet::Parser::AST::Regex do
end
end
+describe Puppet::Parser::AST::Variable do
+ before :each do
+ @scope = stub 'scope'
+ @var = Puppet::Parser::AST::Variable.new(:value => "myvar")
+ end
+
+ it "should lookup the variable in scope" do
+ @scope.expects(:lookupvar).with("myvar", false).returns(:myvalue)
+ @var.safeevaluate(@scope).should == :myvalue
+ end
+
+ it "should return an empty string if the variable wasn't set" do
+ @scope.expects(:lookupvar).with("myvar", false).returns(:undefined)
+ @var.safeevaluate(@scope).should == ""
+ end
+end
+
describe Puppet::Parser::AST::HostName do
before :each do
@scope = stub 'scope'
--
1.6.5.2
Markus Roberts
before (which is I think what people would expect), so:
case $bar {
undef: { notice("undef") }
default: { notice("defined") }
}
won't print "undef".
This is a sticking point for me. Playing with various cases with and without your patch, I'd like to have this:
$foo = undef
case $foo {
undef: { notice("foo undef") }
default: { notice("foo defined") }
}
case $bar {
"": { notice("bar blank") }
undef: { notice("bar undef") }
default: { notice("bar defined") }
}
print the same thing for both foo and bar. undef would be best, but if that isn't practical (too much breakage of existing manifests) then I'd say leave it alone rather than make undef mean something different than "undefined".
-- Markus
Peter Meier
> string.
Please consider that this will be a behavior change, which should be
announced in the release notes etc.
We currently do a lot of checks on "" to check wether a variable have
been set or not:
case $sshd_tcp_forwarding {
'': { $sshd_tcp_forwarding = 'no' }
}
I assume that after this patch this won't work anymore and we have to
add everywhere a check on undef. This isn't really a problem, but the
break in the behavior should be noted and communicated.
cheers pete
Brice Figureau
>
>> This is because "undef" scope variable are returned as an empty
>> string.
>
> Please consider that this will be a behavior change, which should be
> announced in the release notes etc.
That was the whole point of this patch: discuss it and see if we adopt
the behavior change or not.
> We currently do a lot of checks on "" to check wether a variable have
> been set or not:
>
> case $sshd_tcp_forwarding {
> '': { $sshd_tcp_forwarding = 'no' }
> }
>
> For example:
> http://git.puppet.immerda.ch/?p=module-sshd.git;a=blob;f=manifests/init.pp;h=8489a6a6b1a9047379aac942601f10596066522e;hb=7c73e1de97f872ba4c6651b9fa38599a9090311c
>
> I assume that after this patch this won't work anymore and we have to
> add everywhere a check on undef. This isn't really a problem, but the
> break in the behavior should be noted and communicated.
Sure. If the patch get approved it will be part of Rowlf so we have to
make sure it is noted in the "incompatible changes" section of the
release notes.
--
Brice Figureau
My Blog: http://www.masterzen.fr/
Brice Figureau
In fact I was wrong.
The current patch has the following:
* non-assigned variable when accessed *still* returns ''
* undef assigned variable when accessed returns undef instead of ''
Currently your manifests would still work.
Markus objected that both cases should return undef, which is what I had
implemented first, but since it was a behavior change, I made sure we
return '' on un-assigned variable.
Obviously the thing that this patch fixes is only a tiny part of the
story (I'm not even sure people assigns undef to variable in real
manifests).
So I want to start a discussion about this behavior change...
Peter Meier
> implemented first, but since it was a behavior change, I made sure we
> return '' on un-assigned variable.
>
> Obviously the thing that this patch fixes is only a tiny part of the
> story (I'm not even sure people assigns undef to variable in real
> manifests).
>
> So I want to start a discussion about this behavior change...
actually I think it's more intuitive to return undef than an empty
string. I mean undef is somehow the nil of puppet ;) not?
cheers pete
Luke Kanies
I've been thinking the same thing in the thread - that we should s/
undef/nil/g, and formally define what it means in the language.
'undef' was originally meant to be very minimal, but the 'nil' concept
is more useful than just undefining attributes.
--
Every great advance in natural knowledge has involved the absolute
rejection of authority. --Thomas H. Huxley
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com
Luke Kanies
I agree that these should be equal.
As already mentioned, there are some significant backward
compatibility concerns, but it's probably worth making the change.
We could also have undef be considered equal to an empty string, like
perl essentially does, which would mitigate most of the issues, I think.
--
It's a small world, but I wouldn't want to paint it.
-- Stephen Wright
Markus Roberts
* Undefined variables, variables set to undef (and possibly unassigned hash elements) return undef.
* In case expressions, undef values matches both undef and "" branches, but undef branches only match undef values.
* In interpolation, undef produces "".
Thus:
case $sshd_tcp_forwarding {
'': { $sshd_tcp_forwarding = 'no' }
}
$foo = undef
case $foo {
"": { notice("foo blank") }
default: { notice("foo defined") }
}
case $bar {
default: { notice("bar defined") }
}
$foo = undef
case $foo {
undef: { notice("foo undef") }
default: { notice("foo defined") }
}
case $bar {
"": { notice("bar blank") }
undef: { notice("bar undef") }
default: { notice("bar defined") }
}
Hmm. Order dependency in case statements seems counter to the spirit of puppet--is it? It could be removed by saying:
* Undefined variables, variables set to undef, (and possibly unassigned hash elements) return undef.
* In case expressions, undef is treated as "".
* In interpolation, undef produces "".
But then are there any contexts where it makes sense (or would even be possible) to distinguish "" and undef?
-- Markus
Thomas Bellman
> Obviously the thing that this patch fixes is only a tiny part of the
> story (I'm not even sure people assigns undef to variable in real
> manifests).
Since it doesn't really *work* to set a variable to undef, of course
we don't... But yes, I would like that to work properly.
My immediate use case is that I want to do something like this:
case $mode {
"normal": { $stopped = stopped; $running = running }
"chroot": { $stopped = undef; $running = undef }
}
...
service {
"whatever-1": enable => true, ensure => $running;
"whatever-2": enable => false, ensure => $stopped;
}
When I run in a chroot environment, either in %post in kickstart
or when I run Puppet on a complete OS image that I'm going to deploy
on new machines using System Imager, I don't want to manage the
processes what processes are running. (The $mode variable I would
set using a facter environment variable.)
Another use case is in definitions, where I want to provide a default
behaviour for some parameter, but that behaviour may depend on some
other parameters, so it wouldn't be proper to have a fixed default
value for that parameter. Currently I solve that by giving it a
default value that is very unlikely to be used as a real value, but
it would be much nicer to actually be able to check for undef.
/Bellman
Luke Kanies
This seems like a good solution.
> Hmm. Order dependency in case statements seems counter to the
> spirit of puppet--is it? It could be removed by saying:
>
> * Undefined variables, variables set to undef, (and possibly
> unassigned hash elements) return undef.
> * In case expressions, undef is treated as "".
> * In interpolation, undef produces "".
Order-dependent case statements are counter, but with regexes, they're
here now, so the cat's essentially out of the bag.
> But then are there any contexts where it makes sense (or would even
> be possible) to distinguish "" and undef?
I can't imagine any in the compiler, and undef values don't make it
out of the compiler.
--
Don't hit at all if it is honorably possible to avoid hitting; but
never hit soft! -- Theodore Roosevelt
Brice Figureau
case $foo {
default: { notice("defined") }
}
string.
Now, unassigned variable usage returns also undef.
This might produce some issues in existing manifests, although
care has been taken to allow correct behavior in the most commonly
used patterns.
For instance:
undef: { notice("undef") }
default: { notice("defined") }
}
But matching undef in case/selector/if will also match "".
case $bar {
"": { notice("empty") }
default: { notice("defined") }
}
will print "empty".
Of course "" doesn't match undef :-)
lib/puppet/parser/ast/leaf.rb | 7 +++-
spec/unit/parser/ast/arithmetic_operator.rb | 4 +-
spec/unit/parser/ast/comparison_operator.rb | 4 +-
spec/unit/parser/ast/leaf.rb | 49 +++++++++++++++++++++++---
test/language/ast/variable.rb | 2 +-
5 files changed, 54 insertions(+), 12 deletions(-)
diff --git a/lib/puppet/parser/ast/leaf.rb b/lib/puppet/parser/ast/leaf.rb
index b73c781..7b933c3 100644
--- a/lib/puppet/parser/ast/leaf.rb
+++ b/lib/puppet/parser/ast/leaf.rb
@@ -16,6 +16,8 @@ class Puppet::Parser::AST
if ! options[:sensitive] && obj.respond_to?(:downcase)
obj = obj.downcase
end
+ # "" == undef for case/selector/if
+ return true if obj == "" and value == :undef
obj == value
end
@@ -147,7 +149,10 @@ class Puppet::Parser::AST
def evaluate(scope)
parsewrap do
- return scope.lookupvar(@value)
+ if (var = scope.lookupvar(@value, false)) == :undefined
+ var
end
end
index 4a0be48..ad8d994 100755
--- a/spec/unit/parser/ast/arithmetic_operator.rb
+++ b/spec/unit/parser/ast/arithmetic_operator.rb
@@ -61,8 +61,8 @@ describe Puppet::Parser::AST::ArithmeticOperator do
end
it "should work for variables too" do
- @scope.expects(:lookupvar).with("two").returns(2)
+ @scope.expects(:lookupvar).with("one", false).returns(1)
+ @scope.expects(:lookupvar).with("two", false).returns(2)
two = ast::Variable.new( :value => "two" )
diff --git a/spec/unit/parser/ast/comparison_operator.rb b/spec/unit/parser/ast/comparison_operator.rb
index 2631179..9740243 100755
--- a/spec/unit/parser/ast/comparison_operator.rb
+++ b/spec/unit/parser/ast/comparison_operator.rb
@@ -71,8 +71,8 @@ describe Puppet::Parser::AST::ComparisonOperator do
one = Puppet::Parser::AST::Variable.new( :value => "one" )
two = Puppet::Parser::AST::Variable.new( :value => "two" )
- @scope.expects(:lookupvar).with("one").returns(1)
- @scope.expects(:lookupvar).with("two").returns(2)
+ @scope.expects(:lookupvar).with("one", false).returns(1)
+ @scope.expects(:lookupvar).with("two", false).returns(2)
operator = Puppet::Parser::AST::ComparisonOperator.new :lval => one, :operator => "<", :rval => two
operator.evaluate(@scope).should == true
diff --git a/spec/unit/parser/ast/leaf.rb b/spec/unit/parser/ast/leaf.rb
--- a/spec/unit/parser/ast/leaf.rb
+++ b/spec/unit/parser/ast/leaf.rb
@@ -21,6 +21,7 @@ describe Puppet::Parser::AST::Leaf do
end
it "should match values by equality" do
+ @value.stubs(:==).returns(false)
@leaf.stubs(:safeevaluate).with(@scope).returns(@value)
@value.expects(:==).with("value")
@@ -33,6 +34,12 @@ describe Puppet::Parser::AST::Leaf do
@leaf.evaluate_match("value", @scope, :insensitive => true)
end
+
+ it "should match undef if value is an empty string" do
+ @leaf.stubs(:safeevaluate).with(@scope).returns("")
+
+ @leaf.evaluate_match(:undef, @scope).should be_true
+ end
end
describe "when converting to string" do
@@ -72,12 +79,18 @@ describe Puppet::Parser::AST::String do
end
end
-describe Puppet::Parser::AST::Variable do
- describe "when converting to string" do
- it "should transform its value to a variable" do
- value = stub 'value', :is_a? => true, :to_s => "myvar"
- Puppet::Parser::AST::Variable.new( :value => value ).to_s.should == "\$myvar"
- end
+describe Puppet::Parser::AST::Undef do
+ @scope = stub 'scope'
+ end
+
+ it "should match undef with undef" do
+ @undef.evaluate_match(:undef, @scope).should be_true
+ end
+
+ it "should not match undef with an empty string" do
+ @undef.evaluate_match("", @scope).should be_false
end
end
@@ -158,6 +171,30 @@ describe Puppet::Parser::AST::Regex do
end
+describe Puppet::Parser::AST::Variable do
+ before :each do
+ @scope = stub 'scope'
+ @var = Puppet::Parser::AST::Variable.new(:value => "myvar")
+ end
+
+ it "should lookup the variable in scope" do
+ @scope.expects(:lookupvar).with("myvar", false).returns(:myvalue)
+ @var.safeevaluate(@scope).should == :myvalue
+ end
+
+ end
+
+ describe "when converting to string" do
+ it "should transform its value to a variable" do
+ value = stub 'value', :is_a? => true, :to_s => "myvar"
+ Puppet::Parser::AST::Variable.new( :value => value ).to_s.should == "\$myvar"
+ end
+end
+
describe Puppet::Parser::AST::HostName do
before :each do
@scope = stub 'scope'
index 17e078b..49b1dbb 100755
--- a/test/language/ast/variable.rb
+++ b/test/language/ast/variable.rb
@@ -22,7 +22,7 @@ class TestVariable < Test::Unit::TestCase
end
def test_evaluate
- assert_equal("", @var.evaluate(@scope), "did not return empty string on unset var")
+ assert_equal(:undef, @var.evaluate(@scope), "did not return :undef on unset var")
@scope.setvar(@name, "something")
assert_equal("something", @var.evaluate(@scope), "incorrect variable value")
end
--
1.6.5.2
Brice Figureau
I was looking at my assigned patches, and it seems this one had never
been reviewed (it's the v2).
If someone can have a look, and (even) try it, that'd be great :-)
thanks,
Brice
--
Luke Kanies
> --
>
> 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=
> .
>
>
--
When one admits that nothing is certain one must, I think, also admit
that some things are much more nearly certain than others.
-- Bertrand Russell