-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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/nix-{gc,optimise}: make dates
parameter more consistent
#371483
base: master
Are you sure you want to change the base?
Conversation
dates
parameter more consistentdates
parameter more consistent
dates
parameter more consistentdates
parameter more consistent
@@ -86,7 +86,7 @@ in | |||
description = "Nix Garbage Collector"; | |||
script = "exec ${config.nix.package.out}/bin/nix-collect-garbage ${cfg.options}"; | |||
serviceConfig.Type = "oneshot"; | |||
startAt = lib.optional cfg.automatic cfg.dates; | |||
startAt = lib.optionals cfg.automatic cfg.dates; |
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 want a potentially nested list here
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 check if it is a string and then put it into a list so we always have a list?
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.
Yeah, I was wondering if that was necessary - I took this from the original nix-optimise
but it looked weird to me.
type = with lib.types; listOf str; | ||
default = "03:45"; | ||
type = with lib.types; either singleLineStr (listOf singleLineStr); | ||
example = "weekly"; | ||
description = '' | ||
Specification (in the format described by |
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.
We probably should adapt the description to mention that it can be a list
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've slightly changed the description attribute for both; the type
is also displayed directly on the options pages so that should be clear enough in combination?
67f1782
to
5b01dab
Compare
5b01dab
to
45856c7
Compare
I'll link #345573 here. It'd be nice to eventually standardize on one representation for dates. |
Ah, I didn't even know about that issue. If we can decide on the "correct" way I don't mind extending this PR to include autoUpgrade. In my implementation I chose to make gc/optimise more relaxed about their inputs rather than deprecate something. Personally I am content if I can at least use a consistent format in my config, I don't mind which one it is. |
There are many other modules than the specific three mentioned there that could do with being standardized. I just set up this search, using regex to avoid instances of
Of these, I'd say the main three options are I personally think lists are the most clear when it comes to signaling to the user that multiple dates are accepted, but it would be a breaking change for many modules. Most people will also only put in one date, so they would end up constantly having to go through the inconvenience of a wrapping list. If we decided not to go with Final note - if we were to make a treewide change, I'd like to actually make a |
To be honest, I was looking to scale back my nixpkgs time a bit this year and I just wanted to fix some personal annoyances. I don't think I'm the right person to spearhead this PR if the scope blows up so much. |
Totally fine! I can make a future PR, then. I'll put my vote in for standardizing on a string. It breaks the least people's workflows, and matches the systemd standard. |
Kind of a followup to #362366
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.