sanoid: fix sanoid.conf generation #83904

ju1m · 2020-03-31T22:57:17Z

Motivation for this change

sanoid currently generates a bogus sanoid.conf: defaults of NixOS' options are put into templates and into datasets (making the use of templates useless).

Things done

Simplify the generation of sanoid.conf.
As in other modules like services.postfix.config I'm also renaming options to use original names because they are 1:1 mapping to sanoid's config.

A relevant discussion, on #nixos-dev:

julm : while I'm at writing a fix to nixos/modules/services/backup/sanoid.nix (which generates a bogus configfile) I'm wondering if there is a rationale for using CamelCase for options which are direct mapping to sanoid options which use snake_case? eg. use_template becomes useTemplate in NixOS. I'd personnaly prefer to stick to the original, but what's the policy?
rnhmjoj : julm: if it's not too late.. i know at least of the matrix-synapse module, which keeps the original snake_case names. there are also the module.settings options from RFC 0042. personally i don't like having different options styles but keeping the original also has value.
julm : rnhmjoj: thanks!
julm : I'll try to justify to keep the original names because it makes the code simpler (no mapping to be done before supplying the attrset to toINI)
rnhmjoj : julm: if you are doing a 1:1 map of options, yes, i would keep the original names

<!-- Please check what applies. Note that these are not hard requirements but merely serve as information for reviewers. -->

- [ ] Tested using sandboxing ([nix.useSandbox](http://nixos.org/nixos/manual/options.html#opt-nix.useSandbox) on NixOS, or option `sandbox` in [`nix.conf`](http://nixos.org/nix/manual/#sec-conf-file) on non-NixOS linux)
- Built on platform(s)
   - [X] NixOS
   - [ ] macOS
   - [ ] other Linux distributions
- [ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside [nixos/tests](https://github.com/NixOS/nixpkgs/blob/master/nixos/tests))
- [ ] Tested compilation of all pkgs that depend on this change using `nix-shell -p nixpkgs-review --run "nixpkgs-review wip"`
- [ ] Tested execution of all binary files (usually in `./result/bin/`)
- [ ] Determined the impact on package closure size (by running `nix path-info -S` before and after)
- [ ] Ensured that relevant documentation is up to date
- [X] Fits [CONTRIBUTING.md](https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md).

nixos/modules/services/backup/sanoid.nix

lopsided98 · 2020-04-01T18:33:32Z

nixos/modules/services/backup/sanoid.nix

-      type = types.ints.unsigned;
-      default = 48;
+      type = with types; nullOr ints.unsigned;
+      default = null;


Good catch here. This was changed from using null defaults late in the review cycle for the original PR (#72060 (comment)) and I didn't consider the side effects.

cc @infinisil

Agree, when reading the nixos docmentation, null can be confusing. I'd rather like having an explicit default.

nixos/modules/services/backup/sanoid.nix

lopsided98

This looks good and I have tested it on my systems.

The only thing I noticed is that the comment on line 13 should probably be removed because this module doesn't have defaults anymore.

We really should backport this PR to 20.03 because without it sanoid is pretty broken. We can't backport it as is because it is a breaking change, but I think backporting just the null defaults should work.

ju1m · 2020-04-27T15:36:16Z

I've separated the changes in two commits so that the fix could be cherry-picked in 20.03.

lopsided98

Thanks for making the backport easier. I think this is ready to merge.

@infinisil perhaps you can take a look at this since you reviewed the initial sanoid PR?

picnoir

Could you adress the release note typo and rebase this PR?

Not a big fan of the implicit default values, but why not. @lopsided98, you're the maintainer, it's your call.

If you're ok with that, I think this PR is ready to be merged.

picnoir · 2020-08-24T15:07:03Z

nixos/doc/manual/release-notes/rl-2009.xml

+   <listitem>
+     <para>
+       <literal>services.sanoid.datasets.*.useTemplate</literal> has been renamed to
+       <literal>services.sanoid.datasets.*.useTemplate</literal> to match upstream name.


Suggested change

<literal>services.sanoid.datasets.*.useTemplate</literal> to match upstream name.

<literal>services.sanoid.datasets.*.use_template</literal> to match upstream name.

picnoir · 2020-08-24T15:08:45Z

nixos/modules/services/backup/sanoid.nix

-      type = types.ints.unsigned;
-      default = 48;
+      type = with types; nullOr ints.unsigned;
+      default = null;


Agree, when reading the nixos docmentation, null can be confusing. I'd rather like having an explicit default.

lopsided98 · 2020-08-24T20:01:39Z

Not a big fan of the implicit default values, but why not. @lopsided98, you're the maintainer, it's your call.

Is there an alternative that allows templates to work correctly?

numinit · 2020-09-08T20:35:02Z

Didn't realize this was a problem, but fix makes sense to me. +1

lopsided98 · 2020-11-16T01:32:22Z

I'm trying to find someone with merge privileges who can take a look at this PR. Could you move the release notes to 21.03 so hopefully it will be ready to merge?

ju1m · 2020-11-16T02:20:23Z

@lopsided98 I've rebased against current master.

nixos-discourse · 2020-11-16T02:33:53Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/372

lopsided98

I just realized this needs to update the NixOS test with the new option names.

ju1m · 2020-11-25T05:34:03Z

@lopsided98 thanks, I've fixed nixos/tests/sanoid.nix.

ju1m · 2021-02-17T15:09:41Z

Rebase and fix merge conflict due to the rename nixos/doc/manual/release-notes/rl-21{03 => 05}.xml in cfed3b8

SuperSandro2000

It would be nice if we could map the old options to the new ones.

ju1m · 2021-06-27T00:20:15Z

@SuperSandro2000 old options are now supported as you requested.

SuperSandro2000 · 2021-06-29T23:56:54Z

nixos/modules/services/backup/sanoid.nix

      default = [];
    };
+    useTemplate = use_template;


I was more thinking of https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/monitoring/prometheus/default.nix#L681

@SuperSandro2000 AFAICS mkRenamedOptionModule does not work for options in submodules, that's why I've used mkAliasDefinitions instead.

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Apr 1, 2020

ju1m force-pushed the sanoid branch from b28a542 to 67aee2f Compare April 1, 2020 18:05

lopsided98 suggested changes Apr 1, 2020

View reviewed changes

ju1m force-pushed the sanoid branch from 67aee2f to 327a062 Compare April 1, 2020 22:33

ofborg bot added 8.has: changelog 8.has: documentation labels Apr 1, 2020

lopsided98 suggested changes Apr 24, 2020

View reviewed changes

nixos/modules/services/backup/sanoid.nix Outdated Show resolved Hide resolved

ju1m force-pushed the sanoid branch from 327a062 to d67f3e0 Compare April 25, 2020 14:32

ofborg bot added the 10.rebuild-linux: 1 label Apr 25, 2020

lopsided98 approved these changes Apr 26, 2020

View reviewed changes

ju1m force-pushed the sanoid branch 2 times, most recently from 42111d9 to 23a5833 Compare April 27, 2020 15:24

lopsided98 approved these changes Apr 27, 2020

View reviewed changes

picnoir reviewed Aug 24, 2020

View reviewed changes

ju1m force-pushed the sanoid branch from 23a5833 to 598049d Compare August 25, 2020 00:23

lopsided98 requested a review from numinit September 4, 2020 20:04

numinit approved these changes Sep 8, 2020

View reviewed changes

ju1m force-pushed the sanoid branch 2 times, most recently from f57f200 to 9edd200 Compare November 16, 2020 02:18

lopsided98 suggested changes Nov 22, 2020

View reviewed changes

ju1m force-pushed the sanoid branch from 9edd200 to 3bc5876 Compare November 25, 2020 05:30

ju1m requested review from numinit and lopsided98 January 18, 2021 19:13

ju1m force-pushed the sanoid branch from 3bc5876 to a798d2a Compare February 17, 2021 15:07

lopsided98 mentioned this pull request Feb 27, 2021

nixos/syncoid: split in multiple systemd services and harden them #98455

Merged

13 tasks

doronbehar approved these changes Mar 19, 2021

View reviewed changes

lopsided98 mentioned this pull request May 20, 2021

sanoid: 2.0.3 -> 2.1.0 #123644

Merged

10 tasks

ju1m force-pushed the sanoid branch from a798d2a to ecd9d76 Compare May 20, 2021 06:00

ofborg bot removed the 10.rebuild-linux: 1 label May 20, 2021

SuperSandro2000 reviewed May 22, 2021

View reviewed changes

sanoid: fix sanoid.conf generation

b62a093

ju1m force-pushed the sanoid branch from ecd9d76 to b62a093 Compare June 9, 2021 01:29

github-actions bot removed 8.has: changelog 8.has: documentation labels Jun 9, 2021

doronbehar requested a review from SuperSandro2000 June 29, 2021 08:36

SuperSandro2000 reviewed Jun 29, 2021

View reviewed changes

ju1m requested a review from SuperSandro2000 June 30, 2021 02:59

SuperSandro2000 merged commit 30e2735 into NixOS:master Jun 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sanoid: fix sanoid.conf generation #83904

sanoid: fix sanoid.conf generation #83904

ju1m commented Mar 31, 2020

lopsided98 Apr 1, 2020

picnoir Aug 24, 2020

lopsided98 left a comment

ju1m commented Apr 27, 2020

lopsided98 left a comment

picnoir left a comment

picnoir Aug 24, 2020

picnoir Aug 24, 2020

lopsided98 commented Aug 24, 2020

numinit commented Sep 8, 2020

lopsided98 commented Nov 16, 2020

ju1m commented Nov 16, 2020

nixos-discourse commented Nov 16, 2020

lopsided98 left a comment

ju1m commented Nov 25, 2020

ju1m commented Feb 17, 2021

SuperSandro2000 left a comment

ju1m commented Jun 27, 2021

SuperSandro2000 Jun 29, 2021

ju1m Jun 30, 2021

	<literal>services.sanoid.datasets.*.useTemplate</literal> to match upstream name.
	<literal>services.sanoid.datasets.*.use_template</literal> to match upstream name.

sanoid: fix sanoid.conf generation #83904

sanoid: fix sanoid.conf generation #83904

Conversation

ju1m commented Mar 31, 2020

Motivation for this change

Things done

lopsided98 Apr 1, 2020

Choose a reason for hiding this comment

picnoir Aug 24, 2020

Choose a reason for hiding this comment

lopsided98 left a comment

Choose a reason for hiding this comment

ju1m commented Apr 27, 2020

lopsided98 left a comment

Choose a reason for hiding this comment

picnoir left a comment

Choose a reason for hiding this comment

picnoir Aug 24, 2020

Choose a reason for hiding this comment

picnoir Aug 24, 2020

Choose a reason for hiding this comment

lopsided98 commented Aug 24, 2020

numinit commented Sep 8, 2020

lopsided98 commented Nov 16, 2020

ju1m commented Nov 16, 2020

nixos-discourse commented Nov 16, 2020

lopsided98 left a comment

Choose a reason for hiding this comment

ju1m commented Nov 25, 2020

ju1m commented Feb 17, 2021

SuperSandro2000 left a comment

Choose a reason for hiding this comment

ju1m commented Jun 27, 2021

SuperSandro2000 Jun 29, 2021

Choose a reason for hiding this comment

ju1m Jun 30, 2021

Choose a reason for hiding this comment