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.

Sign up for GitHub

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

Jump to bottom

nixos/fedimintd: init services #322815

Merged
merged 1 commit into from
Oct 3, 2024
Merged

nixos/fedimintd: init services #322815

merged 1 commit into from
Oct 3, 2024

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Jun 27, 2024

Description of changes

Follow-up to #316309 adding nixos module and a basic vmtest.

Based on https://github.com/NixOS/nixpkgs/blob/nixos-24.05/nixos/modules/services/networking/bitcoind.nix , which has similar requirements (like being able to run multiple instances for different Bitcoin networks) and is a closely related project.

I also tried to mimic what I've seen in #314440 because I happen to watch that PR.

This is a first time I'm adding NixOS module to nixpkgs, so please be patient with me.

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.11 Release Notes (or backporting 23.11 and 24.05 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.

@dpc dpc force-pushed the 24-06-26-fedimintd-service branch from a0090f7 to 022dd6b Compare June 27, 2024 05:23
@dpc dpc mentioned this pull request Jun 27, 2024
Merged
13 tasks
@dpc dpc force-pushed the 24-06-26-fedimintd-service branch from 022dd6b to 9f88c63 Compare June 27, 2024 05:28
SuperSandro2000
SuperSandro2000 reviewed Jun 27, 2024
View reviewed changes
nixos/tests/fedimintd.nix Outdated Show resolved Hide resolved
nixos/tests/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated
};

nginx = {
enable = mkEnableOption "fedimint";
Copy link
Member

Choose a reason for hiding this comment

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

We should probably change the description here to Wether to configure nginx for fedimint or similar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC no other way than to convert to mkOption for that, so did that.

nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
@dpc dpc force-pushed the 24-06-26-fedimintd-service branch from 9f88c63 to 667bf1e Compare June 27, 2024 18:00
@dpc
Copy link
Contributor Author

dpc commented Jun 27, 2024

@SuperSandro2000 Lots of useful things I didn't catch. Thank you so much.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1817

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1887

@dpc dpc force-pushed the 24-06-26-fedimintd-service branch 3 times, most recently from d033800 to a13232f Compare August 5, 2024 21:16
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4586

@dpc
Copy link
Contributor Author

dpc commented Sep 30, 2024

Pleeeease? :D

@h7x4 h7x4 self-requested a review September 30, 2024 23:38
@h7x4
Copy link
Member

h7x4 commented Sep 30, 2024

I don't have time to review today, but I'll see what I can do tomorrow. Ping me if nothing has happened by friday :)

h7x4
h7x4 reviewed Oct 1, 2024
View reviewed changes
Copy link
Member

@h7x4 h7x4 left a comment

Choose a reason for hiding this comment

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

Here's a few comments. Maybe you could add a release note as well?

nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
@dpc dpc force-pushed the 24-06-26-fedimintd-service branch 3 times, most recently from 8a9ee8a to f2b47ff Compare October 2, 2024 22:37
@dpc dpc force-pushed the 24-06-26-fedimintd-service branch 6 times, most recently from 7314c1d to 74c0351 Compare October 2, 2024 23:20
@dpc
Copy link
Contributor Author

dpc commented Oct 2, 2024

@h7x4 I think I addressed all issues, yours (thank you!) and some other ones I found while setting up configurations based on this PR.

@dpc dpc force-pushed the 24-06-26-fedimintd-service branch from 74c0351 to b788777 Compare October 2, 2024 23:27
@dpc dpc force-pushed the 24-06-26-fedimintd-service branch 3 times, most recently from 9e6b800 to 32642c6 Compare October 3, 2024 16:57
@dpc
Copy link
Contributor Author

dpc commented Oct 3, 2024 •
edited
Loading

I'm running nix run nixpkgs#nixfmt nixos/modules/services/networking/fedimintd.nix, and it doesn't make any changes anymore, so why is nixfmt-check step not like me?

@otech47
Copy link

otech47 commented Oct 3, 2024

ack

@h7x4
Copy link
Member

h7x4 commented Oct 3, 2024

I'm running nix run nixpkgs#nixfmt nixos/modules/services/networking/fedimintd.nix, and it doesn't make any changes anymore, so why is nixfmt-check step not like me?

I believe nixpkgs#nixfmt still refers to the old formatter (aliased to nixfmt-classic). Try using the nixfmt executable from the nixfmt-rfc-style package, maybe manually via nix shell nixpkgs#nixfmt-rfc-style

@dpc dpc force-pushed the 24-06-26-fedimintd-service branch from 32642c6 to 7d583c3 Compare October 3, 2024 18:29
@dpc
Copy link
Contributor Author

dpc commented Oct 3, 2024

nix run nixpkgs#nixfmt-rfc-style nixos/modules/services/networking/fedimintd.nix did it. I thought I was already using nixfmt-rfc-style the first time it failed, so I confused myself believing I need to use the classic one.

@dpc dpc mentioned this pull request Oct 3, 2024
Draft
h7x4
h7x4 approved these changes Oct 3, 2024
View reviewed changes
Copy link
Member

@h7x4 h7x4 left a comment

Choose a reason for hiding this comment

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

Okay, I think it's good to go. I have a few more comments, nothing important.

nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/fedimintd.nix Show resolved Hide resolved
@dpc dpc force-pushed the 24-06-26-fedimintd-service branch from 7d583c3 to ba72798 Compare October 3, 2024 20:05
@h7x4
Copy link
Member

h7x4 commented Oct 3, 2024

Great stuff, thank you for the contribution!

@h7x4 h7x4 merged commit c760c83 into NixOS:master Oct 3, 2024
23 checks passed
@dpc
Copy link
Contributor Author

dpc commented Oct 3, 2024

Thank you and previous reviewers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h7x4 h7x4 h7x4 approved these changes

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (new) 8.has: module (update) 8.has: tests 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dpc @nixos-discourse @h7x4 @otech47 @SuperSandro2000