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

chronograf: init at 1.7.11 #64703

Closed
wants to merge 4 commits into from

Conversation

NefixEstrada
Copy link

Motivation for this change

Adding Chronograf to NixOS to have the whole TICK stack available. Also, I want to use it :P

Things done
  • Created the chronograf package
  • Created the chronograf module
  • Added myself to the maintainers list
Requirements
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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 nix-review --run "nix-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.

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.

Welcome to nixpkgs and thank you for contributing! I have left a few comments on the NixOS module I hope you will find helpful. I'm not a yarn2nix (or nodejs on nix in general) person, so I'll leave it to someone else to review that code.

Please let me know if you have any questions at all.

maintainers/maintainer-list.nix Show resolved Hide resolved
nixos/modules/misc/ids.nix Outdated Show resolved Hide resolved
nixos/modules/misc/ids.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/chronograf.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/chronograf.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/chronograf.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/chronograf.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/chronograf.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/chronograf.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/chronograf.nix Outdated Show resolved Hide resolved
@NefixEstrada
Copy link
Author

@aanderse Everything should be ready now! About the maintainers stuff, do you mean another PR?

@aanderse
Copy link
Member

@NefixEstrada If you're familiar with rewriting git history I mean you should rewrite the history of this PR such that the change you made to maintainers/maintainer-list.nix should be a single commit, before the commit adding the package and service. The other 2 commits can be squashed into a single commit.

If you're not familiar with this git workflow please read up on git history rewriting and feel free to ask if you have any questions.

edolstra and others added 4 commits July 16, 2019 14:07
This works around 'failed to open:
/homeless-shelter/.cargo/.package-cache' with Rust 1.36 even when
we're using 'cargo --frozen'.
- Added the chronograf package
- Added the chronograf module
@NefixEstrada
Copy link
Author

It seems that I also edited the 2 commits previous than the one mines. I'm not really sure how this happened. Sorry for adding everyone to the conversation.

Other than that, I separated the maintainers commit and squashed the other to into a single commit. Should I try to do the same without modifying those commits? If so, could someone give me a hand? Thanks!

(Commands I've executed)
git rebase -i HEAD~2
git reset HEAD^
git add maintainers/maintainer-list.nix
git commit
git add .
git commit
git rebase --continue
git push --force

@ofborg ofborg bot added the 6.topic: rust label Jul 16, 2019
};

dataDir = mkOption {
default = "/var/db/chronograf";
Copy link
Member

Choose a reason for hiding this comment

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

Does this match the upstream default?

--port=${toString cfg.port} \
--bolt-path=${cfg.dataDir}/chronograf-v1.db ${if cfg.influxdbURL != "" then ''\
--influxdb-url=${cfg.influxdbURL}'' else ""} ${if cfg.kapacitorURL != "" then ''\
--kapacitor-url=${cfg.kapacitorURL}'' else ""} ${if !cfg.reporting then ''\
Copy link
Member

@Mic92 Mic92 Jul 16, 2019

Choose a reason for hiding this comment

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

You can use in all three cases lib.optionalString in case the variables are empty.

@@ -17,11 +17,11 @@ let
llvmShared = llvm_7.override { enableSharedLibraries = true; };
in stdenv.mkDerivation rec {
pname = "rustc";
version = "1.35.0";
version = "1.36.0";
Copy link
Member

Choose a reason for hiding this comment

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

The rust upgrade seems unrelated.

src = "${src}/ui";

packageJSON = ./package.json;
yarnLock = ./yarn.lock;
Copy link
Member

Choose a reason for hiding this comment

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

I think yarn2nix does not work in the mode nixpkgs evaluates.

@Mic92
Copy link
Member

Mic92 commented Jul 16, 2019

+10000 lines of yarn2nix is quite a lot for nixpkgs. Is there a way to integrate this into pkgs/development/node-packages/node-packages-v10.json?

@aanderse
Copy link
Member

@NefixEstrada are you motivated to continue with this PR?

@NefixEstrada
Copy link
Author

If someone else wants to continue with the PR, feel free to do it. I feel that my Nix skills level might be too low for correctly add the service.

If you tell me how should I solve the issues that you've pointed out, I can finish the work, but otherwise I'm not going to be able to finish it :(

@aanderse
Copy link
Member

aanderse commented Sep 4, 2019

@NefixEstrada which issues are you specifically having trouble with? The issues I raised seem to be resolved by you for the most part, with the exception of git history. If you're specific on what you're having issues with maybe someone can help you out.

@NefixEstrada
Copy link
Author

It's both the git history and the fact that the package.json shouldn't be in nixpkgs

@Mic92
Copy link
Member

Mic92 commented Sep 25, 2019

For grafana we ended up taking the javascript assets from the binary package and only building the go project itself. Might be a viable option here to avoid the dependency hell.

@sorpaas
Copy link
Member

sorpaas commented Jan 4, 2020

For anyone who wants to use chronograf in the mean time, a docker version is simple to write. I also found https://github.com/m1cr0man/nix-configs/tree/9520cc569f690d0f3603be91c637cef23d00ffff/packages/chronograf but that does not seem to build, unfortunately.

  docker-containers."chronograf" = {
    after = [ "influxdb.service" ];
    args = [
      "--network host"
      "-v /var/lib/chronograf:/var/lib/chronograf"
      "--user ${chronografUid}:${chronografGid}"
      "chronograf"
    ];
  }

Comment on lines +69 to +72
buildFlagsArray = [ ''-ldflags=
-s
-X main.version=${version}
'' ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Use proper list for compatibility with #72074

Suggested change
buildFlagsArray = [ ''-ldflags=
-s
-X main.version=${version}
'' ];
buildFlags = [
"-ldflags="
"-s"
"-X" "main.version=${version}"
];

cp ./build/* $out
'';

installPhase = '':'';
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not dontInstall = true work?

"d '${cfg.dataDir}' 0770 chronograf chronograf - -"
];

systemd.services.chronograf = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to reuse the upstream service file after patching it?

@stale
Copy link

stale bot commented Jul 2, 2020

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 2, 2020
@Mindavi
Copy link
Contributor

Mindavi commented Nov 27, 2021

Closing. Can be reopened or preferably, resubmitted if someone still finds this useful.

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

9 participants