immich: 1.117.0 -> 1.118.1 #348890
immich: 1.117.0 -> 1.118.1 #348890
Conversation
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-darwin: 1
10.rebuild-linux: 1-10
labels
| @@ -91,7 +91,7 @@ in | |||
| }; | |||
| port = mkOption { | |||
| type = types.port; | |||
| default = 3001; | |||
There was a problem hiding this comment.
why change the default port? Seems it could complicate peoples configs if they never set the port option. I would prefer if this option was kept at the current value.
There was a problem hiding this comment.
It's described in the second commit:
This was always upstream's default but they also change the internal port, i.e. behind the reverse proxy, to 2283 in immich-app/immich#13185.
This could be a breaking change though. Should this be behind a stateVersion flag?
There was a problem hiding this comment.
Should this be behind a stateVersion flag?
I thought about that but I don't think it's necessary since the module hasn't made it to a stable NixOS version yet.
There was a problem hiding this comment.
Should this be behind a stateVersion flag?
I thought about that but I don't think it's necessary since the module hasn't made it to a stable NixOS version yet.
People are still using the unstable branch though. In the end the port number doesn't really matter.
There was a problem hiding this comment.
This could be a breaking change though. Should this be behind a stateVersion flag?
It's too heavy for something not released yet and immich clearly advertises they wont let backward-compatibility in their way.
This worked for me after updating the reverse proxy. Phone app and CLI worked as well.
There was a problem hiding this comment.
After some thinking I agree with @dotlambda on this topic.
There was a problem hiding this comment.
My config is
services.nginx = {
enable = true;
virtualHosts."immich.example.com" = {
enableACME = true;
forceSSL = true;
locations."/" = {
proxyPass = "http://${config.services.immich.host}:${toString config.services.immich.port}";
proxyWebsockets = true;
};
extraConfig = ''
client_max_body_size 512M;
'';
};
};so I don't even need to change anything.
There was a problem hiding this comment.
I agree that a stateVersion change would likely be too heavy. But I do wonder what the benefit of changing the port at all is. The upstream port choice really has no bearing on what we use in NixOS so I would have just left what was originally assigned.
That being said I also used config.services.immich.port in my config so it won't be a bother for me. But in general I would avoid changing things that don't need changing.
Either way too late now. But may be useful for future changes of similar nature.
|
@ofborg build immich immich.passthru.tests |
|
@dotlambda I would be interested in maintaining this package and module long-term. Would you want to add me (and also yourself if you are interested too) as maintainers for both? |
|
@dotlambda could I be added as a maintainer as well? I would love to help maintain this package :) |
|
Why did people approve even though |
This was always upstream's default but they also change the internal port, i.e. behind the reverse proxy, to 2283 in immich-app/immich#13185.
haha it never built for me with cuda, so I've it disabled. |
|
@dotlambda try rebasing on latest master commit as the onnx package was fixed. I approved because the onnx changes would've fixed it i believe :p |
No, pydantic and pydantic-settings had to be updated. |
Borg was green even after you ran But I can now indeed confirm that pydantic was updated ^^ |
|
Result of 1 package blacklisted:
4 packages built:
|
|
I'll test it in production! |
|
Runs on my server! |
https://github.com/immich-app/immich/releases/tag/v1.118.0
https://github.com/immich-app/immich/releases/tag/v1.118.1
fixes #348829
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.