-
-
Notifications
You must be signed in to change notification settings - Fork 483
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
module: add prometheus-node-exporter service #1129
Conversation
404c7a9
to
5a68aa6
Compare
1ec7f5d
to
f349f57
Compare
f349f57
to
3bada35
Compare
All requested changes applied. |
ce01d4a
to
6c8d45f
Compare
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.
Looks great to me now. @Enzime, did you want to have another pass over this?
Only one non‐blocking nit.
script = concatStringsSep " " | ||
([ | ||
(getExe cfg.package) | ||
"--web.listen-address" | ||
"${cfg.listenAddress}:${toString cfg.port}" | ||
] | ||
++ (map (collector: "--collector.${collector}") cfg.enabledCollectors) | ||
++ (map (collector: "--no-collector.${collector}") cfg.disabledCollectors) | ||
) + escapeShellArgs cfg.extraFlags; |
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.
Since we probably don’t want shell injection in the other arguments here, could just use escapeShellArgs
for the whole thing.
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.
LGTM
This PR adds a module for the Prometheus Node Exporter, which I want to use in the nixos.org infra.
I will test this shortly, but it is probably good enough for a review.
Options are similar to NixOS, but I chose a different namespace, since what NixOS has is probably overkill.