-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Generate unbound configuration files from blocklist files #2833
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?
Conversation
Yethal
commented
Mar 3, 2025
- Generate unbound.conf files from each hosts file in repository
- Add an overlay exposing the confs files under unboundconfs nested scope
- Unbound users consuming the flake are now able to import the blocklists via config similar to this:
Thank you for submitting this pull request! We’ll get back to you as soon as we can! |
flake.nix
Outdated
line | ||
else | ||
let | ||
address = builtins.elemAt (nixpkgs.lib.strings.splitString " " line) 0; |
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.
Why not split the string once in the let block instead of twice? What happens with null
?
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.
Not sure what you mean by null
with this case. We're splitting the hosts file on newline and apply transformation to all lines that aren't comments or empty lines
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.
elemAt
returns a value or null
. Are there cases where this can fail?
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.
Yes, on empty lines which we're skipping in the if statement
let | ||
pkgs = nixpkgsFor.${system}; | ||
dir = ./alternates; | ||
lists = |
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.
This block would probably run faster as a single fold instead of iterating the whole list 3 times (4 including the post-let
mapAttrs
). If it isn’t that expensive, I still think lib.pipe
would be a lot easier to read this case instead of trying to read the block backwards from lines bottom to top.
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.
When I originally wrote this I was using the experimental pipe-operator and then converted the code to the original pipe-less nix but lib.pipe does seem like an okay compromise
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.
Gotcha. The pipe change here I think is a lot easier for the reader to follow.
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 completely agree but I wasn't sure what's the consensus regarding usage of lib.pipe. In my own nix code I just use pipe operator everywhere but I understand this is not common practice
Why 2 commits reverting a formatting instead of amending to fix the single commit? Some of the formatting would be addressed tho with https://gitlab.com/StevenBlack/hosts/-/merge_requests/2 or #2813 |
Additionally, there is a smell about directories. You should probably be filtering on just |
e27f4a1
to
c18f148
Compare
@toastal I am filtering on hosts files, no other files within /alternates are considered, it's just pipe-less Nix code with multiple chained functions is difficult to read |
7e4631c
to
f0e982b
Compare
f0e982b
to
2cc50cc
Compare
Given the include field in unbound config is a list we could optimize this even further by only generating packages for non-combined list variants and allow users to combine them themselves by adding multiple entries to include list so instead of
People would do
|
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.
No opinion on trying to shorten the list, but generally looks good
This drops the amount of packages to evaluate from 30 to 4 so should speed up evaluation as well. |
@toastal Is there anything else to do here or can we merge? |
Nope. I am not a maintainer… I also have Nix changes open. |
@StevenBlack I believe I require your assistance with this. |