nixos/xonotic: init #98739
nixos/xonotic: init #98739
Conversation
|
@GrahamcOfBorg eval |
|
|
||
| extraConfig = mkOption { | ||
| type = types.lines; | ||
| description = "extra config written to server.cfg"; |
There was a problem hiding this comment.
Descriptions should start with a capital letter and end with a period.
| type = types.attrsOf (types.oneOf [ types.str types.int types.bool ]); | ||
| description = '' | ||
| server config, see | ||
| <link xlink:href="https://github.com/xonotic/xonotic/wiki/basic-server-configuration">https://github.com/xonotic/xonotic/wiki/basic-server-configuration</link> |
There was a problem hiding this comment.
I'm not very good with our documentation format but I thought you could avoid duplicating the link and it will show up the same way.
|
|
||
| preStart = mkOption { | ||
| type = types.lines; | ||
| description = "command to execute before starting xonotic"; |
There was a problem hiding this comment.
It might be beneficial to explain this will run in a shell script before the service starts for some context. Additionally, naming this option to something a non developer/sysadmin would understand would be beneficial too. As it stands I feel like you need to be a NixOS developer to understand this option.
There was a problem hiding this comment.
I'm not good with writing descriptions. Any suggestions?
There was a problem hiding this comment.
@davidak has a passion for UX, so I wonder if he could suggest a better name for this option.
There was a problem hiding this comment.
thanks for recognizing that. i think UX is the most important thing for the success of a project, but i'm not a professional in that area :D
"preStart" and the description seems descriptive. mentioning that it is a shell command would also help.
i would question why it's needed at all. for installing additional maps, packaging them and adding to some list would be more reproducible and intuitive. but when the user wants to add more content, a dataDir option would be better
|
|
||
| openFirewall = (mkEnableOption "open udp port for xonotic") // { default = true; }; | ||
|
|
||
| package = mkOption { |
There was a problem hiding this comment.
This seems a bit overkill... but I won't complain if you really want to keep it.
|
|
||
| networking.firewall.allowedUDPPorts = let | ||
| firewallServers = lib.filterAttrs (name: host: host.openFirewall) cfg.servers; | ||
| in lib.mapAttrsToList (name: host: host.config.port) cfg.servers; |
There was a problem hiding this comment.
You should probably assert this value exists. This might be problematic because you also have an extraConfig in which the sysadmin could define this value...
There was a problem hiding this comment.
extra config is there, because there are cases in a xonotic config, which you cannot make as a key-value pair. But I hope everyone tries to not use it.
There was a problem hiding this comment.
That makes sense. A simple assertion will go a long way.
Kloenk
force-pushed
the
xonotic-module
branch
2 times, most recently
from
94d5250
to
800ae30
Compare
Kloenk
force-pushed
the
xonotic-module
branch
from
800ae30
to
0efec09
Compare
ofborg
bot
added
10.rebuild-darwin: 0
10.rebuild-linux: 1-10
10.rebuild-linux: 1
labels
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
Motivation for this change
Xonotic server module
Rewrite of #83761
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)CC: @petabyteboy