pterodactyl-panel: init at 1.15.1, pterodactyl-wings: init at 1.13.3, nixos/pterodactyl-{panel, wings}: init - #456521
pterodactyl-panel: init at 1.15.1, pterodactyl-wings: init at 1.13.3, nixos/pterodactyl-{panel, wings}: init#456521PadowYT2 wants to merge 5 commits into
Conversation
|
|
Pr title should be "nixos/pterodactyl-{panel, wings}: init" |
This comment was marked as outdated.
This comment was marked as outdated.
|
Since you're introducing both packages and modules, separate each into its own commit, with package init ones preceding the respective module init ones |
baf1adb to
edc4460
Compare
keenanweaver
left a comment
There was a problem hiding this comment.
Amazing work! Thanks for making this. I'm not a Pterodactyl user, but these modules will make me one.
Some observations/questions:
- Can you change the wings module to enable
virtualisation.dockerconditionally with it? Shouldn't be any harm in doing this instead of an assertion. - I know it's annoying, but each config option in both modules should have a description explaining what they're for.
- Are you able to nix-ify a full Pterodactyl setup with these modules? Can I programmatically configure eggs, etc. (I am ignorant of the software)
- May want to consider looking into service hardening
Also have some non-blocking nits:
🔥
Some of them aren't documented officially, will come up with something I guess.
I wish you could. There are no commands within Pterodactyl ( Will check out and apply changes you have suggested. As a first time contribution, honestly thanks for checking out the changes |
It's generally preferred to do everything possible upstream before doing patches and a lot of nix-specific fixes here. It all depends on how receptive upstream is to changes, especially if those changes are for Nix specifically.
You're welcome! I haven't tested them out on my own stuff yet, but I'm planning to deep dive with them and learn Pterodactyl at the same time. |
They wouldn't be nix-specific but will benefit the nix configuration more (and maybe some other setups, like install scripts) |
edc4460 to
a488dda
Compare
I will not do it for this PR, maybe some time in the future as it will take some time
The module is already way too complex for me and I have no clue what should I add. If you want to suggest something, please do |
keenanweaver
left a comment
There was a problem hiding this comment.
The module is already way too complex for me and I have no clue what should I add. If you want to suggest something, please do
Nothing in particular. Thanks for considering!
Some more changes to consider:
a488dda to
c057681
Compare
| phpPackage = lib.mkOption { | ||
| type = lib.types.package; | ||
| readOnly = true; | ||
| default = php; | ||
| description = "The PHP package to use"; | ||
| }; |
There was a problem hiding this comment.
Should this option also be using lib.mkPackageOption?
There was a problem hiding this comment.
I believe so. I just looked through the tree and found a couple of examples that do it for additional packages.
There was a problem hiding this comment.
Just tried, you can't use just lib.mkPackageOption "php" { } as the second option is supposed to be an object. So I guess just leave it like that?
There was a problem hiding this comment.
Probably want to leave it as it is now and we'll see if someone else chimes in.
4f8cf94 to
286d3cb
Compare
|
d9cf5b1 to
41e1971
Compare
41e1971 to
9c584cf
Compare
9c584cf to
bac7272
Compare
|
bac7272 to
64fa293
Compare
|
|
I am waiting for this. Is there a reason why this isn't merged yet? Am I able to help? |
👀 |
|
|
|
06kellyjac
left a comment
There was a problem hiding this comment.
Hey, great to see this. I look forward to trying it
Currently CI is failing due to the manual adjustments:
error: Cannot build '/nix/store/fb9n5wprzfs6wi250s08v242rc5wyglp-nixos-manual-html.drv'.
Reason: builder failed with exit code 1.
Output paths:
/nix/store/y1scwmg6bi4hrqnn6p5n7jwfwpp4nrgd-nixos-manual-html
Last 25 log lines:
> - module-services-pterodactyl-basic-usage
>
> This can happen when an identifier was added, renamed, or removed.
>
> Added new content?
> $ redirects add-content <identifier> <path>
> often:
> $ redirects add-content <identifier> index.html
>
> Moved existing content to a different output path?
> $ redirects move-content <identifier> <path>
>
> Renamed existing identifiers?
> $ redirects rename-identifier <old-identifier> <new-identifier>
>
> Removed content? Redirect to alternatives or relevant release notes.
> $ redirects remove-and-redirect <identifier> <target-identifier>
>
> NOTE: Run the right nix-shell to make this command available.
> Nixpkgs:
> $ nix-shell doc
> NixOS:
> $ nix-shell nixos/doc/manual
>
> NOTE: If your build passes locally and you see this message in CI, you probably need a rebase.
And some other changes raised
| services.mysql = lib.optionalAttrs cfg.database.createLocally { | ||
| enable = true; | ||
| package = pkgs.mariadb; | ||
| ensureDatabases = [ cfg.database.name ]; | ||
| ensureUsers = [ | ||
| { | ||
| name = cfg.database.user; | ||
| ensurePermissions."${cfg.database.name}.*" = "ALL PRIVILEGES"; | ||
| } | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
Considering we default to mariadb shouldn't our DB_CONNECTION = "mysql"; at the top actually be DB_CONNECTION = "mariadb"; (or better we derive it from the mysql package)
There was a problem hiding this comment.
The default provider is mysql and it works fine with a MariaDB server, but sure for pushing to just change the provider. Will do some testing before changing
There was a problem hiding this comment.
The mariadb provider has not been tested by the Pterodactyl team and doesn't work due to having strict GROUP BY rules, resulting in errors
There was a problem hiding this comment.
ok, weird they have a whole separate provider block that doesn't work with the db it's intended for lol.
Can we make sure there's a comment explaining this
| setupScript = pkgs.writeShellApplication { | ||
| name = "pterodactyl-wings-setup"; | ||
| runtimeInputs = with pkgs; [ | ||
| coreutils | ||
| replace-secret | ||
| ]; | ||
| text = '' | ||
| install -Dm640 -o ${cfg.user} -g ${cfg.group} ${ | ||
| (pkgs.formats.yaml { }).generate "config.yml" (lib.recursiveUpdate mainConfig cfg.extraConfig) | ||
| } ${cfg.rootDir}/config.yml | ||
|
|
||
| ${lib.optionalString (cfg.tokenIdFile != null) '' | ||
| replace-secret '@TOKEN_ID@' ${lib.escapeShellArg cfg.tokenIdFile} ${cfg.rootDir}/config.yml | ||
| ''} | ||
|
|
||
| ${lib.optionalString (cfg.tokenFile != null) '' | ||
| replace-secret '@TOKEN@' ${lib.escapeShellArg cfg.tokenFile} ${cfg.rootDir}/config.yml | ||
| ''} | ||
| ''; | ||
| }; |
There was a problem hiding this comment.
I dont think the secret replace is necessary
There was a problem hiding this comment.
_test.go files are test files, which are just unit tests. The actual logic for these specific secrets are here:
https://github.com/pterodactyl/wings/blob/d6116827313dae176ddf4741e233554392993398/config/config.go#L444-L449
There was a problem hiding this comment.
The test files are testing specific functionality. The tests were just the clearest usage/example I could link you
Here's a link for the ResolveToken method on the Configuration struct inside the working code. The godoc comment helps explain what it does.
https://github.com/pterodactyl/wings/blob/d6116827313dae176ddf4741e233554392993398/config/config.go#L414-L422
As long as you set the env var to file:///the/path/to/secret or nix level "file://${theNixVar}"; it'll read it, we don't need to template raw values into the config.
We should also use systemd's LoadCredential feature to ensure the service has access to the secret files.
We can define a services.pterodactyl.wings.secrets.tokenIDFile and services.pterodactyl.wings.secrets.tokenFile etc and when those are set we can auto define services.pterodactyl.wings.settings.token_id = "file://${cfg.secrets.tokenIDFile}"; etc.
There was a problem hiding this comment.
@PadowYT2 LMK when this adjustment is made or if you need help 🙂
There was a problem hiding this comment.
@PadowYT2 LMK when this adjustment is made or if you need help 🙂
This is done with the freeform options, which I kinda need help with (see your other comment about it)
|
Oh and since it's been added since you started the PR if you could please review the https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#automationai-policy and note if you're following the process in the PR body |
Thanks for a thorough review of the PR. I will try to solve/explain most of the things you commented on (been kinda a while since massive nix changes for me).
I'm unsure what to do, as everything has been done properly, with builds (nixpkgs-review) passing and GetPsyched has left his comment saying the redirects are fine. So I guess the note attached is what should be followed? |


Added Pterodactyl Panel and Pterodactyl Wings modules with their packages.
Homepage: https://pterodactyl.io
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.