A collectd::plugin::python acceptance test #805
Conversation
|
ubuntu 16 fails because collectd does not pull in python , only python-lib. So the fact
but this can't be made because
does not exist until python is installed. In the real world this never happens probably because something pulls in python. |
|
so python is a soft dependency? Can you document it in the README.md and install it during the acceptance tests? |
|
It's really a hard dependency so added the python install the collectd::python::module. |
manifests/plugin/python.pp
Outdated
| @@ -47,6 +49,8 @@ | |||
| globals => $globals, | |||
| } | |||
|
|
|||
| ensure_packages([$python_package], { ensure => $python_package_ensure }) | |||
There was a problem hiding this comment.
we should probably wrap this into a $manage_python condition. There are distributions where 'python' defaults to python 3, others default to version 2. Do we need to catch that here as well?
There was a problem hiding this comment.
Collectd is always built against exactly one python. It is the python package untill we have a counter example.
Happy to add a manage_python boolean. It will help people who install python in another funky way.
traylenator
force-pushed
the
pythonaccept
branch
3 times, most recently
from
8c31070
to
c731ab7
Compare
manifests/plugin/python.pp
Outdated
| Hash $modules = {}, | ||
| $order = '10', | ||
| $conf_name = 'python-config.conf', | ||
| Boolean $manage_python = true, |
There was a problem hiding this comment.
I'm currently unsure if this should default to true. This probably breaks the catalog for many people. Should it default to false and be documented in the README.md?
There was a problem hiding this comment.
+1 to not install/manage prerequisites by default.
There was a problem hiding this comment.
collectd::plugin::python
cannot work if python is not installed. What's the difference between a pre-requisite and a requirement?
Package is added with 'ensure_packages' to reduce impact to people catalogs.
Happy to change of course.
There was a problem hiding this comment.
ensure_packages works only if this class gets evaluated at the end. It will still fail if later on in the catalog somebody tries to manage the python package in a python specific module. Also the python package defaults to different versions depending on the operating system.
There was a problem hiding this comment.
I'll change because it's easier to do and get through. It's only ubunutu and I never use that so...
Think I already answered the python version comment above.
ensure_packages works only if this class gets evaluated at the end.
The other class probably needs fixing if it does not ensure_resource on such a fundamental package.
Essentially this is saying it's never okay to install python but given ubunut 16 docker images don't have python this is a new necessity.
Of course there's a total alternate method to this. Remove the facter fact that depends on a python interpreter to work and just set sensible values for python_sitelib on different OSes.
manifests/plugin/python.pp
Outdated
| @@ -47,6 +50,13 @@ | |||
| globals => $globals, | |||
| } | |||
|
|
|||
| if $manage_python { | |||
| ensure_packages([$python_package], { ensure => $python_package_ensure }) | |||
There was a problem hiding this comment.
I don't like this because it's really a soft dependency and is not some kind of separate collectd_python package. For example, if a plugin required mongodb, apache or ruby would it be appropriate to manage installation in this module? That being said, ensure_packages is the best way to do it.
There was a problem hiding this comment.
It's slightly different because it's the fact and puppet manifest that requires 'python'. collectd itself does not require it.
Anyway especially with the default false the code is basically pointless so lets just chop it.
python being installed is pre-requiste to the fact operating correctly so it is installed within the acceptence test and documented.
|
Changed after approval, still okay? |
|
Thanks @bastelfreak @juniorsysadmin for the comments. |
Pull Request (PR) description
A collectd::plugin::python acceptance test