puppetlabs / puppetlabs-apt

Puppet module to help manage Apt
https://forge.puppetlabs.com/puppetlabs/apt
Apache License 2.0
216 stars 463 forks source link

ppa: include apt #1153

Closed kenyon closed 6 months ago

kenyon commented 6 months ago

Summary

This makes the apt::ppa defined type easier to use without having to remember to include the apt class. The original version of ppa.pp did an include like this. See https://github.com/puppetlabs/puppetlabs-apt/pull/1152#issuecomment-1854787537

Also, the README doesn't say that you have to include apt to use apt::ppa: https://github.com/puppetlabs/puppetlabs-apt/blob/e0b3a5db6abb043f106614dffe341f68d88158ab/README.md?plain=1#L171-L175

Fixes #1151

kenyon commented 6 months ago

@smortex do you have some reference on this behavior? I would think that the catalog compiler waits to resolve class variables until after it has loaded all of the classes.

smortex commented 6 months ago

I don't have a reference at hand (not sure what to look for), but let me create a sample file that expose the "wrong" behavior:

warning("foo=${foo}")
$foo = 'value'
warning("foo=${foo}")

class bar (
  $x,
) {
  warning("bar::x=${x}")
}

class baz (
  $x = $bar::x,
) {
  warning("baz::x=${x}")
}

class { 'baz':
}

class { 'bar':
  x => 'value',
}
romain@desktop-fln40kq /tmp % puppet apply -t foo.pp
Warning: Unknown variable: 'foo'. (file: /tmp/foo.pp, line: 1, column: 16)
Warning: Scope(Class[main]): foo=
Warning: Scope(Class[main]): foo=value
Warning: Unknown variable: 'bar::x'. (file: /tmp/foo.pp, line: 12, column: 8)
Warning: Scope(Class[Baz]): baz::x=
Warning: Scope(Class[Bar]): bar::x=value
Notice: Compiled catalog for desktop-fln40kq.lan in environment production in 0.01 seconds
Info: Using environment 'production'
Info: Applying configuration version '1702586183'
Notice: Applied catalog in 0.01 seconds

foo= and baz::x= do not have a value when evaluated, this produce a warning, but not an error. IMHO, that behavior is not great, and at least when an undeclared value is used, setting a value to it later should raise an error to limit this kind of issues, but I guess its not a trivial change.

Edit: Also delaying evaluation would probably make a lot of sense!

kenyon commented 6 months ago

@smortex thanks. I'll just close this, but if someone else wants to add a missing inclusion detection, feel free.

smortex commented 6 months ago

@smortex thanks. I'll just close this, but if someone else wants to add a missing inclusion detection, feel free.

I opened https://github.com/puppetlabs/puppet/issues/9184, I feel like this is not great and should be improved.