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

Aria2 module settings #303429

Merged
merged 3 commits into from
Jun 12, 2024
Merged

Aria2 module settings #303429

merged 3 commits into from
Jun 12, 2024

Conversation

timhae
Copy link
Contributor

@timhae timhae commented Apr 11, 2024

Description of changes

aria2 module settings, added nixos test and myself as maintainer

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.

@timhae
Copy link
Contributor Author

timhae commented Apr 11, 2024

Result of nixpkgs-review pr 303429 run on x86_64-linux 1

1 package blacklisted:
  • nixos-install-tools

Copy link
Contributor

@ambroisie ambroisie left a comment

Choose a reason for hiding this comment

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

I would split the addition of the test from the new settings option.

Could you also either not change the formatting, or split it into a different commit?

Otherwise thank you for adding a test to the module! This makes me much more confident for refactoring it now :-)

nixos/modules/services/networking/aria2.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/aria2.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/aria2.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/aria2.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/aria2.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/aria2.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/aria2.nix Outdated Show resolved Hide resolved
nixos/tests/aria2.nix Show resolved Hide resolved
nixos/tests/aria2.nix Outdated Show resolved Hide resolved
nixos/tests/aria2.nix Outdated Show resolved Hide resolved
@timhae
Copy link
Contributor Author

timhae commented Apr 16, 2024

Thanks for your comments, I have tried to address all of them. Please let me know what you think.

I also took the liberty to add myself as module maintainer
@drupol drupol merged commit 7d270d5 into NixOS:master Jun 12, 2024
24 checks passed
@jys1670
Copy link

jys1670 commented Jun 14, 2024

I am getting error: The option 'services.aria2.settings' is used but not defined. on the latest nixos-unstable.
Was this meant to happen? From a brief look, it seems that services.aria2.settings should be defined with some default values.
@timhae

@timhae
Copy link
Contributor Author

timhae commented Jun 15, 2024

@jys1670 can you please post your services.aria2 settings in your nixconfig?

@jys1670
Copy link

jys1670 commented Jun 15, 2024

@jys1670 can you please post your services.aria2 settings in your nixconfig?

Yeah, sure:

  services.aria2 = {
    enable = true;
    rpcSecretFile = "${pkgs.writeText "aria" "aria2rpc\n"}";
  };

It's working fine if I revert this PR. Should something else be defined for default settings on latest nixos-unstable?
--show-trace (just in case)

@timhae
Copy link
Contributor Author

timhae commented Jun 15, 2024

Sorry for breaking your config, can you change that to:

  services.aria2 = {
    enable = true;
    rpcSecretFile = "${pkgs.writeText "aria" "aria2rpc\n"}";
    settings = { };
  };

@jys1670
Copy link

jys1670 commented Jun 15, 2024

Sorry for breaking your config, can you change that to:

  services.aria2 = {
    enable = true;
    rpcSecretFile = "${pkgs.writeText "aria" "aria2rpc\n"}";
    settings = { };
  };

Thanks! This fixed the issue for me.

@timhae
Copy link
Contributor Author

timhae commented Jun 15, 2024

created PR #320035

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.

6 participants