Skip to content

(PUP-7174) Add 'call' to the Puppet built in language functions#5609

Merged
hlindberg merged 1 commit intopuppetlabs:masterfrom
seanmil:call_function
Jun 2, 2017
Merged

(PUP-7174) Add 'call' to the Puppet built in language functions#5609
hlindberg merged 1 commit intopuppetlabs:masterfrom
seanmil:call_function

Conversation

@seanmil
Copy link
Contributor

@seanmil seanmil commented Feb 3, 2017

Per PUP-7174

@hlindberg
Copy link
Contributor

Tests are failing with this error:

the call method should be callable as call on a Puppet language function with no arguments
     Failure/Error: raise ArgumentError, "Function #{self.class.name}(): Unknown function: '#{function_name}'" unless func_3x
     Puppet::PreformattedError:
       Evaluation Error: Error while evaluating a Function Call, Function call(): Unknown function: 'call_me' at line 2:16 on node test


it 'call on a Puppet language function with no arguments' do
expect(compile_to_catalog(<<-CODE)).to have_resource('Notify[called]')
function call_me() { 'called' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to take - not sure why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the bug I was referring to on Slack that I thought I had found. I've tested the call function in a full environment and it works for namedspaced Puppet language functions. However, it does not work for a non-namespaced Puppet language function. And that seems like a bug in internal_call_function to me. Based on my reading of the code and the API notes it sounds like internal_call_function should work equally for all types of functions. The loader seems unable to find it, but how/why that occurs I don't know. I started digging into the pops code, but quickly got lost.

@puppetcla
Copy link

CLA signed by all contributors.

@seanmil seanmil changed the title Add 'call' to the Puppet built in language functions (PUP-7174) Add 'call' to the Puppet built in language functions Feb 5, 2017
@seanmil
Copy link
Contributor Author

seanmil commented Feb 9, 2017

@hlindberg, I spent a bit more time looking into this one spec test failing and saw what I considered to be some surprising behavior. This is all tested on 2016.4.2

If I take an empty environment (without the call function patch) and define this in manifests/site.pp:

function mainfunc() { 'main manifest function' }
include test

and then create modules/test/manifests/init.pp with:

class test() {
    notify { mainfunc(): }
}

Then I get Error 500 on SERVER: Server Error: Evaluation Error: Unknown function: 'mainfunc' error.

According to https://docs.puppet.com/puppet/4.7/lang_write_functions_in_puppet.html#aside-writing-functions-in-the-main-manifest I believe that it should work.

Also interestingly, if I do: puppet apply -e "function foo() { 'FOO' } ; class test { notify { foo(): } } ; include test" it works and I get:

Notice: FOO
Notice: /Stage[main]/Test/Notify[FOO]/message: defined 'message' as 'FOO'
Notice: Applied catalog in 0.05 seconds

It also works in the agent/master scenario if I call the function directly in site.pp, but not from the module.

I feel like I am hitting the same (or a very similar) issue in the spec test. Given how simple the call function is, and that I'm seeing some odd behavior here, I don't feel it is a problem in the function itself, but rather something else (maybe the loader?)

I think I've hit the limit of what I can diagnose. Any ideas?

@seanmil
Copy link
Contributor Author

seanmil commented Jun 2, 2017

I'm not sure why the appveyor build failed - it looks to be an unrelated problem. However, I have refactored the spec tests a bit and they are all passing for all the original test cases.

@hlindberg
Copy link
Contributor

@seanmil the appveyor tests are failing because of a bug in a recent version of AppVeyor (errors related to UNC paths) - there are 4 failing tests (which we ignore for now).

@hlindberg hlindberg merged commit 7dcdbda into puppetlabs:master Jun 2, 2017
@hlindberg
Copy link
Contributor

Thanks @seanmil for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants