nixos/restic: add command option #432329
nixos/restic: add command option #432329
Conversation
acd2858 to
87b5a1e
Compare
| ''; | ||
| example = [ | ||
| "sudo -u postgres pg_dumpall" | ||
| "/home/user/backup" |
There was a problem hiding this comment.
| "/home/user/backup" | |
| "/home/user/backup.sh" |
Maybe that's a bit clearer to show the intention
There was a problem hiding this comment.
actually i messed the example up, because i though the example section was for showcasing multiple examples haha - i'll fix this bit shortly, it should really be ["sudo" "-u" "postgres" "pg_dumpall"]
There was a problem hiding this comment.
I've pushed the bit that fixes this - let me know if you see any issues
|
I like the feature, was already looking earlier to see whether this existed. Great to see this contribution! The test seems to fail, so that's definitely something to look into. Also because this is a backup module some more eyes on it would be great. Note that I didn't test this (yet). |
oh shoot, I got this working yesterday but maybe I broke something during the merge. I'll take a look when I'm on lunch, thanks for letting me know :) |
|
Would be great if some of the PR context would be in the commit message so it doesn't get lost as easily. Definitely has some useful thoughts and context. |
|
@ofborg test restic |
87b5a1e to
e0c0139
Compare
|
@ofborg test restic |
e0c0139 to
c73ef3a
Compare
|
I'll fix when I get home, sorry I've never never written a nixpkgs test before so it was wizardry to me when I first wrote it |
60dd29a to
5780853
Compare
|
ok should be all ready now! it took me longer than i'd like to admit that everything passed into server.succeed was just bash... the test runs locally now so i'm not really sure what's up with the ofborg skipping, but if anyone has any ideas i'm all ears. I could potentially add a test to make sure that you can't pass both backups.a.paths and backups.a.command, but maybe this is overdoing it? If it's not I'm more than happy to do that. |
I force-pushed a bit more context - do you think it's okay now, or would you suggest adding anything else? |
|
@ofborg test restic |
|
The commit messages look great to me now! I'll see if I can find some time tomorrow or so to look at the delta, but generally if the suggestions are applied I think this is in a quite good state. Thanks! |
|
Will look at merge conflict tonight |
Add module support for --stdin-from-command flag, which was added to restic in restic/restic#4410. I also made a few very small changes here and there in the nix code to make it look a little neater in my opinion. I could potentially add support for the --stdin flag too, but this would require prepending the restic command with an external command and a pipe, which seems a bit messy - and the restic documentation says to prefer --stdin-from-command over --stdin anyway. I could add an option for --stdin-filename, but I feel that this would be better for users to do in extraBackupArgs.
Test checks that we are able to run a backup from a command, and that the resulting backup has the contents that we passed into it.
5780853 to
73f8c1e
Compare
|
Thanks for your (first) contribution! |
Add module support for --stdin-from-command flag, which was added to restic in restic/restic#4410. I also made a few very small changes here and there in the nix code to make it look a little neater in my opinion.
I could potentially add support for the --stdin flag too, but this would require prepending the restic command with an external command and a pipe, which seems a bit messy - and the restic documentation says to prefer --stdin-from-command over --stdin anyway.
I could add an option for --stdin-filename, but I feel that this would be better for users to do in extraBackupArgs.
This is my first (well, other than one i did earlier today) nixpkgs contribution so apologies if I've done anything wrong - please let me know!
I have not added myself to maintainers as I'm unsure if this is needed for a patch to an existing module, and also this is done in #432246 anyway.
I have made an assertion that the stdin backup is mutually exclusive with backing up from paths, but I still need to test that this is necessary - I initially thought it was but I'm not so sure now.Edit: I have checked and this check is actually necessary. It would be cool if you could backup from multiple stdin streams and files at once though, cmon restic......Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.