[dev.icinga.com #13273] support-for-functions-as-parameters

[icinga-migration]

This issue has been migrated from Redmine: https://dev.icinga.com/issues/13273

Created by lbetz on 2016-11-22 08:33:33 +00:00

Assignee: lbetz
Status: New
Target Version: (none)
Last Update: 2017-01-09 10:07:45 +00:00 (in Redmine)

---

fct(a,b) use © {...}
We could maybe have % [ $1.split().map ], but not for $2 or $3, since that would defeat the purpose of this...

[icinga-migration]

Updated by lbetz on 2016-11-22 08:58:16 +00:00

• Done % changed from 0 to 100

[icinga-migration]

Updated by bsheqa on 2016-11-23 13:00:26 +00:00

• Target Version set to v0.6.0
• Done % changed from 100 to 80

[icinga-migration]

Updated by bsheqa on 2016-11-23 13:07:30 +00:00

David on Github:

OK, I found a case (very early already) where the regex above is not working. I will try feeding this info into the corresponding ticket on dev.icinga.com. In case the function body contains an operator (e.g. +), the first regex matches and not the new one I was naively suggesting.

This e.g. is an example that does not work:

name => 'function () use (vhost) { "check-http-" + vhost }',

name = function () use ("vhost) { "check-http-"" + "vhost }"

[icinga-migration]

Updated by bsheqa on 2016-11-23 13:28:39 +00:00

• Target Version changed from v0.6.0 to v0.7.0

[icinga-migration]

Updated by kwisatz on 2016-11-23 16:53:56 +00:00

For the record, the regex I suggested was

if row =~ /^(?:.+)\((.*)\)\s*use\s*\((.*)\)\s*{(.*)}$/
  result += "function (%s) use (%s) { %s }" % [ $1.split(',').map {|x| parse(x.lstrip)}.join(', '), $2.strip, $3.strip ]
elsif row =~ /^(.+)\((.*)$/
[…]

And the test that went with it:

    context "#{os} with attrs => { foo => function (bar) use (var) { unparsed string } }" do
      let(:params) { {:attrs => { 'foo' => 'function  (bar) use (var)  {unparsed string}' }, :object_type => 'foo', :target => '/bar/baz', :order => '10'} }

      it { is_expected.to contain_concat__fragment('icinga2::object::foo::bar')
        .with_content(/foo = function \("bar"\) use \(var\) \{{1} unparsed string \}{1}\n/) }
    end

What it didn't test is function bodies that contain operators such as the plus sign, as mentioned in the comment above. In that case, the first regex

if row =~ /^(.+)\s([\+-]|\*|\/|==|!=|&&|\|{2}|in)\s(.+)$/
    result += "%s %s %s" % [ parse($1), $2, parse($3) ]

already matches and we never even get to the one I suggested. So this entire section needs to be rethought.
Since I finally figured out how to run individual specs with the puppet rspec rake task, I can see if I find something that works for all test cases.

(I actually need this to work in our infrastructure quite soon if I don't want to rewrite the entire thing using resource exports.)

[icinga-migration]

Updated by bsheqa on 2016-11-23 17:28:09 +00:00

This state can also be found in the branch feature/support-for-functions-as-parameters-13273

Is it sufficient for you to use the file ressource and tag it with @ icinga2::config::file@?
Like described here: https://github.com/Icinga/puppet-icinga2-rewrite#custom-configuration-files

[icinga-migration]

Updated by kwisatz on 2016-11-24 09:24:11 +00:00

bsheqa wrote:

This state can also be found in the branch feature/support-for-functions-as-parameters-13273

Is it sufficient for you to use the file ressource and tag it with @ icinga2::config::file@?
Like described here: https://github.com/Icinga/puppet-icinga2-rewrite#custom-configuration-files

Haven't tried it yet, but I will.

Taking that into account, are you still considering adding support for function parameters to the Object resource or not? I have made some progress with modified parse() and value_types() methods that only fail on two test cases, one of which I don't understand and the other I haven't looked into yet:

Here I don't see the difference between expected and actual, really (Note that this is a spec I added):

        -(?-mix:foo = function \(bar\) use \(var\) \{{1} "baz" + "zab" \}{1}\n)
        +
        +object foo "bar"  {
        +  foo = function (bar) use (var) { "baz" + "zab" }
        +}

And this one I haven't yet looked into in detail:

        -(?-mix:ignore where NodeName != "baz" \|{2} !host.display_name)
        +
        +object foo "bar"  {
        +  ignore where "NodeName" != "baz" || !host.display_name
        +}

[icinga-migration]

Updated by kwisatz on 2016-11-24 09:37:38 +00:00

bsheqa wrote:

Is it sufficient for you to use the file ressource and tag it with @ icinga2::config::file@?
Like described here: https://github.com/Icinga/puppet-icinga2-rewrite#custom-configuration-files

This works for me, yes. But of course it relies on me for syntax checks ;)

[icinga-migration]

Updated by kwisatz on 2016-11-24 16:08:58 +00:00

OK, the second failing test now passed, I had removed a condition to be able to test this offline (where the specs only run when connected to the Internet…) but the first is still baffling and I'll continue looking.

[icinga-migration]

Updated by kwisatz on 2016-11-24 18:02:23 +00:00

OK, I don't get those tests, sorry.

Here's the code I came up with, I'm sure it can be refactored to be more concise. For now I will focus on finishing our set-up using the custom configuration files, but I'll try to be of as much help as possible in the future.
The idea here is that variables used inside a function body should be unquoted if they have been passed in as either function arguments or function context and quoted in any other case.

def parse(row, vars = [], context = [])
  result = ''
  puts "Analyzing '#{row}'"
  puts "Vars is currently #{vars}, Context is #{context}"
  if row =~ /^(?:.+)\((.*)\)\s*use\s*\((.*)\)\s*{(.*)}$/
    vars = $1.split(',').map {|x| x.lstrip}
    context = $2.split(',').map {|x| x.strip}
    body = parse($3.strip, vars, context)
    result += "function (%s) use (%s) { %s }" % [ vars.join(', '), context.join(', '), body ]
  elsif row =~ /^(?:.+)\((.*)\)\s*{(.*)}$/
    vars = $1.split(',').map {|x| x.lstrip}
    body = parse($2.strip, vars, context)
    result += "function (%s) { %s }" % [ vars.join(', '), body ]
  elsif row =~ /^(.+)\s([\+-]|\*|\/|==|!=|&&|\|{2}|in)\s(.+)$/
    result += "%s %s %s" % [ parse($1, vars, context), $2, parse($3, vars, context) ]
  else
    if row =~ /^(.+)\((.*)$/
      result += "%s(%s" % [ $1, $2.split(',').map {|x| parse(x.lstrip, vars, context)}.join(', ') ]
    elsif row =~ /^(.*)\)$/
      result += "%s)" % [ $1.split(',').map {|x| parse(x.lstrip, vars, context)}.join(', ') ]
    elsif row =~ /^\((.*)$/
      result += "(%s" % [ parse($1, vars, context) ]
    else
      result += value_types(row.to_s, vars, context)
    end
  end

  return result.gsub(/" in "/, ' in ')
end

def value_types(value, vars = [], context = [])
  if value =~ /^\d+\.?\d*[dhms]?$/ || value =~ /^(true|false)$/ || value =~ /^"(.*)"$/
    result = value
  else
    check = context + vars
    check = !check.empty? ? check.map {|x| Regexp.escape(x)}.join('|') : ''
    if !check.empty? && value =~ /^#{check}$/
      result = value
    elsif value =~ /^!?(host|service|user)\./ || value =~ /^\{{2}.*\}{2}$/
      result = value
    else
      result = "\"#{value}\""
    end
  end

  return result
end


def attribute_types(attr)
  if attr =~ /^[a-zA-Z0-9_]+$/
    result = attr
  else
    result = "\"#{attr}\""
  end

  return result
end

# "Manual" testing ;)
puts parse('{{ foo }}') + "\n\n"
puts parse('function () { bla }') + "\n\n"
puts parse('function (bla) { bla }') + "\n\n"
puts parse('function () use () { bla }') + "\n\n"
puts parse('function () use (var) { foo + bla }') + "\n\n"
puts parse('function () use (var1, var2) { foo + bla }') + "\n\n"
puts parse('function () use (var) { "foo" + bla || baz }') + "\n\n"
puts parse('function () use (bar) { foo + bar || baz }') + "\n\n"
puts parse('function (baz) use (bar) { foo + bar || baz }') + "\n\n"
puts parse('function (baz) use (bar, fuu) { foo + bar || (baz && fuu) }') + "\n\n"

[icinga-migration]

Updated by lbetz on 2016-12-10 11:23:31 +00:00

• Target Version changed from v0.7.0 to v0.8.0

[icinga-migration]

Updated by bsheqa on 2016-12-29 07:44:54 +00:00

• Target Version changed from v0.8.0 to 343

[icinga-migration]

Updated by bsheqa on 2017-01-09 10:07:45 +00:00

• Target Version deleted 343

[bobapple]

@kwisatz @lbetz is this still of interest?

[kwisatz]

Honestly, I think that stressing that "very complex" behavior in host or service objects should not be realized in Puppet but using tagged files instead. Ok, you're losing the syntax checks puppet offers, but icinga2 will catch those kind of errors.
I can see that the complexity far outweighs the usefulness in this case.
I'm not saying it should never be done, or eventually accepted as a PR, but it's definitely nothing of high priority.

[bobapple]

I agree with that. I will close this issue and remove the branch.