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

generator.dependencies as a list feels awkward in some cases #69

Closed
ldesgoui opened this issue Jan 26, 2025 · 5 comments
Closed

generator.dependencies as a list feels awkward in some cases #69

ldesgoui opened this issue Jan 26, 2025 · 5 comments

Comments

@ldesgoui
Copy link
Contributor

When generator.script uses deps in a homogeneous fashion, like in the example, it feels fine, but when I try to generate an environment file where each dep goes to a specific value, it becomes a bit of a burden, handling lists with nix isn't the nicest. I suppose lib.zipListsWith exists.

It feels like it should be possible to type generator.dependencies with oneOf [listOf secret, attrsOf secret], deps would become a list or attrset depending on the input. Heck, we could even have a pre-written env-from-dependencies script for the simple case.

Is this design acceptable? I can try my hand at implementing it, doesn't seem like too much work, but I haven't looked too deep yet.

@oddlama
Copy link
Owner

oddlama commented Jan 27, 2025

You're right, using a list wasn't the best idea. I don't know if it is worthwhile to change this now without breaking anything that already exists. For the medium term I'm rooting for a design like NixOS/nixpkgs#370444 which would provide a way to define generators upstream in nixos and deprecate our age-specific interface.

If you still want to have a go at it, feel free! If you end up with something that is compatible with current configurations we can definitely merge it.

@ldesgoui
Copy link
Contributor Author

ldesgoui commented Jan 27, 2025

I don't know if it is worthwhile to change this now without breaking anything that already exists.

I don't believe that changing the type from listOf unspecified to oneOf [listOf unspecified, attrsOf unspecified] would break any existing code, I could write tests to make sure of it

@oddlama
Copy link
Owner

oddlama commented Jan 27, 2025

What I meant is that currently all existing generators expect a list. If you only want to change the option and not the deps argument in the generators, then I guess there will be no way this can break. In any case, have a look at this snippet:

deps = flip map secret.generator.dependencies (dep: {
host = findHost dep;
name = dep.id;
file = relativeToFlake dep.rekeyFile;
});

Here, the deps argument is constructed. If you want the benefits of an attrset in your generator scripts, then you'd have to change this representation, and that may break generators that depend on it being a list. That's what I was referring to.

@ldesgoui
Copy link
Contributor Author

That's technically true but thinking about the bigger picture, a script that expects deps to be a list will always have dependencies be a list so I wouldn't expect any actual breakage

@oddlama
Copy link
Owner

oddlama commented Feb 4, 2025

Closed by #71

@oddlama oddlama closed this as completed Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants