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

nixos/syncthing: readd settings.device.<name>.introducer option #262191

Closed
wants to merge 2 commits into from

Conversation

WillPower3309
Copy link
Contributor

@WillPower3309 WillPower3309 commented Oct 20, 2023

Description of changes

Seems this option was (accidentally?) removed in #226088

Things done

Tested that it works:
with nix config:

      devices.server = {
        id = "V5AV6D5-5ITLYTL-35UHX6S-LKMFZ6U-FVGLEZP-EFGGR3R-O6AVGG7-ONT5MQE";
        autoAcceptFolders = true;
        introducer = true;
      };

the following config.xml section is generated

    <device id="V5AV6D5-5ITLYTL-35UHX6S-LKMFZ6U-FVGLEZP-EFGGR3R-O6AVGG7-ONT5MQE" name="server" compression="metadata" introducer="true" skipIntroductionRemovals="false" introducedBy="">
        <address>dynamic</address>
        <paused>false</paused>
        <autoAcceptFolders>true</autoAcceptFolders>
        <maxSendKbps>0</maxSendKbps>
        <maxRecvKbps>0</maxRecvKbps>
        <maxRequestKiB>0</maxRequestKiB>
        <untrusted>false</untrusted>
        <remoteGUIPort>0</remoteGUIPort>
        <numConnections>0</numConnections>
    </device>
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 20, 2023
@WillPower3309
Copy link
Contributor Author

First PR, be gentle :)

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Oct 20, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 20, 2023
@h7x4
Copy link
Member

h7x4 commented Oct 21, 2023

Hello @WillPower3309, welcome to nixpkgs!

#226088 references a very notable RFC, named RFC 42. The essence of what this RFC is saying, is that we're going to have a hard time maintaining copies of the config schema of every single service out there. The better alternative is to use something known as freeform modules. You can declare an option (usually called settings, as specified in the RFC) with a type of pkgs.formats.<type> (e.g yaml, json, ini, etc.). Inside this option, you'd declare only the most important options (like hostname, port, or anything you deem a setting that most users would want to change), and let everything else be dynamic and undeclared. It will be up to the user to go look at the upstream documentation, to figure out what options are available.

This option should still be working fine, it's just not declared anymore, as it was probably deemed not important enough. I might be wrong about the last part though, and they just forgot it in the process 😅. If it does not work or causes errors, you can disregard this comment completely.

@WillPower3309
Copy link
Contributor Author

WillPower3309 commented Oct 21, 2023

Hello @WillPower3309, welcome to nixpkgs!

#226088 references a very notable RFC, named RFC 42. The essence of what this RFC is saying, is that we're going to have a hard time maintaining copies of the config schema of every single service out there. The better alternative is to use something known as freeform modules. You can declare an option (usually called settings, as specified in the RFC) with a type of pkgs.formats.<type> (e.g yaml, json, ini, etc.). Inside this option, you'd declare only the most important options (like hostname, port, or anything you deem a setting that most users would want to change), and let everything else be dynamic and undeclared. It will be up to the user to go look at the upstream documentation, to figure out what options are available.

This option should still be working fine, it's just not declared anymore, as it was probably deemed not important enough. I might be wrong about the last part though, and they just forgot it in the process 😅. If it does not work or causes errors, you can disregard this comment completely.

I see! Thanks for the context there. In the case of the syncthing module, the settings are set via the syncthing API which then writes to a config.xml. Would setting the values for that file as you described overwrite that file? If so, the rfc would be incompatible with how we use the module

@WillPower3309
Copy link
Contributor Author

WillPower3309 commented Oct 22, 2023

@Xyz00777 sorry for the ping, do you mind providing more context?

@Xyz00777
Copy link
Contributor

@Lassulus rfc42 and syncthing api config.
Can you provide more information you understand it way better than me 😅 thanks in advance

@Lassulus
Copy link
Member

The syncthing module uses the API, so only options which are present in your configuration are synced to the config.xml.
The code you posted:

      devices.server = {
        id = "V5AV6D5-5ITLYTL-35UHX6S-LKMFZ6U-FVGLEZP-EFGGR3R-O6AVGG7-ONT5MQE";
        autoAcceptFolders = true;
        introducer = true;
      };

should work already without this PR merged. In a freeform module we would just add the option for documentation sake. Also, if we really want to add this option, it should default to none as it is then removed from the API payload and the currently set value would persist. This is important because otherwise we would always override it to false if people would set it imperatively as in using the webinterface.

@WillPower3309
Copy link
Contributor Author

The syncthing module uses the API, so only options which are present in your configuration are synced to the config.xml.
The code you posted:

      devices.server = {
        id = "V5AV6D5-5ITLYTL-35UHX6S-LKMFZ6U-FVGLEZP-EFGGR3R-O6AVGG7-ONT5MQE";
        autoAcceptFolders = true;
        introducer = true;
      };

should work already without this PR merged. In a freeform module we would just add the option for documentation sake. Also, if we really want to add this option, it should default to none as it is then removed from the API payload and the currently set value would persist. This is important because otherwise we would always override it to false if people would set it imperatively as in using the webinterface.

Ah got it, I think I misunderstood the initial comment that the settings could be set to values other than those that are documented in the module, such as introducer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants