-
-
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/nginx: add locations."name".uwsgiPass option and use it #346776
base: master
Are you sure you want to change the base?
Conversation
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.
So I haven't checked, but I assume you can't have both proxyPass
and uwsgiPass
in the same location?
If so, we'll probably want an assertion checking for this?
Probably, I added one. |
I think you pushed that to the wrong PR. |
45dad5a
to
be26ca8
Compare
Pushed the assertion to this PR and added the if blocks for proxyResolveWhileRunning. |
be26ca8
to
be91974
Compare
@ofborg eval |
I'm a little surprised the |
${optionalString (config.uwsgiPass != null && !cfg.proxyResolveWhileRunning) | ||
"uwsgi_pass ${config.uwsgiPass};" | ||
} | ||
${optionalString (config.uwsgiPass != null && cfg.proxyResolveWhileRunning) '' |
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 a little bit torn on this.
On one hand I think it's surprising that the proxy*
options have an effect on uwsgi. OTOH it seems tedious to add counterparts for the uwsgi case.
I'm slightly leaning towards adding uwsgi counterparts for everything we need here. What do others think of this?
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've added the ones that existed. uwsgi has no http_version or set_header commands.
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 also leaning towards adding uwsgi counterparts because this behavior is not intuitive. If we keep the proposed behavior in this PR we IMHO should at least mention in the proxy-related options documentation that they also apply to uwsgi.
As far as I can see this only applies to:
recommendedProxySettings
proxyResolveWhileRunning
proxyTimeout
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 done that :)
be91974
to
7109748
Compare
7109748
to
6443f16
Compare
I am going to use that in another PR for searxng #346777 to reduce duplication for uwsgi.
This also allows to easily overwrite a uwsgi_pass target with an upstreams block.
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.