redis::instance: do not exec cp if source and destination are equal

[trefzer]

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 !

[ekohl]

I wonder why cp is used at all. Doesn't this work:

Suggested change

	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.

[trefzer]

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 !)

[ekohl]

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.

[vox-pupuli-tasks]

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

[olivierk7]

Any update about making the cp optional ?