support keep multi backup files

[liuxu623]

No description provided.

[hashicorp-cla]

All committers have signed the CLA.

[sodabrew]

I'm just a bystander and fellow user. This is an interesting feature. I have some concerns:

• This will require a change to existing configs.
• It has a fixed format for the files.
• It will delete other files that match the format.

Given this is consul-template, it would be interesting to allow a template of the filenames.

Does this make sense as a built-in feature? You could script the exec command to stash off a copy of the just-generated file each time the contents changes.

[eikenb]

Thanks for the PR @liuxu623.

The show stopper with this PR is that it will break backwards compatibility. That is a no-go for the project at this point. So if you'd like to re-work the PR to make it backwards compatible and maybe consider @sodabrew's input as well, I'll take another look at it.

Thanks.

[liuxu623]

@eikenb @sodabrew I have make it backwards compatible, please help me review the code, thank you.

[greut]

it looks like some gofmt is required.

[eikenb]

Hey @sodabrew, @greut thanks for chiming in on this ticket, I have a question for you. Did you chime in to just be helpful or do you have an interest in this feature? I'm not 100% on if this is worth it yet and am curious if there is any interest outside the original author. Thanks.

[sodabrew]

I have used the backup and exec features with my own little script to save copies of some configs after they are generated.

[eikenb]

Hey @liuxu623, thanks for updating the PR to make it backwards compatible.

There are a few things left that I'd like to see changed. First is how backup = true and max_backup = 0 cancel each other out. At first I was thinking maybe renaming max_backup to something like additional_backup and change the logic to have it be 1 with backup=true and then more with that option. But I don't think this works that well.

After thinking about it more I just think making the multiple rotating backups completely separate from the simple backup options. What I mean is that if backup = true is set, it does just what it does now. No more, no less. Then we have a separate setting, say rotating_backups which is set to a number, and if that is set you keep that many date stamped rotating backups. They would be separate, such that you could have them both on. I think this will be simpler to understand and maintain while keeping backwards compatibility and allowing more flexibility.

With this change and the small bit I put inline, I think we'll be close.

Thanks.

[eikenb]

Oh.. one last thing. In your tests, if you could break them up into logical chunks using t.Run() with an appropriate label I'd appreciate that as well. I find doing that makes the tests much easier to read than one long function.

Thanks.

[eikenb]

Hey @liuxu623.. just a heads up. I changed the current backup code a bit (simplifying it and eliminating a race) which might cause conflicts.

[liuxu623]

Fixes #1270

[liuxu623]

@eikenb I have change max_backup to rotating_backups and add more backup test.

[eikenb]

Hey @liuxu623. I haven't forgotten about this issue but am probably not going to use it as is. We are refactoring the core logic of consul-template out into a separate library which changes much of the code this is working with but will make implementing something like this much easier after. Sorry for wasting your time having you rework it then not use it.

We can leave it open as a reminder about the feature once the refactoring is done or we can close it for now and revisit it later. If you'd prefer the later, maybe re-file it as a ticket to keep it on the radar?

Thanks again for your work on this and I'm sorry about the delays.

[eikenb]

I'm going to close this as I'm not sure if it would be generally useful enough to keep this around to remind me to add it after the new library update. Going to wait to see if it comes up again in the future.

Thanks again for working on this and I'm sorry it didn't work out.