nixos/rustic: init module #372935
nixos/rustic: init module #372935
Conversation
|
I hope you dont mind that i merged. |
|
Not at all, thank you for having reviewed and landed, and happy to see it undergo greater use than just my own! :) |
|
Wait a minute. This implementation calls Reverting. |
|
For anyone looking at this, this was reverted with consensus, and will be going through a deeper review, leaving enough time for all stakeholders to comment. The new PR is at #373042 @emilylange Can you voice again your concerns on #373042, ideally pointing out specific parts of the code? I thought I had done a fine job wrt. eg. shell escaping, but I may have missed some parts of it. |
| else if v ? postgres then | ||
| v.postgres |
There was a problem hiding this comment.
The integration with PostgreSQL seems overly specific. Nixpkgs has 20+ database packages; should this module grow to include them all? To me, it would make more sense for the operator to compose this module with the existing postgresqlBackup module. The latter would dump the databases on a schedule, then this one would back them up. Wdyt?
| else if o.pipeJsonInto ? command then | ||
| " --json | ${o.pipeJsonInto.command} \"$1\"" | ||
| else if o.pipeJsonInto ? prometheus then | ||
| " --json | ${prometheusWriteScript o.pipeJsonInto.prometheus.nodeExporterCollectorFolder} \"$1\"" |
There was a problem hiding this comment.
As with PostgreSQL, in my opinion the integration with Prometheus is overly specific for this module. There are other observability clients, such as Amazon CloudWatch Agent added after 24.11; should this module know about them all?
How easy would it be for the operator to integrate this module with Prometheus themselves using the generic constructs?
| #!${pkgs.bash}/bin/bash | ||
| set -euo pipefail | ||
| TZ="" ${cfg.package}/bin/rustic backup${ifFilesArgs + ifCommandArgs + genericArgs + pipeJsonIntoCmd} |
| checkProfiles = lib.mkOption { | ||
| type = lib.types.bool; | ||
| default = true; | ||
| description = '' | ||
| If enabled, checks that the generated rustic profiles are valid. |
There was a problem hiding this comment.
Why would the operator not want this?
| backups = lib.mkOption { | ||
| default = { }; |
There was a problem hiding this comment.
Is the idea that the operator can specify profiles and/or backups, with profiles being files the operator writes himself while backups is generated from a config?
|
|
||
| - [git-worktree-switcher](https://github.com/mateusauler/git-worktree-switcher), switch between git worktrees with speed. Available as [programs.git-worktree-switcher](#opt-programs.git-worktree-switcher.enable) | ||
|
|
||
| - [rustic](https://github.com/rustic-rs/rustic), a Rust-based backup tool. Available as [services.rustic](#opt-services.rustic.enable). |
There was a problem hiding this comment.
As this module is quite complex, IMO it necessitates some unit tests.
|
Oops, I added my comments on the older PR. I copied them to the new PR. |
This has been running smoothlessly on my 4 systems for quite a while, on both aarch64 and x86-64 linux.
Considering the complexity of the module, I'll wait for (either a very long time or) a review from another committer before landing. But I can already say I'm very happy with it personally :)
The only future work I'd see on it would be to add easy toggles to automatically run a FS-level snapshot before backing up, which I'd likely implement for ZFS and maybe BTRFS. Or maybe I'd PR that upstream directly, idk, it's not on the short-term todo-list anyway.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.