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

statsd: restore support for {influx,librato,stackdriver} backends #32051

Merged
merged 2 commits into from Dec 4, 2017

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Nov 25, 2017

Motivation for this change

adds backends (follow-up for #31235)

These packages will be placed into an environment using
backendsToPackages. This function explicitly maps backends to
pkgs.nodePackages.${type} unless it's a builtin (some backends
are directly available in the statsd source tree). This ensures that only
valid backends that work on NixOS are used (if not, the build already
breaks at evaluation time).

The log will be redirected to stdout to be able to watch the entire
output using journalctl.

Configuration parameters for the backends need to be set using
services.statsd.extraConfig as each backend has its own options and
all of them shouldn't be validated and checked explicitly and manually.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@Ma27
Copy link
Member Author

Ma27 commented Nov 25, 2017

I think it might be the best option to add a full-blown testcase for statsd, then we should be fine :)

@Ma27 Ma27 changed the title statsd: restore support for {influx,librato,stackdriver} backends WIP statsd: restore support for {influx,librato,stackdriver} backends Nov 25, 2017
@Ma27 Ma27 mentioned this pull request Nov 25, 2017
8 tasks
@Ma27 Ma27 changed the title WIP statsd: restore support for {influx,librato,stackdriver} backends statsd: restore support for {influx,librato,stackdriver} backends Nov 26, 2017
@Ma27
Copy link
Member Author

Ma27 commented Nov 26, 2017

@garbas I added a simple testcase and removed the WIP label. If everything is fine for you, we can merge this now :-)

@Ma27 Ma27 force-pushed the statsd/add-plugins branch 2 times, most recently from e4452e0 to d582406 Compare November 26, 2017 09:28
@Mic92
Copy link
Member

Mic92 commented Nov 26, 2017

@GrahamcOfBorg test statsd

Copy link

@GrahamcOfBorg GrahamcOfBorg left a comment

Choose a reason for hiding this comment

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

Failure for system: x86_64-linux

error: attribute ‘statsd’ in selection path ‘tests.statsd’ not found

@Mic92
Copy link
Member

Mic92 commented Nov 26, 2017

If your test is not in nixos/release.nix it will be never executed by hydra.

@Ma27
Copy link
Member Author

Ma27 commented Nov 26, 2017

ah good to know :-)

@Mic92
Copy link
Member

Mic92 commented Nov 26, 2017

@GrahamcOfBorg test statsd

Copy link

@GrahamcOfBorg GrahamcOfBorg left a comment

Choose a reason for hiding this comment

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

Failure for system: x86_64-linux

error: anonymous function at /var/lib/gc-of-borg/.nix-test-rs/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-zoidberg/nixos/tests/statsd.nix:1:25 called with unexpected argument ‘system’, at /var/lib/gc-of-borg/.nix-test-rs/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-zoidberg/nixos/tests/make-test.nix:5:41

@Ma27
Copy link
Member Author

Ma27 commented Nov 27, 2017

@Mic92 how does the bot call these tests? (it's actually the first test I contribute to nixpkgs and I just used nix-build ./nixos/tests/statsd.nix :)

@garbas garbas self-requested a review November 28, 2017 13:59
These packages will be placed into an environment using
`backendsToPackages`. This function explicitly maps backends to
`pkgs.nodePackages.${type}` unless it's a builtin. This ensures that only
valid backends that work on NixOS are used (if not, the build already
breaks at evaluation time).

The log will be redirected to `stdout` to be able to watch the entire
output using `journalctl`.

Configuration parameters for the backends need to be set using
`services.statsd.extraConfig` as each backend has its own options and
all of them shouldn't be validated and checked explicitly and manually.
@Ma27
Copy link
Member Author

Ma27 commented Dec 4, 2017

@garbas I just resolved the merge conflict introduced by the autogenerated node2nix expressions, so this should be mergable again :)

Copy link
Member

@garbas garbas left a comment

Choose a reason for hiding this comment

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

tnx!

@garbas garbas merged commit af75b48 into NixOS:master Dec 4, 2017
@Ma27 Ma27 deleted the statsd/add-plugins branch December 4, 2017 18:31
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.

None yet

4 participants