-
-
Notifications
You must be signed in to change notification settings - Fork 17.8k
Create bore nixos module #472353
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
base: master
Are you sure you want to change the base?
Create bore nixos module #472353
Conversation
a49e99b to
907b40a
Compare
nixpkgs-review builds all packages that were "changed" by a PR. You can view what was added, removed, and changed in the "eval summary" CI results. In this case: I don't think running nixpkgs-review adds any value in this case. |
MattSturgeon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with bore and can't comment on whether you're using or configuring it correctly. I'm also not intimately familiar with NixOS systemd services. I can review general nix and module system stuff though.
I think the biggest issue is that you've decided to cleanly separate the "server" and "local" stuff into separate files. As much of the implementation is identical, this leads to a lot of duplication.
We can DRY this up by combining similar files together and explicitly handling where local/server need to be different. Or alternatively, implement "common" things in a shared file and "special" things in separate files that import the shared file.
Also, when this PR is ready to be merged (or ideally, sooner while it's being reviewed), your commits will need some cleanup. See https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions
- Create one commit for each logical unit.
- If you have commits
pkg-name: oh, forgot to insert whitespace: squash commits in this case.
Usegit rebase -i. See Squashing Commits for additional information.
In this case that essentially means:
maintainers: add zsupernixos/services/bore: initnixos/tests/bore: init
|
When rebasing to squash the commits into the following:
There are a few commits that touched module files and test files. What should I do in that case? |
Rewriting git history can get tricky in that scenario. There are lots of fancy techniques, depending on your exact scenario and preferred workflow. The simplest approach is to kinda start from scratch. # Replace <base_commit> with the commit before your PR
# This will remove your commits, but keep their changes in the worktree
$ git reset --mixed <base_commit>
# Now you've reset the commits, you can re-create them...
# You can use `-p` to add individual hunks of a file
$ git add -p
# Or just add specific files or folders
$ git add nixos/tests
$ git commit -m "nixos/tests/bore: init"
# If you can copy the original commit's SHA hash,
# you can also re-use their commit message automatically,
# using --reuse-message or --reedit-message:
$ git add maintainers
$ git commit --reedit-message a9c9d68ebab918830489f8949f6a216ee215db2fIf you ever think you've lost a commit, check the |
|
Okay cool, that shouldn't be too bad then. And if I need to make any further changes after the fact (say another reviewer wants me to change the tests), should I just rebase those commits back into the |
This comes down to your personal workflow, but most reviewers will expect to see the commits rebased/amended when you make the change. Personally, I like to use Such fixup commits can either be pushed up to the PR; they demonstrate you plan to squash the change if it is accepted. Or you can do the rebase/squash before pushing; this leaves the PR in a "merge-ready" state, which is usually best. Another alternative approach is to use Then there are tools like (I'll minimise these comments as off-topic later, so the thread isn't too noisy) |
1042638 to
083d33f
Compare
a5634ba to
89a3d19
Compare
89a3d19 to
8c2fbc4
Compare
MattSturgeon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM. Code looks well structured and robust.
I'd ideally appreciate a review from someone who deals with systemd modules more than I do, if anyone is able to review.
Only thanks to your help! Seriously, I learned so much from this thread, thank you so much for taking the time to review! I'll be sure to use these norms & conventions for any future PRs (and general Nix code) I create :) |
This PR creates a NixOS module for
bore, a TCP tunneling program. More can be read about bore here. While Bore is already packaged withinnixpkgs(available atpkgs.bore-cli), its functionality can be greatly improved if integrated as a full NixOS module.This PR creates said module, exposing options like
services.bore.serversandservices.bore.local. Each.<name>entry within these attribute sets corresponds to a separate systemd service runningbore [local|server] ....As for the checklist below, I have tried running
nixpkgs-review, but it seems to kill my tmux terminal pane before showing any kind of useful output. I can debug & rerun it if needed for this PR. Additionally, I have not written official NixOS tests for this module yet (since I have yet to learn the necessary syntax for doing so), though I have tested the options thoroughly on my own servers. Writing the tests is something I plan on doing in the coming days (as I figure out how to do so, and also depending on the reception of this PR).EDIT: I have written NixOS tests and ensured they are passing.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.