-
Notifications
You must be signed in to change notification settings - Fork 463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ppa: include apt #1153
ppa: include apt #1153
Conversation
This makes the 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 puppetlabs#1152 (comment) Fixes puppetlabs#1151
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter default value for options
and package_name
depend on apt
being included before that class is used.
So this will likely to weird things. Detecting the missing inclusion and reporting it still make sense, this is usually done by checking that the class exist and raising an error if not found, e.g. with nginx:
@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. |
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
Edit: Also delaying evaluation would probably make a lot of sense! |
@smortex thanks. I'll just close this, but if someone else wants to add a missing inclusion detection, feel free. |
I opened puppetlabs/puppet#9184, I feel like this is not great and should be improved. |
Summary
This makes the
apt::ppa
defined type easier to use without having to remember toinclude
theapt
class. The original version ofppa.pp
did aninclude
like this. See #1152 (comment)Also, the README doesn't say that you have to
include apt
to useapt::ppa
:puppetlabs-apt/README.md
Lines 171 to 175 in e0b3a5d
Fixes #1151