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
riot-web: accept conf override as attrset and str #81452
Conversation
c9e5cca changed the type from str to attrset, which broke some people's config. GitHub: closes NixOS#81416
☑️ works for me with string |
with an attrset I fail to overwrite the keys in This default_server_config = {
m.homeserver = {
base_url = "https://example.org";
server_name = "example.org";
};
m.identity_server = {
base_url = "";
};
}; becomes (json) "default_server_config": {
"m.homeserver": {
"base_url": "https://matrix-client.matrix.org",
"server_name": "matrix.org"
},
"m.identity_server": {
"base_url": "https://vector.im"
},
"m": {
"homeserver": {
"base_url": "https://example.org",
"server_name": "example.org"
},
"identity_server": {
"base_url": ""
}
}
}, |
@mguentner You can use quotes around keys in the nix expression. |
Indeed. Thanks :) |
configOverrides = writeText "riot-config-overrides.json" (builtins.toJSON (noPhoningHome // conf)); | ||
}); | ||
userOverrides = writeText "riot-config-user.json" ( | ||
with builtins; if isAttrs conf then toJSON conf else conf |
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 a fan of options that have no proper type.
I think it would be better to add a configFile
option for this: it would still be a breaking change but it would only happen in the next relase, with a mention in the release notes.
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.
We could only backport this PR and leave attrset
-only for the future.
Let's keep the compatibility with string configuration only for release-19.09. |
Motivation for this change
c9e5cca changed the type from str to attrset, which broke some people's config.
GitHub: closes #81416
@mguentner please test
This probably needs to be back-ported.
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)