-
Notifications
You must be signed in to change notification settings - Fork 101
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
Update configuration.nix #189
Update configuration.nix #189
Conversation
Awesome 🌟
|
3e4cad6
to
819ff1f
Compare
I squashed your fixup, clarified nix-bitcoin will auto-detect invalid settings in all other comments as well, removed the possible null value for However, I don't think it's a good idea for service modules to enable services they depend on or to define assertions directly in the relevant module files.
IMO this reduces modularity and should instead be handled in higher-level nix-bitcoin expressions like |
Great fixups, thanks! I'm still convinced we should add the assertions to the corresponding modules (like in NixOS) as they are already tightly coupled:
You could indeed add the |
Thanks for the explanation, I agree now on adding assertions to the modules. I'll work on implementing. I still am not convinced it's a good idea to cross-enable modules without the users explicit direction. But I'm of course willing to reconsider. Any comment on this @jonasnick ? |
Imo allowing modules to be interchangeable is only very rarely useful. Instead of using a different clightning module it should be possible to make the existing clightning module more flexible. Moreover, having more logic in the modules themselves simplifies writing and maintaining them which I clearly noticed when @erikarvstedt added the option to set something like As for modules implicitly enabling other modules, I'd prefer to not do that to allow users to easily see and opt-in to what software they actually use on their node (for now at least... the number of modules is still manageable). |
Remove the possible null value for bitcoind.prune and set prune = 0 in bitcoind as a default. Remove prune = 0 in secure-node.nix and the mkForce in configuration.nix (bitcoind.prune = lib.mkForce ).
HWI can be enabled if electrs is enabled as long as electrs.high-memory is disabled.
819ff1f
to
1f60bbc
Compare
Moved assertions to individual modules and split them up into separate commits. |
Fixups. |
1f60bbc
to
34fb412
Compare
Just realized we need to ignore the lnd, clightning exclusivity assertion in order to run the tests, do you know how to do that? |
I've added this to the fixups. |
34fb412
to
b9559ef
Compare
Had to slightly correct your fixup and added a comment, explaining the |
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.
ACK b9559ef
b9559ef
to
980d4f8
Compare
Changed the HWI assertion to check disablewallet directly, as this is the speicifc option that makes it incompatible. |
980d4f8
to
f280d54
Compare
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.
ACK f280d54
This PR updates
configuration.nix
comments to the most current inter-option dependencies and introduces anassertions.nix
to verify that all these dependencies are met.Closes #37