nixos-infra jitsi prep #297809
nixos-infra jitsi prep #297809
Conversation
There was a problem hiding this comment.
I'm not sure it's a good idea to add more technical debt in the Jitsi module to fix the fact that the Prosody module doesn't use a nice user-facing API. OTOH, I also use neither of those modules, so it would be nice to get active users / maintainers for this software to weigh in.
| @@ -135,6 +135,8 @@ The pre-existing [services.ankisyncd](#opt-services.ankisyncd.enable) has been m | |||
|
|
|||
| - `unrar` was updated to v7. See [changelog](https://www.rarlab.com/unrar7notes.htm) for more information. | |||
|
|
|||
| - `prosody` `services.prosody.muc.*.vcard_muc` was removed in favor of `services.prosody.muc.*.extraModules`. | |||
| type = types.bool; | ||
| default = true; | ||
| description = lib.mdDoc "Adds the ability to set vCard for Multi User Chat rooms"; | ||
| extraModules = mkOption { |
There was a problem hiding this comment.
Lists tend to be annoying because they make it very difficult to remove items from the default value. Can this be an attrset instead with e.g. "muc_mam".enable = true;?
(I actually don't know if we have something better as a common pattern for this in NixOS - I just know lists are annoying and that's how I would intuitively have go at fixing this.)
| extraModules = mkOption { | ||
| type = types.listOf types.str; | ||
| default = [ "muc_mam" "vcard_muc" ]; | ||
| description = "Add more modules to the muc Component"; |
There was a problem hiding this comment.
Capitalize MUC to match the documentation in other settings. Also remove the unnecessary capitalization from Component.
| updateMucs = mkOption { | ||
| type = with types; attrsOf anything; | ||
| default = { }; | ||
| description = "This is a very hacky work around setting and only added because prosody muc is a list."; |
There was a problem hiding this comment.
This descriptions tells me nothing at all about how this option is expected to be used or what it does. My understanding is that this is a way to inject extra Prosody settings for a MUC, which is difficult to do otherwise due to the combination of 1. the Prosody module not using an attrset for its instances; 2. the Jitsi module being in charge of configuring Prosody itself?
- Have you considered refactoring Prosody to use an attrset like what your
updateMucsoption uses as its API? It should even be doable to keep compatibility with the old list format by auto-transforming the list into an attrset using v.domain. - If this is not possible, please improve the naming and documentation here. How it's implemented should be of no concern to a user of the option and has (IMHO) no place in the documentation.
| @@ -205,7 +218,16 @@ in | |||
| tls = mkDefault true; | |||
| websocket = mkDefault true; | |||
| }; | |||
| muc = [ | |||
| # the piece of code bellow looks at the domain name of the attrs in the list and updates the attrs if the domain name matches. | |||
| runHook postInstall | ||
| ''; | ||
|
|
||
| # nothing todo here |
There was a problem hiding this comment.
typo: to do
(And it would be more useful to document why - which is because it's all just Lua scripts.)
|
I quit nixpkgs, and thus won't be finishing this. |
Description of changes
Adds all the fun pieces to implement nixos/infra#401
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 馃憤 reaction to pull requests you find important.