Skip to content
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/factorio: add extraSettings and package options #77478

Merged
merged 1 commit into from
Mar 30, 2020

Conversation

artemist
Copy link
Member

Motivation for this change

Currently there is no way to set game settings, such as administrators, other than those specified. There is also no way to use the experimental version.

Things done

The package option allows users to use the experimental version, or
override to a specific version with their own modified package.
extraSettings allows users to override default game settings without
adding many more settings.

I also tested this change and successfully ran a factorio server based on this.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Currently there is no way to set game settings, such as administrators.
extraSettings allows users to override default game settings without
adding many more settings.

The package option allows users to use the experimental version, or
override to a specific version with their own modified package.
@@ -37,7 +36,7 @@ let
only_admins_can_pause_the_game = true;
autosave_only_on_server = true;
admins = [];
};
} // cfg.extraSettings;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of adding an option to override all of the other configuration, why don't you extend the current options to include admins and others.

Also, you should probably be using something like mkMerge to ensure that the keys are gracefully composed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extending the current options seems like a good idea. I didn't want to bloat the options too much, but that would definately help with documentation. I'll update this PR and add an option for admins.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to create a factorio server with services.factorio and everything worked straight away. However, I can't make myself an admin, not even when I delete the state directory and restart the service with extraConfig = { admins = [ "username" ] } in place. Is extraConfig currently broken or am I doing something wrong? As far as I see it's generally not possible to connect a TTY to the server and run /promote <username>.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this issue as well, I'll look into it. But probably not til after the 20.09 release

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that being said, do you mind making an issue on github? better way to track than a comment, and please cc me

@veprbl veprbl merged commit 61a7975 into NixOS:master Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants