search

tohuwabohu / puppet-duplicity

Puppet module to manage backups based on duplicity.
Apache License 2.0
8 stars 33 forks source link

Do not ensure absent python-paramiko #41

Closed arnaudmorin closed 7 years ago

arnaudmorin commented 7 years ago

Package python-paramiko can be installed by other modules, so we must use ensure_resource instead of package resource directly. Also, we don't want to remove that package if we remove duply because it may be used by some other modules / packages on the system. Best would be to ask duply package maintainer to add the python-paramiko as dependency and avoid such workarounds in the puppet code itself (but bug is still opened on launchpad: https://bugs.launchpad.net/ubuntu/+source/duplicity/+bug/1169463).

tohuwabohu commented 7 years ago

hmmm ... you raise a valid point. I thought the usage of ensure_resource could lead to a race condition and its usage was discourage but may be that has changed. Let's see how it goes.

tohuwabohu commented 7 years ago

Package python-paramiko can be installed by other modules, so we must use ensure_resource instead of package resource directly.

I had a quick look at the ensure_resource documentation which states ...

If the resource already exists, but does not match the specified parameters, this function attempts to recreate the resource, leading to a duplicate resource definition error.

So that means if someone has (or wants to use)

package { 'python-paramiko': ensure => latest }

the ensure_resources won't save him but cause a duplicate declarations error. In fact anything besides present would cause this problem (and right now there is little he could do).

An alternative could be to wrap the package resource of the python-paramiko package behind a workaround_manage_paramiko if block which can be used to manage or not manage the resource.

I'm ok with merging the PR as it is, just pointing out if another module is declaring this package resource the usage of ensure_resource may not be the silver bullet.

arnaudmorin commented 7 years ago

Agree with you about the ensure latest thing. But most of the time, we prefer using ensure present instead of latest. But if you prefer I can also add a protection with workaround_manage_paramiko if statement. You tell me.

arnaudmorin commented 7 years ago

Hum, I just check the code of ensure_resource versus ensure_packages. The later may be better: it rely on a function called findresource and does nothing if the resource already exists. I will switch the PR to ensure_packages

tohuwabohu commented 7 years ago

I've released version 4.5.2 to the forge, thank you very much for the contribution.