-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
thanos: init at 0.6.0 & NixOS module #60500
Conversation
@GrahamcOfBorg build thanos The following contains some preliminary tests for thanos: |
@Mic92 I see you've done some work on |
930bccf
to
8ef1718
Compare
I really don't like having more such huge modules in nixpkgs. Having options for every config setting has some serious downsides, check out NixOS/rfcs#42 if you haven't seen it already. |
@infinisil thanks for pointing me to that RFC. I didn't see it yet. You raise some interesting points. I guess I'll have to make my mind up what my opinion is on the matter. |
@GrahamcOfBorg test prometheus2 |
4aeb105
to
13c264b
Compare
@GrahamcOfBorg test prometheus2 |
@GrahamcOfBorg test prometheus2 |
@infinisil regarding NixOS/rfcs#42, I understand the motivation for it: NixOS modules that encode every setting as a NixOS option are, to quote the RFC, "time consuming to implement, tedious to review and hard to maintain". This is true. I do think though that the time invested in implementing, reviewing and maintaining the module will benefit the user significantly. She'll get a module which is
OTOH reducing the time spent implementing, reviewing and maintaining modules is also important so I'm willing to investigate a
|
I've tested the binary and this works as expected. |
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.
I have not tested yet, unsure if I can afford to do so myself. This is a pure CR.
(mkIf cfg.sidecar.enable { | ||
systemd.services.thanos-sidecar = { | ||
wantedBy = [ "multi-user.target" ]; | ||
after = [ "network.target" "prometheus2.service" ]; |
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.
should we enable prometheus (services.prometheus.enable
) or write an assertion for it?
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.
I think an assertion is an excellent idea! I'll add it.
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.
Awesome! Thank you for adding the assertions. Not a blocker, but it would be great to add a test for the assertions.
runCommandNoCC "bzr-no-cert-validation" { | ||
inherit bazaar; | ||
buildInputs = [ makeWrapper ]; | ||
} "makeWrapper $bazaar/bin/bzr $out/bin/bzr --add-flags -Ossl.cert_reqs=none"; |
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.
is there a way this can go into go-modules instead?
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.
We could add it to nativeBuildInputs
by default but that would cause an unnecessary build dependency for all go-modules (except for thanos
of course).
However git
is also added by default so there's precedent. OTOH git
is used by most go-modules while bzr
isn't.
In general I think it's worthwhile to have support for overriding go-modules.
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.
I understand the need to be able to support overriding the go-modules and I would leave that. I'm just thinking that any package using bzr
will have to copy your hack over to their package and most likely spend time figuring out why they're getting a TLS error in the first place. We could maybe control it with a flag defaulting to false? Something like enable-bzr ? false
?
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.
Apparently thanos-0.5.0
doesn't depend on bzr
anymore so I removed the overrideModAttrs
setting.
It would still be nice to add a enableBzr
flag but this can be done in a separate PR.
@GrahamcOfBorg test prometheus2 |
The test fails on
I'll increase the disk space on |
@GrahamcOfBorg test prometheus2 |
@GrahamcOfBorg test prometheus2 |
I'll leave in the support for overrideModAttrs because that can be useful for other go packages.
This is to fix the following error in the test on aarch64-linux: store# [ 126.911144] thanos[739]: level=error ts=2019-06-16T14:00:26.59870538Z caller=main.go:182 msg="running command failed" err="error executing compaction: first pass of downsampling failed: create dir: mkdir /var/lib/thanos-compact/downsample: no space left on device" store# [ 126.942655] systemd[1]: thanos-compact.service: Main process exited, code=exited, status=1/FAILURE
@GrahamcOfBorg test prometheus2 |
Please don't merge since this is work-in-progress.EDIT: this is now ready for review.Motivation for this change
I would like to use Thanos in NixOS.
0.4.0 Release notes.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)