-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nixos/matrix-synapse: migrate to generators.toYAML and other cleanup #120260
nixos/matrix-synapse: migrate to generators.toYAML and other cleanup #120260
Conversation
d32f59d
to
fd35a10
Compare
184e779
to
ead0cb4
Compare
981e6c1
to
8d783b4
Compare
8d783b4
to
f3062d3
Compare
27f2410
to
a232790
Compare
40f5be3
to
d3e8994
Compare
@ajs124 how did you inspect the test machine and find the contents of the files? |
d3e8994
to
3dffbc2
Compare
Also, add the neccessary mkChangedOptionModule imports. This matches what is in the ratelimiting config in Synapse. See https://github.com/matrix-org/synapse/blob/develop/docs/sample_config.yaml#L822
Not sure why they were ever strings. This converts the strings to integers if that was how th user specified it.
3dffbc2
to
20c1540
Compare
I evaluated the test from master and with this PR applied. Then I used |
=> error: Could not convert 1 to float. | ||
*/ | ||
# Obviously, it is a bit hacky to use fromJSON this way. | ||
toFloat = str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'm qualified to approve this, although I guess it seems fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@infinisil (or somebody else), could you maybe approve this addition to lib/strings.nix
Was removed in matrix-org/synapse@1c4f05d |
This option was removed in 2016 by this commit: matrix-org/synapse@1c4f05d
Synapse's cipher string has been updated to require ECDH key exchange. Configuring and generating dh_params is no longer required, and they will be ignored. matrix-org/synapse#4229 https://github.com/matrix-org/synapse/blob/develop/CHANGES.md#features-48
20c1540
to
aeb092a
Compare
Thanks for finding this! I've updated the commit message. |
|
||
configFile = pkgs.writeText "homeserver.yaml" ( | ||
generators.toYAML { } (filterAttrs (_: v: v != null) | ||
(fold recursiveUpdate { } [ yamlConfig cfg.settings ]))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing this, set the defaults in the config
section by assigning to services.matrix-synapse
directly with mkDefault
's. Check out https://nixos.org/manual/nixos/stable/index.html#ex-settings-nix-representable for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand what you mean. I don't think I am setting any defaults here... it's just collapsing all of the individual options that already exist on the module into a single attrset so that it can be converted to YAML.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right. Still should be done this way though. E.g. see the how the ZNC module provides support for legacy options (ignore the mkOverride 900
bit):
nixpkgs/nixos/modules/services/networking/znc/options.nix
Lines 231 to 261 in 7de613d
services.znc.config = let | |
c = cfg.confOptions; | |
# defaults here should override defaults set in the non-legacy part | |
mkDefault = mkOverride 900; | |
in { | |
LoadModule = mkDefault c.modules; | |
Listener.l = { | |
Port = mkDefault c.port; | |
IPv4 = mkDefault true; | |
IPv6 = mkDefault true; | |
SSL = mkDefault c.useSSL; | |
URIPrefix = c.uriPrefix; | |
}; | |
User.${c.userName} = { | |
Admin = mkDefault true; | |
Nick = mkDefault c.nick; | |
AltNick = mkDefault "${c.nick}_"; | |
Ident = mkDefault c.nick; | |
RealName = mkDefault c.nick; | |
LoadModule = mkDefault c.userModules; | |
Network = mapAttrs (name: net: { | |
LoadModule = mkDefault net.modules; | |
Server = mkDefault "${net.server} ${optionalString net.useSSL "+"}${toString net.port} ${net.password}"; | |
Chan = optionalAttrs net.hasBitlbeeControlChannel { "&bitlbee" = mkDefault {}; } // | |
listToAttrs (map (n: nameValuePair "#${n}" (mkDefault {})) net.channels); | |
extraConfig = if net.extraConf == "" then mkDefault null else net.extraConf; | |
}) c.networks; | |
extraConfig = [ c.passBlock ]; | |
}; | |
extraConfig = optional (c.extraZncConf != "") c.extraZncConf; | |
}; |
The reason it should be done this way is because otherwise if other modules try to access config.services.matrix-synapse.settings
, they won't have the full view of the set options, but rather only a partial one that doesn't include the ones from yamlConfig
here. And in general recursiveUpdate
should be avoided in modules, because the module system already supports merging by itself, and a much better version of it.
}; | ||
|
||
configFile = pkgs.writeText "homeserver.yaml" ( | ||
generators.toYAML { } (filterAttrs (_: v: v != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also check out #75584, which shows how you can use pkgs.formats.yaml
for both the type and the generator in one.
And the filterAttrs
here looks like it only filters top-level null
's out, is that intended like this? And is there a problem with leaving in the null
s?
Superseded by #158605. |
Motivation for this change
generators.toYAML
function which is more maintainable.toFloat
library function to assist in converting strings to floats (this is modeled aftertoInt
).verbose
andbcrypt_rounds
options toint
suser_creation_max_duration
option (not found in synapse codebase at all)uploads_path
option (Removed here: Remove vestiges of uploads_path config matrix-org/synapse#9462)tls_dh_params_path
option (synapse_admin_mau:current metric is not updated when not limiting usage matrix-org/synapse#4229)expire_access_token
option (Remove non-functional 'expire_access_token' setting matrix-org/synapse#5782)I am doing all of these deprecations at once to try and avoid
mkChangedOptionModule
chains.Note to Reviewers
Manual testing performed
Test 1
I tested using the following config:
Test 1 Nix config
ExecStart
line:jq
for clarity):Generated config (piped through `jq`)
Test 2 (deprecated conversions)
I tested using the following config:
Test 2 Nix config
The following warnings were shown:
I ensured that Synapse runs without error.
I looked at the generated homeserver.yaml file to ensure that everything looked correct. Here is the generated config (piped through
jq
for clarity):Generated config (piped through `jq`)
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)