-
-
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/gitea: add nginx configuration #218020
base: master
Are you sure you want to change the base?
Conversation
Don't need access to |
locations."^~ /avatars/" = { | ||
alias = "${cfg.stateDir}/data/avatars/"; | ||
tryFiles = "$uri =404"; | ||
|
||
extraConfig = '' | ||
default_type "image"; | ||
'' + nginxCommonHeaders; | ||
}; | ||
|
||
locations."^~ ${cfg.settings.server.STATIC_URL_PREFIX}/assets/" = { | ||
alias = "${cfg.package.data}/public/"; | ||
tryFiles = "$uri =404"; |
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 thought about it a few days, and I really do not see that much value in letting nginx serve static files directly.
As mentioned in #218020 (comment), even big instances don't seem to enable this feature.
And in the context of nixpkgs, it just makes this config unnecessarily complex.
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.
If possible, I always prefer to serve static files directly through nginx. In this case the file will be transferred a little faster.
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.
/avatars/
(and /repo-avatars/
, for that matter) isn't static, though.
It dynamically resizes the image based on the ?size
uri parameter.
And can be overwritten via picture.AVATAR_UPLOAD_PATH
(picture.REPOSITORY_AVATAR_UPLOAD_PATH
), which would break "${cfg.stateDir}/data/avatars/"
.
Furthermore, those could be stored remotely using an entirely different storage driver using AVATAR_STORAGE_TYPE
(REPOSITORY_AVATAR_STORAGE_TYPE
).
I stand by what I said and do not understand why this nixos/gitea module needs to be made more complex than it needs to be.
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 can later try to test the operation with these parameters.
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.
/avatars/
(and/repo-avatars/
, for that matter) isn't static, though.
It dynamically resizes the image based on the?size
uri parameter.
Image processing is done by the browser, the gitea
service is not involved.
And can be overwritten via
picture.AVATAR_UPLOAD_PATH
(picture.REPOSITORY_AVATAR_UPLOAD_PATH
), which would break"${cfg.stateDir}/data/avatars/"
.Furthermore, those could be stored remotely using an entirely different storage driver using
AVATAR_STORAGE_TYPE
(REPOSITORY_AVATAR_STORAGE_TYPE
).
Fixed!
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.
If Gravatar is enabled, then /avatar/<email-hash>?size=96
redirects (http/303) to https://secure.gravatar.com/avatar/<email-hash>?d=identicon&s=96
You are, however, correct, that the local storage does not resize the images server-side (yet?1)
Your avatar-serving breaks if
- Gravatar isn't disabled (
DISABLE_GRAVATAR
), which as of v1.18 moved to the database, meaning you can no longerassert
it from the nixos config. OR OFFLINE_MODE
is enabled, which also disables Gravatar as side effect. ORAVATAR_STORAGE_TYPE
/REPOSITORY_AVATAR_STORAGE_TYPE
is to something custom/not local
Plus, the docs for STATIC_URL_PREFIX
explicitly state
[...] Avatar images are dynamic resources and still served by Gitea. [...]
Footnotes
-
Could change in the future, see https://github.com/go-gitea/gitea/issues/20918 ↩
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.
If gravatar is used, the link will have an address like this - https://www.gravatar.com/avatar/...
As a result, the rule location ^~ /avatars/ {
will not work for them.
Plus, the docs for
STATIC_URL_PREFIX
explicitly state
If avatar handling in gitea changes in the future, we can fix the nginx configuration.
Added small fix.
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.
oh lol sorry -- my bad, yes, you are right.
I somehow overlooked that additional s
in /avatars/
compared to /avatar/
😅
I would still prefer this to be dropped altogether.
But I also don't want to annoy you anymore regarding this.
Just keep it however you imagined it originally and let the other reviewers decide.
Sorry for the noise (and the whole back and forth)
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.
Your comments helped improve PR. I didn't take into account possible changes to the defaults when I created the PR.
9e825b3
to
907ff8f
Compare
Just two minor things. I guess it'd be nice to extend the VM test to check if e.g. avatars are served correctly, but other than that it looks good to me, thanks 👍 |
907ff8f
to
343df9d
Compare
Tested on a test virtual machine, no problems with avatars |
343df9d
to
dc916f9
Compare
dc916f9
to
2e30023
Compare
root = "/var/empty"; | ||
|
||
locations."/" = { | ||
proxyPass = (if cfg.enableUnixSocket then "http://unix:/run/gitea/gitea.sock" else "http://127.0.0.1:${toString cfg.httpPort}"); |
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.
proxyPass = (if cfg.enableUnixSocket then "http://unix:/run/gitea/gitea.sock" else "http://127.0.0.1:${toString cfg.httpPort}"); | |
proxyPass = if cfg.enableUnixSocket then "http://unix:/run/gitea/gitea.sock" else "http://127.0.0.1:${toString cfg.httpPort}"; |
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.
One could also use "http://unix:${cfg.settings.server.HTTP_ADDR}"
instead of
"http://unix:/run/gitea/gitea.sock"
.
And if we go down that route, it might also be worth setting cfg.httpAddress = "127.0.0.1"
(which in turn sets cfg.settings.server.HTTP_ADDR
) when cfg.configureNginx
is enabled to further deduplicate the addresses used and allow something like:
proxyPass = "http://" +
(lib.optionalString cfg.enableUnixSocket "unix:") +
cfg.settings.server.HTTP_ADDR +
(lib.optionalString (!cfg.enableUnixSocket) ":${toString cfg.httpPort}");
Or assert cfg.enableUnixSocket = true
when cfg.configureNginx = true
instead of just setting the default for it.
That would at least simplify the logic /shrug
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.
Update to:
proxyPass = if cfg.enableUnixSocket then "http://unix:${cfg.settings.server.HTTP_ADD}" else "http://127.0.0.1:${toString cfg.httpPort}";
And if we go down that route, it might also be worth setting
cfg.httpAddress = "127.0.0.1"
(which in turn setscfg.settings.server.HTTP_ADDR
) whencfg.configureNginx
is enabled to further deduplicate the addresses used and allow something like:
By default HTTP_ADDR = "0.0.0.0";
, not 127.0.0.1
.
Or assert
cfg.enableUnixSocket = true
whencfg.configureNginx = true
instead of just setting the default for it.
That would at least simplify the logic /shrug
Change to:
enableUnixSocket = mkOption {
type = types.bool;
default = cfg.configureNginx;
or:
enableUnixSocket = mkOption {
type = types.bool;
default = true;
?
b1d4414
to
3e6c62b
Compare
Remove |
@emilylange @Ma27 Do you think we can come to a conclusion on this change? Either, we decrease the scope into smaller PRs so that authors can at least dispatch some useful changes and then for the more controversial stuff, we can keep this PR. |
@RaitoBezarius see #218020 (comment). I guess I could have been more explicit, but back then I figured this wording alone with the context surrounding it is clear enough. My bad, let me give it another go: I completely withdrew my code ownership of gitea and no longer intent to contribute to gitea (in nixpkgs). Any remaining reviews, objections, whatever, of open or not-yet-merged PRs can and should be ignored. I maintain forgejo, not gitea -- and while forgejo initially did use almost everything from gitea in nixpkgs (drv, module, VM test), this is no longer the case. |
My apologies, I will not do this mistake again. |
e576aff
to
06f6404
Compare
Update PR. |
3f79474
to
8da45ee
Compare
8da45ee
to
1a26488
Compare
1a26488
to
7ef308a
Compare
4511526
to
52f37bb
Compare
52f37bb
to
9e3e8ae
Compare
Description of changes
Add nginx configuration.
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/
)