variable not resolved in native provider "concat"

[PierreR]

I have just updated the nginx module from jfryman from 0.0.9 to 0.0.10.

I have an error that really looks like a bug:

Here is the error:

Could not resolve variable $nginx::config::conf_dir in context nginx::config or root at # "./modules/nginx/manifests/resource/upstream.pp" (line 66, column 12)
puppetresources: error!

This is related to this line: https://github.com/jfryman/puppet-nginx/blob/master/manifests/resource/upstream.pp#L71

I don't see why the variable nginx::config::conf_dir is not resolved with concat while it was working with file { "${nginx::config::conf_dir}/conf.d/${name}-upstream.conf":.

Could it be a bug I introduce while providing the concat provider ?

[bartavelle]

This certainly is a bug, and it's probably not related to concat itself. The line number seems to indicate it's related to the defaults ...

[bartavelle]

How do you reproduce this ?

[PierreR]

This is how I use the nginx module:

  include profile::nginx

  $upstream = 'jenkinsmaster'

  nginx::resource::upstream { $upstream:
    members => ['localhost:8080', ],
  }

  nginx::resource::vhost { $vhost:
    proxy   => "http://${upstream}",
  }

I am using puppet-librarian and as soon as I change mod "jfryman/nginx", "0.0.9" into mod "jfryman/nginx", "0.0.10 in my Puppetfile it fails.

[PierreR]

I am pretty sure the first line can be replaced by include nginx as I am only doing the following in profile::nginx:

  class {'::nginx':
    manage_repo          => false,
    client_max_body_size => $client_max_body_size;
  }

[PierreR]

Note that the error looks a bit random. This is what I get while changing node:

puppetresources -p . -o jenkinsmaster --hiera ./tests/hiera.yaml --facts-override tests/facts-staging.yaml --pdbfile tests/facts.yaml  --ignoremodules=java,tool -v ERROR -t 'file'
Could not resolve variable $nginx::params::nx_global_owner in context nginx::params or root at # "./modules/nginx/manifests/init.pp" (line 31, column 1)

PS: actually there is a small difference between the two nodes ... So it is not so random. For the first error (puppetcenter node), the upstream resource is done by a foreman module (but it is done the same way using nginx::resource::upstream)

[bartavelle]

I don't think you can replace class{ 'nginx': } with just include nginx, because they don't do the same think in puppet, and IIRC this is exploited by a module you reported a bug with, probably this one.

[bartavelle]

I also get failures with version 0.0.10, but they are "obvious". For example, init requires, nginx::params::nx_global_owner, but it's never defined. I tried adding them manually, but then I encounter other errors ... I can't get to where you are.

[PierreR]

I have been trying to use github head to avoid the obvious errors from 0.0.10. There are still few errors that can be fixed easily (I will submit a PR for them at https://github.com/jfryman/puppet-nginx).

Point one

The only puppetresources error left for my jenkinsmaster role is related to this line:
https://github.com/jfryman/puppet-nginx/blob/master/manifests/resource/vhost.pp#L463
I cannot really see why it fails with:

Don't know how to convert this to a string:
undef at # "./modules/nginx/manifests/resource/vhost.pp" (line 465, column 3)
puppetresources: error!

Point two

About the other error, the one you cannot reproduce and occurs only in one of my node, I believe I nail it !

Could not resolve variable $nginx::config::conf_dir in context nginx::config or root at # "./modules/nginx/manifests/resource/upstream.pp" (line 71, column 12)

What is going on it that after the merge of this pr
the defined upstream resource depends on the nginx class.

The reason it does not work for one on that node is because I want to let the application define its upstream even if a vhost is never defined. So in a way even if I don't use the nginx class. It looks like this is not supported anymore. I have submitted an issue to ask if this is an intended change.

So in conclusion, my sole interrogation at this point is why this fails (Point one)

Thanks for your help !

[bartavelle]

Can you try printing one of the two variables in "${access_log_tmp} ${format_log}" ? I suppose one of them is undefined.

[PierreR]

Is it even going there ? I do expect $format_log to be undef and in that case it should not enter the default case.
I had to change the source file to go to that stage. So line 465, column 3 is actually the first line of the expression: $access_log_real = $format_log ?

[PierreR]

If I try notice("format log is ${format_log}"), it blows up with undef on that line ... Is there a way to debug print the undef ?

[bartavelle]

There aren't many options when compilation fails right now, but it certainly means here the $format_log is indeed undefined !

[PierreR]

So we have a puppetresources bug, right ?

One alternative is to rewrite

 $access_log_tmp = $access_log ? {
   undef => "${nginx::config::logdir}/${name_sanitized}.access.log",
   default => $access_log,
}
  $access_log_real = $format_log ? {
    undef => $access_log_tmp,
    default => "${access_log_tmp} ${format_log}",
}

into

  if $access_log == undef {
    if format_log == undef {
      $access_log_real = "${nginx::config::logdir}/${name_sanitized}.access.log"
    } else {  
      $access_log_real = "${nginx::config::logdir}/${name_sanitized}.access.log ${format_log}"
    }
  } else { # access_log is defined
    if format_log == undef {
      $access_log_real = $access_log
    } else { # both defined
      $access_log_real = "${access_log} ${format_log}"
    }
  }

But it is hardly an improvement ;-)

[bartavelle]

A way to fix this is to have $format_log = "" as a default value instead of undef.

That it fails is a goal of puppetresources. It catches uses of undef, because it usually means something went wrong, and Puppet might happily do something unexpected. In this case it's bad setting of default values. Setting a default parameter to undef makes no sense :

• It's something the user must set, and the module will not work properly if it isn't set. In that case it must not have a default value.
• It will be automatically converted to some default value (such as the empty string). In that case, why not directly set the default value ?

[bartavelle]

Default values set to undef is a major code smell IMO. This issue might be a contentious issue though ;)

[PierreR]

Well setting $format_log = "" doesn't solve the problem entirely. For some reason $access_log_tmp is undef while it should have been set by the previous statement:

  $access_log_tmp = $access_log ? {
    undef   => "${nginx::config::logdir}/${name_sanitized}.access.log",
    default => $access_log,
  }