nixos/syncthing: Use API to merge / override configurations

[doronbehar]

Description of changes

If one sets either of override{Device,folder}s to false, the jq * operator doesn't merge well the devices and folders, creating duplicate IDs for folders as observed in #230146. This PR makes the script iterate via a Bash for loop the devices and folders IDs and merges the keys using upstream's curl -X PATCH support for single objects.

Hence this commit fixes #230146.

Things done

• 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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.05 Release Notes (or backporting 22.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.

[ncfavier]

creating duplicate IDs for folders

If that's the only problem, why not just fix the jq script to use unique_by(.id)?

[doronbehar]

If that's the only problem, why not just fix the jq script to use unique_by(.id)?

I tried to modify the jq script without success though indeed perhaps it's possible. The suggested solution is also much more readable in my opinion, and more robust.

[Lassulus]

there is a small refactor happening in #226088 so maybe we merge that and you could rebase the PR?
I struggle a bit with the mixing of jq and bash since it's making the code a bit harder to follow for me

[doronbehar]

I struggle a bit with the mixing of jq and bash since it's making the code a bit harder to follow for me

I totally understand, and I can at least add more comments, let's see what can we do after #226088 is merged.

[doronbehar]

@ofborg test syncthing-init syncthing

[doronbehar]

The tests work. I updated the commit to include many more comments. Your review is welcome @Lassulus .

[ncfavier]

#230196 (comment) still holds.

[doronbehar]

@ncfavier I am not sure we want to use unique_by(.id), see the example from jq's website:

https://jqplay.org/jq?q=unique_by(.foo)&j=%5B%7B%22foo%22%3A%201%2C%20%22bar%22%3A%202%7D%2C%20%7B%22foo%22%3A%201%2C%20%22bar%22%3A%203%7D%2C%20%7B%22foo%22%3A%204%2C%20%22bar%22%3A%205%7D%5D

It can't really differentiate (in our case) between the array object that was created by the Nix configuration, vs the object that was further expanded by Syncthing. I think we want to be sure that the folder / device we add via the API have the same other attributes as those of the Nix configuration.

This adds up to the readability advantage I find in the current script: The difference between curl -X argument PUT vs POST for /rest/config/{device,folder}s is documented in syncthing's website and it comes handy specifically for our case. Plus, perhaps in the future we can add more options to the service so that even the folders could be PATCHed if chosen by the user. Same goes for the other options, ldap and gui settings - to allow a mixture of imperative and declarative behavior for these settings as well.

If you find some sophisticated way of performing this logic via jq, I'd be willing to review it, but personally it proved to be much harder then it seemed to me, especially in comparison to the solution currently implemented.

[doronbehar]

I tested these changes as part of trying to reproduce #232679 - with and without my patch, and it works for me (had to fix small Bash issue).

[ncfavier]

I think this PR can probably wait until 23.05 is released.

[doronbehar]

I think this PR can probably wait until 23.05 is released.

Personally I won't mind, as I always use nixos-unstable. But I still think this is an annoying issue that may have bothered many users without them knowing about it, and they would appreciate to notice it is fixed in 23.05. I am optimistic that after your fixup PR and the release notes, few to zero users will complain about incompatibilities. Let's wait for responses on #232679 in the meantime.

In anycase, I'd be glad to reach some kind of agreement regarding the issues mentioned earlier - Bash / Nix mixing readability, and using more jq logic vs Bash logic.

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-in-distress/3604/62

[doronbehar]

Rebased this after #233386 . @Lassulus & @ncfavier your review is welcome 🙏. nixosTests.syncthing* tests are working.

[Lassulus]

I'm a bit unhappy that we are no longer atomic if we do multiple curl requests

[doronbehar]

I'm a bit unhappy that we are no longer atomic if we do multiple curl requests

Why is a single, atomic curl request better? If there's a single, minor problem with it, it completely fails, and it's hard to debug why. This is exactly what happened in #230146, and what took me hours to debug.

[doronbehar]

I improved the comments, and used lib.pipe to hopefully improve readability.

[Lassulus]

I'm a bit unhappy that we are no longer atomic if we do multiple curl requests

Why is a single, atomic curl request better? If there's a single, minor problem with it, it completely fails, and it's hard to debug why. This is exactly what happened in #230146, and what took me hours to debug.

Ah indeed. then we had no atomic behavior before also. So it's not a regression but an improvement

[doronbehar]

@ofborg test syncthing syncthing-init syncthing-no-settings

Thanks for the review @Lassulus. do you think we should have another eye approving this?

[doronbehar]

I just amended a bit the commit message, no real change besides that.

[Lassulus]

If you know someone who has the time and interest to review it would be cool, otherwise I would merge it in the next week