Config: Add run before/after/fail to more restic commands than backup

[jkellerer]

This PR addresses #125 .

@creativeprojects , please check if this is fine. After that I'll update the config reference and documentation.

[codecov[bot]]

Codecov Report

Base: 73.05% // Head: 72.88% // Decreases project coverage by -0.17% :warning:

Coverage data is based on head (1f1b4a9) compared to base (1b8774c). Patch coverage: 77.95% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
- Coverage   73.05%   72.88%   -0.18%     
==========================================
  Files          71       74       +3     
  Lines        7212     7225      +13     
==========================================
- Hits         5269     5266       -3     
- Misses       1731     1745      +14     
- Partials      212      214       +2     

Impacted Files	Coverage Δ
config/profile.go	89.34% <69.04%> (-2.42%)	:arrow_down:
wrapper.go	83.28% <82.35%> (+1.52%)	:arrow_up:
config/global.go	85.00% <0.00%> (ø)
monitor/prom/backup.go	100.00% <0.00%> (ø)
preventsleep/darwin.go	75.00% <0.00%> (ø)
preventsleep/linux.go	50.00% <0.00%> (ø)
preventsleep/windows.go	80.00% <0.00%> (ø)
monitor/prom/metrics.go	94.20% <0.00%> (+0.17%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[creativeprojects]

I was preparing version 0.18.0 but I can squeeze that in 👍🏻

[creativeprojects]

Yeah it looks good to me, I do like how it simplifies the profiles actually 😄

Thanks for that, it's really cool 👍🏻

[jkellerer]

Yes I thought so as well, it's getting less error prone to change things as there are not so many things to adjust and remember.

[jkellerer]

Will update the documentation tomorrow

[jkellerer]

While updating the documentation, I found some issues with the logic:

• run-after-fail now exists also for backup (and other commands), but when should it be triggered: Always like in finally or only when the failure is in the command sequence (run-before OR run OR run-after) => probably more correct ( chart link ).
• check, forget, init & prune also inherit the run hooks but they are only run when these commands are executed directly (not for indirect invocation via backup). Likely it would be better to include the run hooks for copy only at the moment to not confuse users.

If you want to release 0.18 soon, it may be better not to wait on this PR.

[creativeprojects]

I see, this is getting a bit complicated to follow.

There's probably no need for so many run targets TBH. There isn't a run-after-fail for any command just now and nobody asked for it. It doesn't mean it's not useful though.

BTW the chart makes sense to me.

Since the introduction of the copy command I can see how useful it would be as part as a backup bundle: Like a check -> backup -> copy -> retention, but also as a target on its own so it's good to be adding support for the copy target.

[creativeprojects]

check, forget, init & prune also inherit the run hooks but they are only run when these commands are executed directly (not for indirect invocation via backup). Likely it would be better to include the run hooks for copy only at the moment to not confuse users.

I was thinking a bit more about this one: Maybe we shouldn't make it available to these commands because it really is confusing. Agreed we should add it to the copy command. Now you can keep the code as-is but we don't mention it in the documentation?

For v2 I'm thinking we could add these in a separate section, like:

profiles:
  full:
    backup:
      source: /
      
      check:
        with-cache: true

      retention:
        keep-last: 3

    check:
      read-data: true

or is it also confusing to have 2 different check sections? In a way we already split retention and forget...

[jkellerer]

Thanks a lot for your thoughts on this 👍

Moving retention and check into backup for v2 is not a bad idea (implies that retention at its current place is removed in v2). What remains a bit confusing is that the commands can be configured twice in the same profile. Not sure if this makes sense to most users.

The alternative would be to remove retention entirely and replace it by forget. Then we could just add an option to run another restic command before or after a command. E.g. by allowing to add section names like check and forget into run-before and run-after or by creating dedicated properties run-command-before, run-command-after. This would make it more obvious that the sections are executed as configured.

Or maybe command chaining isn't required when we have groups and the option to schedule them in v2?

Regarding changes to this PR: I'll keep the code mostly but I will adjust the types so that only the copy command will inherit the actions. Also I will make sure it executes as documented in the updated flow-chart.

[creativeprojects]

That's also a good idea. Or even maybe create a scripts section like:

profiles:
  new:
    backup:
      source: /
    scripts:
      full_backup:
        - check
        - backup
        - copy
        - forget

that we would call this way:

resticprofile --name new --script full_backup

Although there might also be some situations where a script over multiple profiles could be needed...

[jkellerer]

Also nice :) .. looks like we're getting closer, how about this:

aliases:
  full_backup:
    - backup
    - check
    - copy
    - forget

groups:
  all:
    profiles: ['*'] # - wildcard support might be nice too

profiles:
  new:
    backup:
      source: /

And called via: resticprofile all.full_backup

Thoughts behind this:

• scripts might be considered as something more powerful, think the proposal is more like aliases.
• For aliases it is common to be in the same namespace than commands, so no other calling convention is needed.
• groups serve as the glue to call it on multiple profiles.
• aliases are global like groups to make that possible.
• We could also allow overriding aliases in profiles to refine them where needed.

And lastly, if we only deprecate retention and check-before/after instead of removing it, we could auto-create aliases in v2 like this:

aliases:
  backup: # check-before: true, check-after: true, retention/after-backup: true
    - check
    - backup
    - forget
    - check

[creativeprojects]

Yes, I do like the concept of aliases. Simple to understand, and reusable across profiles.

Using internal aliases to mimic the existing configuration would work very well to keep compatibility with v1 while avoiding code duplication.

I'll think about it a bit more, and then I'll probably update the documentation on v2.

Thanks, that was excellent brainstorming 👍🏻

[jkellerer]

One thing to consider is how to handle common flags and additional CLI args. We already had this issue with the current implementation. With aliases it requires handling in the alias definition.

[creativeprojects]

Thanks, that was excellent work 👍🏻

[jkellerer]

Btw. regarding retention: I changed my mind on this section. While technically retention is a configuration for forget, it does logically make sense that retention and forget can be configured separately.

The section retention would usually cover the general lifetime of snapshots while forget would usually be used to manually forget things that happen out of order (e.g. changed config or removing single snapshots)

To integrate retention properly with aliases, retention should likely become a resticprofile command that can also be called directly.

Just added this to a closed PR to not forget it when implementing aliases.