Skip to content
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/clightning: init module #50531

Closed
wants to merge 1 commit into from

Conversation

@jb55
Copy link
Contributor

commented Nov 17, 2018

I copied a lot of stuff from the openvpn module for this one to support multiple networks at once.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

nixos/clightning: init module
Signed-off-by: William Casarin <jb55@jb55.com>

@jb55 jb55 requested a review from Infinisil as a code owner Nov 17, 2018

options = {

dataDir = mkOption {
type = types.path;

This comment has been minimized.

Copy link
@jb55

jb55 Nov 17, 2018

Author Contributor

forgot default here

{
options = {
services.clightning = {
networks = mkOption {

This comment has been minimized.

Copy link
@erikarvstedt

erikarvstedt Nov 26, 2018

Member

The name networks seems to be a leftover from openvpn. I think nodes would be more fitting.

# clightning requires bitcoin-cli to talk to either spruned or bitcoind
path = [ pkgs.altcoins.bitcoind ];

after = [ "network.target" ];

This comment has been minimized.

Copy link
@erikarvstedt

erikarvstedt Nov 26, 2018

Member

There's no network.target for user units (see systemctl --user list-units -t target). You can remove this dependency.

config = mkIf (cfg.networks != {}) {

systemd.user.services =
listToAttrs (mapAttrsFlatten (name: value: nameValuePair "clightning-${name}" (mkLightningNode value name)) cfg.networks);

This comment has been minimized.

Copy link
@erikarvstedt

erikarvstedt Nov 26, 2018

Member

Can be simplified to

mapAttrs' (name: value: nameValuePair "clightning-${name}" (mkLightningNode value name)) cfg.networks;

};
}
'';

This comment has been minimized.

Copy link
@erikarvstedt

erikarvstedt Nov 26, 2018

Member

I'd remove the whitespace here, one blank line should be enough.

};
};
});

This comment has been minimized.

Copy link
@erikarvstedt

erikarvstedt Nov 26, 2018

Member

Minor: These blank lines between brackets can be removed. There's more of them down below.

config = mkOption {
type = types.lines;
description = ''
Configuration of this clightning node.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Nov 27, 2018

Member

Can you link to the docs for the configuration here?

@jb55

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2018

I'm having trouble getting both mainnet and testnet nodes to play nice together on the same machine, so I'm going to shelve this for now ...

@jb55 jb55 closed this Dec 8, 2018

@jb55

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2018

ooops nm I was just confused...

@jb55

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

I'm going to close this for now until I have time to fix some issues with it

@jb55 jb55 closed this Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.