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

unifi-poller: add service and prometheus-exporter #96830

Merged
merged 4 commits into from Sep 8, 2020

Conversation

elseym
Copy link
Member

@elseym elseym commented Aug 31, 2020

Motivation for this change

also:

  • extends the prometheus-exporters tests to handle hyphenated exporter-names (by overriding the name of the testnode)
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS (just the package)
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@elseym elseym requested a review from WilliButz as a code owner August 31, 2020 13:30
@elseym
Copy link
Member Author

elseym commented Aug 31, 2020

oh well. didn't see #95994.
@bachp interested in merging our efforts?

Copy link
Member

@lheckemann lheckemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes I'd suggest, overall LGTM :)

nixos/modules/services/monitoring/unifi-poller.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/unifi-poller.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/unifi-poller.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/unifi-poller.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/unifi-poller.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/unifi-poller.nix Outdated Show resolved Hide resolved
nixos/tests/prometheus-exporters.nix Show resolved Hide resolved
nixos/tests/prometheus-exporters.nix Outdated Show resolved Hide resolved
nixos/tests/prometheus-exporters.nix Show resolved Hide resolved
@elseym
Copy link
Member Author

elseym commented Sep 3, 2020

@lheckemann thanks for the pointers. 🙂 i'll update the pr tomorrow.

Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this software - I just want to mention that we shouldn't have any password options, only passwordFile like options please.

nixos/modules/services/monitoring/unifi-poller.nix Outdated Show resolved Hide resolved
Copy link
Member

@lheckemann lheckemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While a little refactoring on the prometheus exporters side wouldn't be amiss (in order to allow the warnings to apply to prometheus-exporter use, not just standalone use of the poller), I think blocking this addition on said refactoring wouldn't make much sense.

I'm happy with how the other points have been addressed, so I'd be all for merging it — @aanderse is that alright with you?

Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without understanding the software itself module LGTM 👍

@elseym
Copy link
Member Author

elseym commented Sep 6, 2020

thanks for all your help.
the modules now only allow password files and provide the default passwords via writeText. i chose not to use the option name passwordFile but rather keep pass to minimise juggling with the generated json data and keep the modules more maintainable. the provided descriptions and option types should make the usage clear enough.

@lheckemann lheckemann merged commit ef4e81d into NixOS:master Sep 8, 2020
@lheckemann lheckemann deleted the unifi-poller branch September 8, 2020 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace unifi_exporter with unifi-poller (prometheus exporter)
4 participants