-
Notifications
You must be signed in to change notification settings - Fork 108
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
add hosts module arg #164
add hosts module arg #164
Conversation
This looks fairly benign, perhaps a usage example in the docs would be pertinent |
Agreed a usage example would be nice. I'm not sure where to put one. This change could be useful for profiles, modules, and hosts. But I guess hosts would still be the best place to include it. |
Actually, I was wanting to add a section to the docs, something like |
Yeah that would be perfect |
If we do add |
d2358f7
to
24505ab
Compare
switched to use _module.args based on comment in nixpkgs/lib/modules.nix. hosts arg should never be used in an imports line - evaluated modules can't be imported again,. |
215b3c9
to
0144da6
Compare
I increased the scope of this PR to cover #169. Now lib functions can access self and any input. And inputs is passed as a special arg, since that could be used in an imports statement. Self is passed as a module arg since I can't see a reason to why it would be part of an imports statement. |
LGTM I think it might be still too early for a smdocumented usage example. As I understand it, this idea came up towards a possible way reacting to another hosts configuration. That is mostly it's identity (hostname, ip, systemd machine id, — maybe static link local ipv6?) I think you need to rebase? |
Yeah exactly, although with self and inputs I think this PR has more value. It is now an alternative method of passing inputs to your hosts instead of extern. So for mobile-nixos you can't use the extern modules section, because you can't add the pinephone module to all hosts. So using the inputs specialArg would be better. You can also use self.packages for cross compiling. I don't know much about wireguard, but I would imagine that you might need to know information about other hosts to create the config for each host. |
I think either way we're blocked due to the ci issues. And I think this PR is pretty much ready to merge. |
flake.nix
Outdated
@@ -57,7 +57,7 @@ | |||
overlay = import ./pkgs; | |||
overlays = lib.pathsToImportedAttrs (lib.pathsIn ./overlays); | |||
|
|||
lib = import ./lib { inherit nixos pkgs; }; | |||
lib = import ./lib (inputs // { inherit nixos pkgs self; }); |
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 was debating over doing this or just making it available as inputs
. I prefer this way so a lib function could just do something like { nix-darwin, ...}:
and get access to nix-darwin.
The alternative would be { inputs, ... }: inputs.nix-darwin.lib.darwinSystem
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.
And also a lib function could do args@{ ... }:
and remove pkgs
self
and lib
to get access to all inputs. This is probably only useful for #190
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.
Maybe let's keep it name spaced as inputs
. Similar to self
this is well-known self-explaining top level flake glossary.
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 personally prefer this method because it feels more in alignment with the callPackage
paradigm - or in this case callLib
. You specify the things you need for your function and you get those inputs. If we just pass inputs itself, then you take everything, which is more similar to the module system.
But I'm ok with either, they both get the job done.
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.
hm, for the sake of consistency, we probably shouldn't import it differently than how we import it with ./hosts
, though.
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.
hm, for the sake of consistency, we probably shouldn't import it differently than how we import it with ./hosts, though.
But do we really need consistency between lib and hosts? they are two different things.
@nrdxp Which one would you prefer? I think once decide how to pass inputs to lib, this should be ready.
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.
nix.registry = mapAttrs (n: v: { flake = ${v}; }) inputs;
look clearer, indeed 😉
No need to be aware if some additional unwanted attributes sneak in and cause nasty side effects in unexpected places.
(over: nix.registry = mapAttrs (n: v: { flake = ${v}; }) (removeAttrs inputs [ "self" "pkgs" "lib" ]);
)
So this PR now only covers passing inputs to lib and hosts. And it adds the hosts module arg as a convenience. |
Do we actually want to pass |
My reasoning is theres no harm in doing so. And it helps with flakes containing nixosModules that you only want in one host. And |
Is this an issue despite lazy evaluation? (sorry my ignorance)
Did you have any specific use case in mind?
Well, it dilutes the interface. I don't think that's good on such a general basis which affects I mean all of a sudden, we have to ways to accomplish the same. This is a recipe for trouble. EDIT: Until we have our interfaces sorted out, I'm leaving the |
I removed that part. I realize its a pretty niche use case and also I can just do it manually in |
Thank you @Pacman99 for all this inspiration and thought fodder. Interaction / discussion has it all! ❤️ |
Indeed it does! Thank you too! |
Also this is back to its original state, just adds hosts module arg. So I think its ready to merge. |
bors delegate+ |
✌️ Pacman99 can now approve this pull request. To approve and merge a pull request, simply reply with |
bors r+ |
👎 Rejected by too few approved reviews |
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.
bors r+
Build succeeded: |
should help with #163. Fixes #169
But either way this could be generally useful. I have one use case of setting up a minecraft bungeecord proxy with servers on different hosts.