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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

grafana-alloy: init at 1.0.0 #306048

Closed
wants to merge 1 commit into from
Closed

grafana-alloy: init at 1.0.0 #306048

wants to merge 1 commit into from

Conversation

claudiiii
Copy link
Contributor

Description of changes

Adding (grafana-)alloy, a rewrite of grafana-agent.

Closes #303418

@flokli @emilylange I added you as maintainers as you already maintain grafana-agent and signaled interest in maintaining alloy as well. I hope that's ok!

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 馃憤 reaction to pull requests you find important.

@flokli
Copy link
Contributor

flokli commented Apr 22, 2024

@flokli @emilylange I added you as maintainers as you already maintain grafana-agent and signaled interest in maintaining alloy as well. I hope that's ok!

Sure. Do you also want to become a maintainer of this package?

@claudiiii claudiiii requested a review from flokli April 24, 2024 21:59
@hbjydev
Copy link
Contributor

hbjydev commented Apr 28, 2024

Would very much like to see this merged soon :)

@hbjydev
Copy link
Contributor

hbjydev commented Apr 29, 2024

Also, not sure if this is correct, but having fixed some stuff with the package locally, it looks like it requires >8GB of /tmp space to build? I use tmpfs for my /tmp so it broke for me. Otherwise I was tempted to ask you to add me as a maintainer too because I'll be using this package a lot lol

@flokli
Copy link
Contributor

flokli commented Apr 29, 2024

It's not just a matter of pressing the merge button.

Ongoing maintenance, as well as proper testing of this (and future releases) is important, and so far neither me nor @emilylange got a chance to test it, there's no VM tests and no thoughts around a NixOS module.

Additionally, the initial PR author adding this package didn't react on the question on whether they also want to maintain it, nor update it to pass CI.

Yes, if you intend to maintain it too, and work on these bits, you might want to open a PR by yourself. I might find some time this week to do some testing on my side as well.

@claudiiii
Copy link
Contributor Author

I'm not gonna add myself as a maintainer (especially not with the current controversies). That's why I asked if it's ok to add you as maintainers.

I'm personally only interested in the convert functionality. But if someone want's to add a module and vm tests I'm very open to that.

@hbjydev
Copy link
Contributor

hbjydev commented Apr 29, 2024

Yes, if you intend to maintain it too, and work on these bits, you might want to open a PR by yourself. I might find some time this week to do some testing on my side as well.

I was more referencing that I can't maintain it because of system-specific build issues than anything else, not trying to make comments about anything outside of that, but that I would have otherwise liked to help maintain it.

@flokli
Copy link
Contributor

flokli commented Apr 29, 2024

Also, not sure if this is correct, but having fixed some stuff with the package locally, it looks like it requires >8GB of /tmp space to build? I use tmpfs for my /tmp so it broke for me. Otherwise I was tempted to ask you to add me as a maintainer too because I'll be using this package a lot lol

Yeah, unfortunately nix builds inside /tmp, which is a problem with larger packages. It's a long-known Nix issue, tracked in NixOS/nix#4137 (comment). You can check if it works if you disable tmp on tmpfs, but I fully understand to have reasons for having it like this :-)

I'm not gonna add myself as a maintainer (especially not with the current controversies). That's why I asked if it's ok to add you as maintainers.
I'm personally only interested in the convert functionality. But if someone want's to add a module and vm tests I'm very open to that.

Both fair points, thanks for the clarification. As I stated, I'd be up to maintaining it, but only if/once I use it, so I'll see if I get to migrating some of my configs to it, and cough up a NixOS module (and test) for it. I'll keep this PR updated (and probably push them to it if that's ok)

@claudiiii
Copy link
Contributor Author

I was more referencing that I can't maintain it because of system-specific build issues than anything else, not trying to make comments about anything outside of that, but that I would have otherwise liked to help maintain it.

Ah sorry I wasn't clearer, this was meant as a reply to @flokli :)

hbjydev

This comment was marked as resolved.

@hbjydev
Copy link
Contributor

hbjydev commented May 7, 2024

Nevermind, I misread the other comments.

@hbjydev
Copy link
Contributor

hbjydev commented May 7, 2024

@flokli Could you re-review this and approve/request more changes? This has been sat for a little while due to that requested change :)

@flokli
Copy link
Contributor

flokli commented May 7, 2024

I didn't have a chance to test-drive this on my systems, and am not willing to merge a PR adding myself as a maintainer if I don't use and maintain it (yet). Additionally, I got some IRL things to deal with.

Please remove me from the list of maintainers, and check with @emilylange if they're willing to adopt this package.

rev = "v${version}";
owner = "grafana";
repo = "alloy";
sha256 = "sha256-G+lLxdUnE07QXt2wBcS6K3DVHIS35aKCh0TZCzpNgBE=";
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer hash = ... to sha256 = ... Both currently work, but the former will make it easier if the algorithm ever needs to change.

Since there could be different hashing algorithms in use, this make sense.

More info: https://nixos.org/manual/nixpkgs/stable/#fetchurl

Nixpkgs contributors are currently recommended to use hash.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

There's a hash mismatch on Darwin, can you please check it out or adjust meta.platforms accordingly?

@geekodour
Copy link
Contributor

@claudiiii are you working on the change @drupol suggested?

@claudiiii
Copy link
Contributor Author

@geekodour No, I don't have the capacity for this right now, also I'm not using the package any more. I'm gonna close this PR.

To anyone, please feel free to implement the requested changes on top of mine and create a new PR 馃檪

@claudiiii claudiiii closed this Jun 2, 2024
@vunso
Copy link

vunso commented Jun 6, 2024

Thanks a lot @claudiiii for your changes, I have created new PR #317741 馃檪

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.

Package request: Grafana Alloy
7 participants