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/gitweb: add some (crucial) options #75602
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.
Works fine here, thanks! :)
LGTM. Could you squash the 3 commits together into 1? Also, it'd be nice if you could add a NixOS test, so we could ensure things keep working ;-) |
ping @vanyaklimenko |
This replaces some hardcoded values in nginx's VirtualHosts's configuration with customizable options. Previous values are kept as default, so nothing should break for existing users. Co-Authored-By: Florian Klink <flokli@flokli.de>
dbdb3a3
to
ed52a65
Compare
@vanyaklimenko NixOS tests are always nice for modules as at bare minimum it gives committers who have no domain specific knowledge of a module a better indication they aren't breaking anything by hitting 'merge'. That being said, this module doesn't currently have a test and as your PR mentions these are crucial options. If anyone has a chance to come back to this and create a NixOS test full bonus points will be awarded 🎉 Merging based on the code seeming reasonable to myself and @flokli as well as testing provided by @gnidorah. Thanks for your contribution! |
Motivation for this change
This replaces some hardcoded values in nginx's VirtualHosts's configuration with customizable options. Previous values are kept as default, so nothing should break for existing users.
Call it a minimal refactoring.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @gnidorah