Skip to content

Conversation

@nathanregner
Copy link
Contributor

@nathanregner nathanregner commented Jun 3, 2025

Related: #136

Summary of commits

feat: add home-manager module

All options are supported minus openFirewall, user, group. The dataDir and runDir options are not shared, as they have slightly different defaults

Notes:

  • Perhaps we could share even more code here if we didn't use NixOS's abstracted service/socket options. home-manager doesn't help you out much here.

  • Systemd hardening options seem to break the tmux tests specifically. I didn't spend much time digging into this, so they're only enabled for the NixOS module.

refactor: factor out common config/refactor: add nixos- prefix to file names

For ease of reviewing changes, this was done before adding the home-manager module. Just moving code around for reusability.

To "prove" nothing changed, I recommend diffing these two commits with --color-moved.

I also checked the output of vm tests with nix-diff:

nom build .\#checks.x86_64-linux.nixos-simple.driver.nodes.server.system.build.toplevel
nix diff result ../master/result
...

bump: flake inputs

Update to NixOS 25.05 so we don't have to pin an old version of home-manager

@nathanregner nathanregner marked this pull request as draft June 3, 2025 01:51
Copy link
Owner

@Infinidoge Infinidoge left a comment

Choose a reason for hiding this comment

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

Few miscellaneous things:

  • I'd prefer it if the library stuff was moved to the root /lib, however if it is too annoying to implement then that can be a refactor I'll look into later.
  • Rename shared to common. I just think it sounds better.
  • Note that commit messages are not conventional commits for this repo. See CONTRIBUTING for more information.

@nathanregner
Copy link
Contributor Author

* I'd prefer it if the library stuff was moved to the root `/lib`, however if it is too annoying to implement then that can be a refactor I'll look into later.

I can totally do that. I avoided doing so to prevent mkOpt', mkBoolOpt', etc from becoming part of the public API. Honestly it was probably a bit overkill pulling those functions into their own files.

* Rename `shared` to `common`. I just think it sounds better.

Agreed

* Note that commit messages are **not** conventional commits for this repo. See [CONTRIBUTING](https://github.com/Infinidoge/nix-minecraft/blob/master/CONTRIBUTING.md) for more information.

Fixed (I think, wasn't exactly sure what do with the module refactors)

@nathanregner nathanregner marked this pull request as ready for review June 5, 2025 01:39
@Infinidoge
Copy link
Owner

I can totally do that. I avoided doing so to prevent mkOpt', mkBoolOpt', etc from becoming part of the public API. Honestly it was probably a bit overkill pulling those functions into their own files.

The main lib directory is already kinda a mix of everything tbh.
I should put in effort later to define a better internal vs external boundary, because stuff like rakeLeaves doesn't need to be exported by a Minecraft repo, and is honestly only exported as the lib flake output out of habit and not out of necessity.
That said, this repo doesn't really have a defined API, so this isn't particularly a problem at the moment.

Fixed (I think, wasn't exactly sure what do with the module refactors)

The refactor does throw a bit of a wrench into things that will have to be worked out in the CONTRIBUTORS.
I think module is for common/both, module/nixos for NixOS-specific, and module/home for home manager specific.

@nathanregner
Copy link
Contributor Author

The main lib directory is already kinda a mix of everything tbh. I should put in effort later to define a better internal vs external boundary, because stuff like rakeLeaves doesn't need to be exported by a Minecraft repo, and is honestly only exported as the lib flake output out of habit and not out of necessity. That said, this repo doesn't really have a defined API, so this isn't particularly a problem at the moment.

Makes sense. I went ahead and merged the module lib with the root lib.

I'm not super happy with the way I'm importing ../../lib everywhere. I also considered adding a self: parameter before the module arguments, but then it has to be passed through to ../common/minecraft-servers as well, which felt even worse.

@Infinidoge
Copy link
Owner

Infinidoge commented Jun 5, 2025

Could use mapAttrs in the root flake.nix to pass in lib (same as the packages), plus could import common there too and pass it via the flake as well. Not perfect, but removes the relative import, at least.

Copy link
Owner

@Infinidoge Infinidoge left a comment

Choose a reason for hiding this comment

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

Overall looks great! Couple small things, plus I have a few notes-to-self for potential refactors down the line, but nothing that stands in the way of this.

@nathanregner
Copy link
Contributor Author

Conflicts resolved: pulled dc09faa and 19658c5 into the common module

@Infinidoge
Copy link
Owner

Sorry for the lack of activity, uni and life have taken me off of GitHub for a while.

Code looks good, commits are clean, very good PR. Only thing missing is a bit of documentation. Could you document the home module a bit in the readme? (I really need to setup a docs website...)

Make sure there is a note about loginctl enable-linger. Without linger, the Minecraft server will be shut off after logging out of ssh, etc, which would be unexpected behavior.

@nathanregner
Copy link
Contributor Author

Sorry for the lack of activity, uni and life have taken me off of GitHub for a while.

Code looks good, commits are clean, very good PR. Only thing missing is a bit of documentation. Could you document the home module a bit in the readme? (I really need to setup a docs website...)

Make sure there is a note about loginctl enable-linger. Without linger, the Minecraft server will be shut off after logging out of ssh, etc, which would be unexpected behavior.

No worries whatsoever! Added docs but they may be a bit sparse; I can add more if you think it'd be helpful

@Infinidoge
Copy link
Owner

Sorry to throw more work on you.
I recently merged several PRs, which has led to a number of merge conflicts.
If it isn't too difficult, could you go ahead and resolve those?

@nathanregner
Copy link
Contributor Author

No worries! Looks like ff6604f, 9e8067f, 900f1d1 were the main conflicts and should be resolved now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants