Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Second attempt at adding PeerTube package #106492
Second attempt at adding PeerTube package #106492
Changes from all commits
616453e
eb94a32
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we used
DynamicUser
instead (combined with systemd'sStateDirectory
)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't know what DynamicUser is in this case, how would that work? One thing that I know I have to do is use the tmpfiles.rules to set the datadirs of the project as being owned by the peertube user. (The actual datadir is only set in the config file, not using the module.) So I think it makes sense to have a username here that one could access using a variable in order to set certain permissions. (As long as we don't auto-generate the entire PeerTube config file from within the module. But it's really complete, I would not advise that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DynamicUser can be used to dynamically allocate an ephemeral user/group when the service starts. That is usually possible if the users data does not need to be accessed by other users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case it would be hard, because the user needs to externally set the permission of some directories to be owned accessible by that user. I don't intend to do the full PeerTube configuration in the nixos module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why systemd provides
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But those directories are only used in the PeerTube's internal config file, NixOS never knows about them at the module level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would need to be fixed I think. Does peertube not support unix socket authentication for postgresql as I think this is the approach other services take?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, it might do, but I didn't work that out yet. Yeah that's why I suggested maybe just getting the
pkg
in might be easier, since modules are always a bit more opinionated..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I will look into this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenroose see stevenroose#2. Feel free to squash the commits after merging. You can keep me as co-author if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an proposal but this make it work for me out of the box with self-signed certificates added to my local trust store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, where does that magic string
"/etc/ssl/certs/ca-certificates.crt"
come from? Could we perhaps add this conditional on some nixos variable? Or is"/etc/ssl/certs/ca-certificates.crt"
the standard location always for certs in NixOS?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/security/ca.nix#L85 it's called the canonical location for NixOS so should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also many other packages/modules reference it or the old
/etc/ssl/certs/ca-bundle.crt
one.