-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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/mattermost: Clean up and modernize #208181
base: master
Are you sure you want to change the base?
Conversation
6b42e85
to
0228f5c
Compare
See the tests for an example of how to add and enable a plugin. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/1619 |
To have a truly immutable plugins install & enable functionality we will need to patch Mattermost's source code. But for now this is fine I guess... |
(mkRemovedOptionModule [ "services" "mattermost" "matterircd" "enable" ] "This option has been removed as it shouldn't be part of the Mattermost module.") | ||
(mkRemovedOptionModule [ "services" "mattermost" "matterircd" "package" ] "This option has been removed as it shouldn't be part of the Mattermost module.") | ||
(mkRemovedOptionModule [ "services" "mattermost" "matterircd" "parameters" ] "This option has been removed as it shouldn't be part of the Mattermost 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.
We should move those to its own module instead.
Haven't forgotten about this. I think the strategy will be to use the current Mattermost module as a reference to re-add the missing features. |
0228f5c
to
fac011b
Compare
Based on NixOS#198040. Prioritizes backwards compatibility, including database and plugin compatibility, while adding more sensible defaults like database peer auth.
fac011b
to
52fc542
Compare
userPart = | ||
if user == null && password == null then | ||
"" | ||
else if user != null && password != null then | ||
escapeURL user + ":" + escapeURL password | ||
else | ||
escapeURL (if password != null then password else user); |
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'm not sure this is correct. This logic could produce password@host
, but that would be interpreted as a username. Is it even valid to have a password and no username?
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.
Technically a username (without a password) is valid. I'm interpreting a password without a username as a sort of API key usecase, though this is more for URIs in general even though not necessarily Postgres ones.
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.
In a URL with apikey@host
, apikey
is parsed as a username, even if its function is closer to a password. If someone wants to connect to their postgres with an API key, they probably know enough about URLs to know they should use the username field.
users.users = optionalAttrs (cfg.user == "mattermost") { | ||
mattermost = { | ||
group = cfg.group; | ||
uid = config.ids.uids.mattermost; | ||
home = cfg.statePath; | ||
}; | ||
users.users.${cfg.user} = { | ||
group = cfg.group; | ||
uid = config.ids.uids.mattermost; | ||
home = cfg.dataDir; |
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 is a bit cleaner, but if someone did reconfigure cfg.user
from the default, this isn't functionally identical. Do we just not expect anyone to have used cfg.users
— in which case, why is it even an option — or is this more correct than the previous behavior?
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.
Thanks, I think there was a reason this was added from the blame and I'll change it back.
Edit: I'm actually not sure. It's in this diff:
7c6d253#diff-3517cc8e7468615ccea52f1b8fb62377b14733d2e0a08ae5c7d390650402b57eR149
@fpletz do you think we need this still?
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 figured it out. The secret is to use uid = mkIf (cfg.user == "mattermost") config.ids.uids.mattermost;
since then we auto-assign a UID if and only if the username is nonstandard. Same with gid. Without this, my tests with a nonstandard username failed because there was no user created.
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 don't know how many NixOS Mattermost deployments there are or how many of them are changing the user, but suppose someone was running it as their own personal user. With the previous behavior, no user configuration is done. With the current behavior, this would cause a conflict on group
and home
requiring mkForce
or similar.
Maybe there needs to be a release notes item to the effect of "if you run mattermost with a nonstandard user, you should watch out/review the changes"? I still have no idea whether we're breaking anyone with this or just fixing a potential bug.
Description of changes
Taking ideas from #198040, support more complex database configurations and better defaults like peer authentication.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes