redis::instance: do not exec cp if source and destination are equal #430
Conversation
trefzer
force-pushed
the
fix_copy_exec
branch
2 times, most recently
from
f3a51ce
to
0c52693
Compare
manifests/instance.pp
Outdated
| exec { "cp -p ${redis_file_name_orig} ${redis_file_name}": | ||
| path => '/usr/bin:/bin', | ||
| subscribe => File[$redis_file_name_orig], | ||
| refreshonly => true, | ||
| } |
There was a problem hiding this comment.
I wonder why cp is used at all. Doesn't this work:
| exec { "cp -p ${redis_file_name_orig} ${redis_file_name}": | |
| path => '/usr/bin:/bin', | |
| subscribe => File[$redis_file_name_orig], | |
| refreshonly => true, | |
| } | |
| file { $redis_file_name: | |
| source => $redis_file_name_orig, | |
| subscribe => File[$redis_file_name_orig], | |
| } |
I haven't checked and it may result in some duplicate definition, but still: puppet can already copy files this way.
There was a problem hiding this comment.
right, I tried (but without subscribe, which is not needed since the source is checked anyway !). Obviously I get an error when running with noop since the source is not (yet) available.
The question is, why there are two config files needed with the same content ?
If there is really need for two files, why not write them directly with puppet ?
So I agree, that this is just a dirty patch to make things a little bit better, but I also would prefer a 'proper' solution (which I cannot implement cause of available time !)
There was a problem hiding this comment.
right, I tried (but without subscribe, which is not needed since the source is checked anyway !)
I wasn't sure about that so I tried to play it safe :)
The question is, why there are two config files needed with the same content ?
If there is really need for two files, why not write them directly with puppet ?
So I agree, that this is just a dirty patch to make things a little bit better, but I also would prefer a 'proper' solution (which I cannot implement cause of available time !)
I think the reason is that you can change the Redis config at runtime using config statements. This would then only be reverted when you change the actual Redis config via Puppet (the cp is then executed again). So maybe my suggestion failed to maintain that.
However, I agree it's very weird to allow both local config and have configuration management in place. Perhaps the module should default to NOT doing that. Your patch looks like a decent step in that direction.
|
Dear @trefzer, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
|
Any update about making the cp optional ? |
Pull Request (PR) description
Avoid using this copy with destination and source beeing equal
Remark: generally it's a bad idea to use an exec statement to copy a file. Since this way we cannot see diffs etc.
A better way would be to use a file{ $destination: source=> $source} statement, but running with noop makes it fail then !