-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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/nextcloud-notify_push: fix connecting to mysql via socket #348114
base: master
Are you sure you want to change the base?
Conversation
Thanks! I don't use mysql/mariadb so I must admit I myself never tested this. 😅 |
if dbType == "postgresql" then "?host=${cfg.dbhost}" else | ||
if dbType == "mysql" then "?socket=${cfg.dbhost}" else throw "unsupported dbtype" | ||
if isPostgresql then "" else | ||
if isMysql then "@localhost" else throw "unsupported dbtype" |
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 don't really want to throw inside the module system as that will not show all errors/warnings but that was already here before and we would need to refactor the database configuration a lot to bring that up to the right 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.
I fully agree. This whole nested if expression also looks a bit ugly.
2291322
to
b9ca849
Compare
Fix #310335
Hey folks, I'm a Nextcloud developer and am currently tinkering with NixOS. Here is a little fix I came up with.
Instructing Nextcloud to connect to MySQL via a socket requires setting dbhost to
localhost:/run/mysqld/mysqld.sock
while Postgresql requires just the path to the socket.Thus, the notify_push service wasn't able to connect to MySQL via a socket due to a malformed
DATABASE_URL
env variable. The host part is omitted if a socket is detected.I refactored and fixed the logic when MySQL is used to always include the host part and also add the socket option to the
DATABASE_URL
variable. Notify_push will now connects to the MySQL socket properly.I tested this locally with a fresh Postgres and MySQL database to make sure that there are no regressions.
This is my first PR here so please let me know in case something needs to be adjusted.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.