nixos/tsm-client: migrate to freeform settings (RFC42) #253428
Conversation
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
Yarny0
force-pushed
the
tsm-freeform
branch
from
d8dd39a to
a0ee3f9
Compare
|
Updated branch:
|
|
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/2655 |
There was a problem hiding this comment.
Clean work!
The migration approach is unusual. It would be great to follow up by creating some kind of mkRenamedAttrsOptionModule to solve this for everyone. I don't think it poses any risks to merge it as-is, though.
I don't use tsm-client and I don't know how to test it, but code changes LGTM.
| makeDsmSysLines = key: value: | ||
| # Turn a key-value pair from the server options attrset | ||
| # into zero (value==null), one (scalar value) or | ||
| # more (value is list) configuration stanza lines. | ||
| if isList value then map (makeDsmSysLines key) value else # recurse into list | ||
| if value == null then [ ] else # skip `null` value | ||
| [ (" ${key}${ | ||
| if value == true then "" else # just output key if value is `true` | ||
| if isInt value then " ${builtins.toString value}" else | ||
| if path.check value then " \"${value}\"" else # enclose path in ".." | ||
| if singleLineStr.check value then " ${value}" else | ||
| throw "assertion failed: cannot convert type" # should never happen | ||
| }") ]; | ||
|
|
||
| makeDsmSysStanza = {servername, ... }@serverCfg: | ||
| let | ||
| # drop special values that should not go into server config block | ||
| attrs = removeAttrs serverCfg [ "servername" "genPasswd" ]; | ||
| in | ||
| '' | ||
| servername ${servername} | ||
| ${concatLines (concatLists (mapAttrsToList makeDsmSysLines attrs))} | ||
| ''; | ||
|
|
There was a problem hiding this comment.
Did you look into using formats.keyValue? It looks like it would need to be adapted to skip null values. That option would be a great addition to it anyway.
There was a problem hiding this comment.
Yes, but the format required here has more deviations from what formats.keyValue (and also formats.ini) supports:
nullshould be skippedtrueshould yield a key-only line- The
servernamekey must be stated before other keys; it acts like a section head.
I don't think one could (or should) accomplish this by extending keyValue; it's just too specialised. Like, e.g., to gitIni, one could add a dsmSys format, but I feel tsm-client doesn't deserve its own (global) format definition in nixpkgs.
| }) cfg.servers); | ||
| }) cfg.servers) | ||
| # XXX migration code for freeform settings, this can be removed after 23.11: | ||
| ++ (enrichMigrationInfos "assertions" (addText: { assertion, message }: { inherit assertion; message = addText message; })); |
There was a problem hiding this comment.
This line took me a few minutes to figure out. I finally realized that it's equivalent to this:
++ (enrichMigrationInfos "assertions" (addText: assertion: assertion // { message = addText assertion.message; }));I think your style here is fine - just commenting in case others are similarly confused!
| # XXX migration code for freeform settings, these can be removed after 23.11: | ||
| options.warnings = optionsGlobal.warnings; | ||
| options.assertions = optionsGlobal.assertions; |
There was a problem hiding this comment.
This won't be the only module where we want to rename options inside attrsOf submodule. So that we don't lose track of this need, could you create an issue and link to this PR as an example use case?
There was a problem hiding this comment.
Such an issue exists already: #96006
I was thinking about a general solution, but the tricky part is "transporting" assertions and warning out of the submodule into the main configuration area. Here, in the PR at hand, I'm doing it manually, by defining assertions and warnings options in the submodule. Then I have to evaluate both of them outside of the submodule (with mapAttrsToList...) and I have to filter them out of the submodule's result where it is used to generate the configuration. I ultimatelly failed at encapsulating all this inside a mkRenamedAttrsOptionModule, it just requires too many assumptions about the design and usage of the submodule where it would be applied.
Note: I also encountered these PRs that try/tried to implement assertions and warnings at submodule level. Such an addition would probably be a good step towards a general solution for submodules.
- [Reverted] Module-builtin assertions, disabling assertions and submodule assertions #97023 , got reverted due to infinite recursion errors, has already many links to other PRs where it would be useful.
- nixos: reusable assertions #207187
| @@ -1,4 +1,5 @@ | |||
| { config, lib, pkgs, ... }: | |||
| { config, lib, options, pkgs, ... }: # XXX migration code for freeform settings: `options` can be removed after 23.11 | |||
There was a problem hiding this comment.
Since there is (as far as I know) no formal deprecation policy, it may be best to keep the migration code around for a release or three.
There was a problem hiding this comment.
I changed the comment to indicate the year 2025. That should be plenty of time.
(Also rebased onto current nixos-unstable and re-ran all tests)
| "extraConfig" "text" | ||
| "name" "server" "port" "node" "passwdDir" "includeExclude" |
There was a problem hiding this comment.
Did you consider adding a settings option on the server submodule, rather than commingling the freeform options with the existing ones? It might eliminate the need to ignore options here and simplify logic in a few places.
There was a problem hiding this comment.
Yes, but I don't think it's worth the effort: Most attribute keys listed here are due to the migration mechanism; as soon as the migration code gets removed, there are only two attributes left. The servername is usually defined with the attribute key below servers. So we would add another attribute set level only for a single option.
delroth
added
the
12.approvals: 1
Yarny0
force-pushed
the
tsm-freeform
branch
from
a0ee3f9 to
b1a6b54
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1114 |
delroth
removed
the
12.approvals: 1
|
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/2728 |
|
Is there anything I can do that might help getting this merged before breaking changes are restricted for NixOS 23.11 ? |
|
Rebased onto current
Tests still pass. Is there anything I can do that might help getting this merged? |
With the tsm-client 8.1.19.0 release, IBM renamed the product brand from "IBM Spectrum Protect" to "IBM Storage Protect": https://www.ibm.com/support/pages/node/6964770 . The package already got updated in commits 5ff5b2a and a4b7a62 . The commit at hand updates the modules accordingly.
`tsm-client` uses a global configuration file that must contain coordinates for each server that it is supposed to contact. This configuration consists of text lines with key-value pairs. In the NixOS module, these servers may be declared with an attribute set, where the attribute name defines an alias for the server, and the value is again an attribute set with the settings for the respective server. This is organized as an option of type `attrsOf submodule...`. Before this commit: Important settings have their own option within the submodule. For everything else, there is the "catch-all" option `extraConfig` that may be used to declare any key-value pairs. There is also `text` that can be used to add arbitrary text to each server's section in the global config file. After this commit: `extraConfig` and `text` are gone, the attribute names and values of each server's attribute set are translated directly into key-value pairs, with the following notable rules: * Lists are translated into multiple lines with the same key, as such is permitted by the software for certain keys. * `null` may be used to override/shadow a value that is defined elsewhere and hides the corresponding key. Those "important settings" that have previously been defined as dedicated options are still defined as such, but they have been renamed to match their corresponding key names in the configuration file. There is a notable exception: "Our" boolean option `genPasswd` influences the "real" option `passwordaccess', but the latter one is uncomfortable to use and might lead to undesirable outcome if used the wrong way. So it seems advisable to keep the boolean option and the warning in its description. To this end, the value of `getPasswd` itself is later filtered out when the config file is generated. The tsm-backup service module and the vm test are adapted. Migration code will be added in a separate commit to permit easy reversal later, when the migration code is no longer deemed necessary.
Check for spaces or duplicate names in server config keys.
Since server config keys are case insensitive,
a setting like
```
{
compression = "yes";
Compression = "no";
}
```
would lead to an ambiguous configuration.
To help users migrate from the previous settings to new freeform settings type, the commit at hand adds some `mkRemovedOptionModule` and `mkRenamedOptionModule`. These modules are not designed to work inside an attribute set of submodules. They create values for `assertions` and `warnings` to inform the user of required changes. Also, these informational texts do not contain the full attribute path of the changed options. To work around these deficiencies, we define the required options `assertions` and `warnings` inside the submodule and later add the values collected inside these options to the corresponding top-level options. In the course of doing so, we also add the full attribute path to the informational texts so the user knows these warning and error messages refer to the `tsmClient.servers` option. Also, we have to filter out `warnings`, `assertions`, and the "old" options when rendering the target config file.
|
Rebased again onto upcoming No changes otherwise. All tests still pass. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Description of changes
This pull request reworks the
tsm-clientmodule to be compliant with RFC42 a.k.a. freeform settings.tsm-clientuses a global configuration file that must contain coordinates for each server that it is supposed to contact, similar toHostentries in~/.ssh/config. Each server section contains several key-value pairs that can easily be represented by a freeform attribute set. Details of the change are explained in the corresponding commit message.Adding migration code for existing configurations is not easy, as the migration happens inside an
attrsOf submodule ...wheremkRemovedOptionModuleandmkRenamedOptionModuledon't work out of the box. This pull request adds additional options inside the submodule that are required by the migration modules to work properly. All this migration code is added in a separate commit so it can easily be reverted later when it is no longer deemed necessary. Details on how the migration mechanism works are also explained in the commit message.Besides, this pull request adds some assertions for more thorough checks on the validity of the server configuration settings and performs some minor cleanups.
As soon as this is merged, the
tsm-clientcheckbox in #144575 can be checked.Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Result of
nixpkgs-reviewrun on x86_64-linux 11 package blacklisted: