Backup option breaks concat

[j-vizcaino]

The backup option, provided to the concat resource is used to backup the final target file before overwritting it.
Problem is: the backup option is used for the concat fragments file resources and, even worse, the fragments directory.
It means that, when backup is enabled (say ".bak") and a fragment changes, the file is first backed up inside the fragment dir, then removed (directory has purge, recurse set to true), but backed up again because the File[/var/lib/puppet/concat/_target_file] also has backup => ".bak" !

The final concatenated file results in the fragment appearing twice !

Pull request fixes this issue.
I've been able to update unit test, but I can't run the acceptance ones. Maybe they need to be updated as well.
Please consider this before the 1.2.4 release, I think this is a really serious bug.

Thank you

[jhoblitt]

I agree. Fragments should not be backed up and at one point that was the behavior of this module.

[j-vizcaino]

Looking at the code in branch master, I think the same patch needs to be applied there (don't want to stumble upon the same bug when hitting the 2.x releases)

[jhoblitt]

@j-vizcaino Actually, it looks like 1.2.x is ahead of master. Sigh. @hunner , @mhaskel , @cmurphy - is 1.2.x supposed to be forward merged into master or fixes cherry picked from master into that branch? Currently, 1.2.x has the 1.2.4 release it in while metadata.json in master still claims 1.2.3.

[underscorgan]

@jhoblitt at this point all dev should be happing against master, just fyi. 1.2.x is just a release branch and 2.0.x is largely just there for posterity.

[bmjen]

@j-vizcaino @jhoblitt the change to fragment backups is detailed in this PR.
#288

By essentially reverting this behavior, static-compilation will now break.

[j-vizcaino]

@bmjen What do you mean by static-compilation ? RSpec tests are all green. Dunno about beaker tests though...

[bmjen]

@j-vizcaino check out PR #288 and the comments in the referenced Jira ticket.
https://tickets.puppetlabs.com/browse/MODULES-1700

[j-vizcaino]

@bmjen I don't see why the backup parameter has anything to do with the helper script not being deployed... it seems pretty strange.

Anyway, activating backup for the fragments leads to a severe regression that causes updated fragments to appear twice (one: the new fragment, two: the older fragment) in the final file. The final file is crippled and unusable. That can't be the answer the problem mentionned in the Jira ticket.

As far as I know, this patch has been active on 4-5 different hosts for a week and encountered no problem so far. RSpec tests seems to confirm that also. Puppet version is 3.7.5 open-source.

[bmjen]

@j-vizcaino the title is misleading. The root cause of the problem can be read towards the bottom of the comments list.

https://tickets.puppetlabs.com/browse/MODULES-1700?focusedCommentId=159006&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-159006

So, concat::backup currently defaults to 'puppet' so that the original file can be backed up when it is changed, but the design of concat's backup logic was to eliminate fragments from each being backed up as they are unneeded normally.
When combined with the static compiler and concat::fragment resources using the source attribute, the backup parameter not being applied to the fragments appears to be the problem.