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

Refactor nix to make it easier to import into other projects #7635

Merged
merged 2 commits into from
Nov 22, 2022

Conversation

trevorwhitney
Copy link
Collaborator

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.

@trevorwhitney trevorwhitney requested a review from a team as a code owner November 8, 2022 19:00
@grafanabot
Copy link
Collaborator

./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%

@grafanabot
Copy link
Collaborator

./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%

@trevorwhitney
Copy link
Collaborator Author

@simonswine @owen-d are probably best to review this one. thanks!

Copy link
Member

@jdbaldry jdbaldry left a 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!

Copy link
Contributor

@simonswine simonswine left a 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 🙂

@trevorwhitney
Copy link
Collaborator Author

@jdbaldry self.rev is set to the commit sha of self (since that is treated as a git repo), but only if the git tree is clean. Otherwise it's not set.

This is not well documented. I think I pieced this knowledge together from many searches across discourse.nixos.org, like this comment or this post

@trevorwhitney trevorwhitney merged commit 683efa0 into grafana:main Nov 22, 2022
@trevorwhitney trevorwhitney deleted the nix-refactor branch November 22, 2022 17:07
Abuelodelanada pushed a commit to canonical/loki that referenced this pull request Dec 1, 2022
…#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants