borgmatic: Allow top-level extra options #3793
Conversation
DamienCassou
force-pushed
the
borgmatic-hooks
branch
from
aed4339
to
e05da65
Compare
| @@ -168,7 +170,7 @@ let | |||
| } // config.retention.extraConfig; | |||
| consistency = removeNullValues { checks = config.consistency.checks; } | |||
| // config.consistency.extraConfig; | |||
| }; | |||
| } // config.extraConfig); | |||
There was a problem hiding this comment.
It may be a bit unfortunate that this doesn't do a recursive update so consistency.extraConfig = … would give a different result compared to extraConfig.consistency = ….
There was a problem hiding this comment.
I think it is a very good point. Would you mind giving me a clue on how to implement that please?
There was a problem hiding this comment.
after discussing on Matrix, we decided to:
- get rid of all the
extraConfigoptions and only keep the top-level one - use
mkRenamedOptionModuleto warn the user about the deprecated API
Unfortunately, I've been unable to make it work. I pushed a WIP commit to show my latest try. Can someone please help?
There was a problem hiding this comment.
The latest issue I have is
error: You're trying to declare a value of type `list'
rather than an attribute-set for the option
`programs.borgmatic.backups.<name>.imports'!
This usually happens if `programs.borgmatic.backups.<name>.imports' has option
definitions inside that are not matched. Please check how to properly define
this option by e.g. referring to `man 5 configuration.nix'!
(use '--show-trace' to show detailed location information)
There was a problem hiding this comment.
The immediate problem is that you're defining imports inside options, but it should be at the same level as config and options.
The real problem is that these mkRemovedOptionModules will not work because of NixOS/nixpkgs#96006. The most promising attempt to fix that is currently NixOS/nixpkgs#207187.
There was a problem hiding this comment.
If I understand what @ncfavier just said, it seems impossible to deprecate the non-top-level extraConfig options for now. Having both a top-level extraConfig option and having non-top-level ones is messy.
The only solution I see is to have no top-level extraConfig option. As a result, I suggest we introduce a top-level option for each of borgmatic's setting sections and add one extraConfig for each. If I'm not mistaken after reading the example configuration file, this just means adding output and hooks top-level options.
There was a problem hiding this comment.
I implemented this approach in #4108.
DamienCassou
force-pushed
the
borgmatic-hooks
branch
3 times, most recently
from
ac9bdef
to
70bf034
Compare
|
Can someone please answer my question? |
DamienCassou
force-pushed
the
borgmatic-hooks
branch
from
70bf034
to
93982b3
Compare
|
Closing this PR in favor of #4108. |
Description
Fix #3760
Checklist
Change is backwards compatible.
Code formatted with
./format.Code tested through
nix-shell --pure tests -A run.all.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
Added myself as module maintainer. See example.
Added myself and the module files to
.github/CODEOWNERS.