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/earlyoom: add notificationsCommand option #53444
Conversation
@@ -63,6 +63,15 @@ in | |||
Enable debugging messages. | |||
''; | |||
}; | |||
|
|||
notificationsCommand = mkOption { | |||
type = types.string; |
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.
Use types.str
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 changed the type. What's the difference? Seems to do something with how it's merged?
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, types.string
concatenates strings for merging, which is almost never what you want, so that's why it's deprecated.
|
||
notificationsCommand = mkOption { | ||
type = types.string; | ||
default = null; |
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.
Can you provide an example in the example
attribute?
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 added example from earlyoom's readme. I didn't want to add it because it's fairly convoluted.
99ff817
to
e545c64
Compare
example = "sudo -u example_user DISPLAY=:0 DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus notify-send"; | ||
description = '' | ||
Command used to send notifications. | ||
See https://github.com/rfjakob/earlyoom#notifications for details. |
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.
Oh one more thing, you need to wrap the link in <link xlink:href="https://..."/>
to make it show up correctly in the rendered docs.
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.
done
Add option for specifying command that will be used for sending notifications. See https://github.com/rfjakob/earlyoom#notifications for details.
e545c64
to
5798d12
Compare
Motivation for this change
Add option for specifying command that will be used for sending notifications.
See https://github.com/rfjakob/earlyoom#notifications for details.
Also there is now a script in the repo for sending notifications to all users. When a new version of earlyoom will be released, perhaps it'll make sense to add an option to use this script.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)