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

xandikos: backport systemd socket activation #161690

Closed
wants to merge 2 commits into from

Conversation

schnusch
Copy link
Contributor

@schnusch schnusch commented Feb 24, 2022

Motivation for this change

Backported jelmer/xandikos#155 to add support for systemd socket activation. This also enables UNIX sockets with proper owner/permissions, because systemd creates them for us.

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/)
  • 22.05 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@0x4A6F
Copy link
Member

0x4A6F commented Feb 28, 2022

I'm a little bit baffled about your PR description and the actual changes.

@schnusch
Copy link
Contributor Author

schnusch commented Mar 1, 2022

Yeah, you're right. Xandikos was always able to do that, this PR just adds that to the nginx configuration.

@0x4A6F
Copy link
Member

0x4A6F commented Mar 2, 2022

Topic and changes fit together.

But I mean the mismatching description:

Remove prometheus-client, it is replaced by aiohttp-openmetrics which is not
yet packaged.

@schnusch
Copy link
Contributor Author

schnusch commented Mar 2, 2022

Yeah, I think you are right, when I made this I still had xandikos-0.2.6 and was about to create a pull request for that too but noticed this and probably mixed up the pull requests.

Copy link
Member

@0x4A6F 0x4A6F left a comment

Choose a reason for hiding this comment

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

LGTM, could you also expand nixos/tests/xandikos.nix to include a subtest for sockets usecase?

@schnusch
Copy link
Contributor Author

schnusch commented Mar 2, 2022

While writing a test I noticed that the permission of the socket are too strict to be useful. Changing the services UMask does not seem appropriate. We should probably put this off until systemd's socket activation makes it into xandikos, see jelmer/xandikos#155.

@schnusch schnusch marked this pull request as draft April 14, 2022 15:15
@schnusch schnusch marked this pull request as ready for review May 1, 2022 11:46
@schnusch schnusch requested a review from 0x4A6F May 1, 2022 11:46
@schnusch schnusch changed the title nixos/xandikos: support UNIX sockets xandikos: backport systemd socket activation May 1, 2022
@schnusch schnusch force-pushed the xandikos branch 2 times, most recently from 38f15b8 to bce1a19 Compare May 5, 2022 19:10
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 2, 2022
@schnusch
Copy link
Contributor Author

Rebased to fix merge conflict.

@schnusch
Copy link
Contributor Author

schnusch commented Sep 5, 2023

Closed in favor of #253523 #253634.

@schnusch schnusch closed this Sep 5, 2023
@schnusch schnusch deleted the xandikos branch September 6, 2023 10:42
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.

2 participants