-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Refactor nix to make it easier to import into other projects #7635
Conversation
./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0.1%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
@simonswine @owen-d are probably best to review this one. thanks! |
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.
LGTM.
TIL: about self.rev
. Do you know of any documentation I can read to understand this better?
I'm not sure if my review is enough for merging but I can at least it confirm that I was able to hop into the shell environment and build Loki with Nix. It's the first time in a while I've built Loki and it's super cool to see us use Nix in some our projects!
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.
LGTM
I think your overlays are more package definition as you are not overriding things within existing packages (with .extend
or .override
), you defining them from scratch again (see https://nixos.wiki/wiki/Overlays). But I think we should still merge it at is 🙂
…#7635) **What this PR does / why we need it**: This makes it easier for projects that depend on Loki (like enterprise logs) to more easily import the Loki nix flake in order to use the overlays and configurations it contains. It also removes the `build-vars.nix` because it was super annoying without really getting us much.
What this PR does / why we need it:
This makes it easier for projects that depend on Loki (like enterprise logs) to more easily import the Loki nix flake in order to use the overlays and configurations it contains.
It also removes the
build-vars.nix
because it was super annoying without really getting us much.