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: repackage with node2nix #31235

Merged
merged 1 commit into from Nov 5, 2017
Merged

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Nov 4, 2017

Motivation for this change

In #31032 it's attempted to get entirely rid of npm2nix.
One our packages which still used npm2nix is statsd.

Due to the fact that it's part of the NPM registry, it could be added to the nodePackages set.

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 4, 2017

(/cc @svanderburg )

@Ma27
Copy link
Member Author

Ma27 commented Nov 4, 2017

currently running nox-review.

@Ma27
Copy link
Member Author

Ma27 commented Nov 4, 2017

fast-cli breaks, however it breaks as well when running nix-build -A fast-cli on master without my change, so this seems unrelated.

@svanderburg
Copy link
Member

svanderburg commented Nov 4, 2017

@Ma27 The fast-cli breakage is indeed unrelated. I fixed this package some time ago, but it appears to be broken again.

Have you also tested the NixOS module that runs it as a service? I think you also need to update the package reference there and check whether the service still works.

@Ma27
Copy link
Member Author

Ma27 commented Nov 4, 2017

@svanderburg ah, didn't know about the module, will test it tonight :-)
The package reference doesn't need to be updated, I aliased nodePackages.statsd to statsd in all-packages.nix for compliancy reasons.

@Ma27
Copy link
Member Author

Ma27 commented Nov 5, 2017

@svanderburg the services.statsd module still works for me.

I checked it using the following VM configuration from my branch:

{
  statsd = {
    users.extraUsers.vm = {
      isNormalUser = true;
      password = "vm";
    };

    services.statsd.enable = true;
  };
}

The executable exists, the systemd services running on the default port (8125 in that case), the config defaults will be generated by the nixos module, it seems as nothing needs to be changed.

@svanderburg
Copy link
Member

@Ma27 Cool! I guess this issue is resolved then!

@svanderburg svanderburg merged commit 51536b2 into NixOS:master Nov 5, 2017
@Ma27 Ma27 deleted the statsd-with-node2nix branch November 5, 2017 12:50
@svanderburg
Copy link
Member

@Ma27 I also just fixed the fast-cli package, so this should no longer be an issue

@garbas
Copy link
Member

garbas commented Nov 15, 2017

@Ma27 @svanderburg Is there a reason statsd backends were not included?

-, "statsd-librato-backend"
-, "stackdriver-statsd-backend"
-, "statsd-influxdb-backend"

@garbas
Copy link
Member

garbas commented Nov 15, 2017

Could we add this packages to the list?

@Ma27
Copy link
Member Author

Ma27 commented Nov 24, 2017

@garbas sure, will do this on the weekend :)

@garbas
Copy link
Member

garbas commented Nov 24, 2017

thank you, thank you, thank you! :)

@Ma27
Copy link
Member Author

Ma27 commented Nov 25, 2017

@garbas see #32051 (still WIP, but already testable)

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