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/tox-node: init #59368
nixos/tox-node: init #59368
Conversation
fdbfe47
to
3569d48
Compare
cfg = config.services.tox-node; | ||
homeDir = "/var/lib/tox-node"; | ||
|
||
bootstrapNodes = [ |
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.
Where is this list coming from? Is it possible to pull it from the tox-node
package?
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.
From here: https://github.com/tox-rs/tox-node/blob/master/dpkg/config.yml
Is it possible to pull it from the tox-node package?
It is easier to add them into the module.
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.
@adisbladis does this answer satisfy you?
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 reluctant to merge this PR as-is. I don't like the additional maintenance burden put on maintainers while updating the package.
A lot of our users are not on NixOS and will possibly miss updating the service.
It's not a lot of extra lines of code to pull this data from the package.
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 going to actually maintain this module.
It's not a lot of extra lines of code to pull this data from the package.
It's not very clear how to do this. Would you like to explain how to pull node list from the package?
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.
Another possibility is to reconstitute the config out-of-store at runtime
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.
but does it make sense to have this as an option so the user can override
I believe so.
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 don't think we would need IFD at all.
I'll hack a POC of what I have in mind :)
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.
Check out my changes in my nixpkgs fork: https://github.com/adisbladis/nixpkgs/tree/tox-node (adisbladis@984461c).
I noticed that the dpkg
directory is not in tox-node-0.0.7
yet so I had to resort to using fetchurl
on this file separately for now.
Feel free to cherry-pick (and optionally squash) this change if you are satisfied with it.
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.
Feel free to cherry-pick (and optionally squash) this change if you are satisfied with it.
Done.
c931726
to
967a1cd
Compare
r? |
You'll have to elaborate on that. |
Github says “changes required” even though I've applied necessary changes. |
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.
In general LGTM
I'd like to hear back from @adisbladis on the bootstrapNodes
first, though.
Bonus points if anyone else who uses tox-node
could test this as well.
Aside from my notes on hard-coded bootstrap nodes this PR looks fine. |
Get these from upstream tox-node package instead. This is likely to cause less maintenance overhead over time and following upstream bootstrap node changes is automated.
Looks great now! Thanks for your contribution :) |
This PR broke tarball build due to the missing descriptions on the module options. (Sorry if the tone seems a bit rough, just stating what I found out.) Adding more details, in case it confuses people: The tarball build is more strict and will abort the build when any warning is produced. I do not know if there are easier methods to catch these warnings other than doing the tarball build. |
The |
@joachifm since it happens at build time no, ofborg doesn't build untrusted inputs automatically, only evals untrusted inputs automatically. |
Whoops.. Descriptions added in 9a176d6 |
Thanks @adisbladis! |
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Right now it uses the same uid as tox-bootstrapd, this probably should be changed.