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/paperless: json-encoded variables viable in unit and manage script #243084

Closed
wants to merge 1 commit into from

Conversation

ctheune
Copy link
Contributor

@ctheune ctheune commented Jul 12, 2023

Some paperless settings (like PAPERLESS_CONSUMER_IGNORES) are taken as JSON-encoded strings. Using regular double-quotes and then using -quoted double quotes within the JSON string works for the systemd unit but not for systemd.

With this combination we can actually use toJSON to write those values without quoting hassle as a regular nix expression.

Includes a test that verifies we're getting this right for both ends.

Thanks: erikarvsted for even better quoting.

Description of changes

Improve quoting and add a test.

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:
  • 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.

Some paperless options (like PAPERLESS_CONSUMER_IGNORES) are taken
as JSON-encoded strings. Using regular double-quotes and then using
\-quoted double quotes within the JSON string works for the systemd
unit but not for the manage command.

With this combination we can actually use `toJSON` to write those
values without quoting hassle as a regular nix expression.

Includes a test that verifies we're getting this right for both ends.

Thanks: erikarvsted for even better quoting.
@ctheune ctheune requested review from erikarvstedt and leona-ya July 12, 2023 14:51
@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 Jul 12, 2023
@ctheune
Copy link
Contributor Author

ctheune commented Jul 12, 2023

This is a spin-off from #242084

@erikarvstedt
Copy link
Member

Thanks, this is a duplicate of #242275, so I guess we can close this.

@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 Jul 12, 2023
@ctheune
Copy link
Contributor Author

ctheune commented Jul 12, 2023

Ouch. Interesting. I'll have to figure out how to do contributions properly. I usually have limited time frames to work on personal stuff and obviously I don't want to waste time duplicating effort. I decided to work on the issues in the original PR from top down instead of filling the whole slot I had with reading it and then not doing anything and having to come back again days later. Seems like that can quickly become a waste of time (not just for me).

Maybe extending the tests in the other PR is a valuable addition?

@erikarvstedt
Copy link
Member

Maybe extending the tests in the other PR is a valuable addition?

toShellVars is already tested here, I don't think we should add this to the paperless test.
Feel free to reopen.

@ctheune
Copy link
Contributor Author

ctheune commented Jul 12, 2023

Hmm. I'm not sure I understand. There was a regression in the paperless module that didn't depend on whether toShellVars is broken but the actual integration in paperless was. Shouldn't that be tested now that we caught it in a broken state?

@erikarvstedt
Copy link
Member

Not a regression, this was once the intended behavior that got broken for the service env when the module was updated to paperless-ngx.
The code now makes it clear that var contents should be exported verbatim. A regression from this is unlikely.
Yes, in principle we could test this, but I think it's too trivial. (Weak preference.)

@ctheune
Copy link
Contributor Author

ctheune commented Jul 13, 2023

Thanks for clarifying. Coming from a "100% test coverage" and "every regression gets a test" background I'm more on the cautious side. Something I can sympathize with is the amount of tests we run in total and reducing test coverage to a "sane minimum" ...

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants