Giuseppe Lavagetto has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/402345 )
Change subject: wmflib: simplify the role() function, convert to the new API
......................................................................
wmflib: simplify the role() function, convert to the new API
Converting to the new API makes some things possible, namely:
- Clean access to the top scope
- Cleanly require the function is called exactly once
- Use call_function for 'include'
but on the other hand makes it impossible to verify if we're calling the
function from the node scope. Please note that this happens for a reason
- messing with the scope in function can cause undefined behaviour and
indeed I found edge cases where this can happen, for instance when
trying to access the variables before they're defined can make them null.
Change-Id: I32631c84a7f6340a0d7de2ae57dfbef87015c46b
---
A modules/wmflib/lib/puppet/functions/role.rb
D modules/wmflib/lib/puppet/parser/functions/role.rb
M modules/wmflib/spec/functions/role_spec.rb
3 files changed, 73 insertions(+), 101 deletions(-)
Approvals:
Giuseppe Lavagetto: Looks good to me, approved
jenkins-bot: Verified
diff --git a/modules/wmflib/lib/puppet/functions/role.rb
b/modules/wmflib/lib/puppet/functions/role.rb
new file mode 100644
index 0000000..877624e
--- /dev/null
+++ b/modules/wmflib/lib/puppet/functions/role.rb
@@ -0,0 +1,54 @@
+# == Function: role ( string $role_name [, string $... ] )
+#
+# Declare the _roles variable (or add keys to it), and if the role class
+# role::$role_name is in the scope, include it. This is roughly a
+# shortcut for
+#
+# $::_roles[foo] = true
+# include role::foo
+#
+# and has the notable advantage over that the syntax is shorter. Also,
+# this function will refuse to run anywhere but at the node scope,
+# thus making any additional role added to a node explicit.
+#
+# If you have more than one role to declare, you MUST do that in one
+# single role stanza, or you would encounter unexpected behaviour. If
+# you do, an exception will be raised.
+#
+# This function is very useful with our "role" hiera backend if you
+# have global configs that are role-based
+#
+# === Example
+#
+# node /^www\d+/ {
+# role mediawiki::appserver # this will load the
role::mediawiki::appserver class
+# include ::standard #this class will use hiera lookups defined for the
role.
+# }
+#
+# node monitoring.local {
+# role icinga, ganglia::collector #GOOD
+# }
+#
+# node monitoring2.local {
+# role icinga
+# role ganglia::collector #BAD, issues a warning
+# }
+Puppet::Functions.create_function(:role) do
+ dispatch :main do
+ param 'String', :main_role
+ end
+ def main(main_role)
+ scope = closure_scope
+ # Check if the function has already been called
+ if scope.include? '_role'
+ raise Puppet::ParseError, "role() can only be called once per node"
+ end
+ role = main_role.gsub(/^::/, '')
+ # Backwards compat
+ scope['_roles'] = { role => true }
+ # This is where we're going in the future
+ scope['_role'] = role
+ rolename = 'role::' + role
+ call_function('include', rolename)
+ end
+end
diff --git a/modules/wmflib/lib/puppet/parser/functions/role.rb
b/modules/wmflib/lib/puppet/parser/functions/role.rb
deleted file mode 100644
index 971782b..0000000
--- a/modules/wmflib/lib/puppet/parser/functions/role.rb
+++ /dev/null
@@ -1,85 +0,0 @@
-# == Function: role ( string $role_name [, string $... ] )
-#
-# Declare the _roles variable (or add keys to it), and if the role class
-# role::$role_name is in the scope, include it. This is roughly a
-# shortcut for
-#
-# $::_roles[foo] = true
-# include role::foo
-#
-# and has the notable advantage over that the syntax is shorter. Also,
-# this function will refuse to run anywhere but at the node scope,
-# thus making any additional role added to a node explicit.
-#
-# If you have more than one role to declare, you MUST do that in one
-# single role stanza, or you would encounter unexpected behaviour. If
-# you do, an exception will be raised.
-#
-# This function is very useful with our "role" hiera backend if you
-# have global configs that are role-based
-#
-# === Example
-#
-# node /^www\d+/ {
-# role mediawiki::appserver # this will load the
role::mediawiki::appserver class
-# include ::standard #this class will use hiera lookups defined for the
role.
-# }
-#
-# node monitoring.local {
-# role icinga, ganglia::collector #GOOD
-# }
-#
-# node monitoring2.local {
-# role icinga
-# role ganglia::collector #BAD, issues a warning
-# }
-
-module Puppet::Parser::Functions
- newfunction(:role, :arity => -2) do |args|
- # This will add to the catalog, and to the node specifically:
- # - A global 'role' hash with the 'role::#{arg}' key set to true;
- # if the variable is present, append to it
- # - Include class role::#{arg} if present
-
- # Prevent use outside of node definitions
- unless is_nodescope?
- raise Puppet::ParseError,
- "role can only be used in node scope, while you are in scope
#{self}"
- end
-
- # Now check if the variable is already set and issue a warning
- container = '_roles'
- rolevar = compiler.topscope.lookupvar(container)
- if rolevar
- raise Puppet::ParseError,
- "Using 'role' multiple times might yield unexpected results, use
'role role1, role2' instead"
- else
- compiler.topscope.setvar(container, {})
- rolevar = compiler.topscope.lookupvar(container)
- end
-
- # sanitize args
- args = args.map do |x|
- if x.start_with? '::'
- x.gsub(/^::/, '')
- else
- x
- end
- end
-
- # Set the variables
- args.each do |arg|
- rolevar[arg] = true
- end
-
- args.each do |arg|
- rolename = 'role::' + arg
- role_class = compiler.topscope.find_hostclass(rolename)
- if role_class
- send Puppet::Parser::Functions.function(:include), [rolename]
- else
- raise Puppet::ParseError, "Role class #{rolename} not found"
- end
- end
- end
-end
diff --git a/modules/wmflib/spec/functions/role_spec.rb
b/modules/wmflib/spec/functions/role_spec.rb
index b350208..5bb643c 100644
--- a/modules/wmflib/spec/functions/role_spec.rb
+++ b/modules/wmflib/spec/functions/role_spec.rb
@@ -6,35 +6,38 @@
let(:pre_condition) {"
class role::test {}
-class role::test2 {}
+class role::test2 {
+ if ($::_roles['test2']) { file { '/tmp/test': ensure => present} }
+ file{ \"/tmp/${::_role}\": ensure => present}
+}
"}
it "should be called with one parameter" do
should run.with_params.and_raise_error(ArgumentError)
end
- it "throws error if called outside of the node scope" do
- should run.with_params('cache::text').and_raise_error(Puppet::ParseError)
- end
it "throws error if called on a non-existing role" do
- is_expected.to
run.with_params('foo::bar').and_raise_error(Puppet::ParseError)
+ is_expected.to run.with_params('foo::bar').and_raise_error(Puppet::Error)
end
it "includes the role class" do
is_expected.to run.with_params('test')
+ expect(catalogue).to contain_class('role::test')
end
- it "raises an error when called more than once in a scope" do
- scope.function_role(['test2'])
- expect { scope.function_role(['test']) }.to raise_error(Puppet::ParseError)
+ it "adds the keys to the top-scope variables" do
+ is_expected.to run.with_params('test2')
+ expect(catalogue).to contain_file('/tmp/test')
+ expect(catalogue).to contain_file('/tmp/test2')
end
- it "adds the keys to the top-scope variable" do
- scope.function_role(['test', 'test2'])
- expect(scope.lookupvar('_roles')).to eq({'test' => true, 'test2' => true})
- end
-
- it "includes the role classes" do
- scope.function_role(['test'])
- expect(scope.find_hostclass('role::test')).to
be_an_instance_of(Puppet::Resource::Type)
+ context 'function has already been called' do
+ let(:pre_condition) {"
+class role::test {}
+class role::test2 {}
+role('test')
+" }
+ it "raises an error when called more than once in a scope" do
+ is_expected.to run.with_params('test2').and_raise_error(Puppet::Error)
+ end
end
end
--
To view, visit https://gerrit.wikimedia.org/r/402345
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I32631c84a7f6340a0d7de2ae57dfbef87015c46b
Gerrit-PatchSet: 5
Gerrit-Project: operations/puppet
Gerrit-Branch: production
Gerrit-Owner: Giuseppe Lavagetto <glavage...@wikimedia.org>
Gerrit-Reviewer: Alexandros Kosiaris <akosia...@wikimedia.org>
Gerrit-Reviewer: Giuseppe Lavagetto <glavage...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
