Skip to content
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

Merged
merged 1 commit into from Jan 16, 2020

Conversation

@vanyaklimenko
Copy link
Contributor

@vanyaklimenko vanyaklimenko commented Dec 13, 2019

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @gnidorah

Copy link
Contributor

@gnidorah gnidorah left a comment

Works fine here, thanks! :)

@flokli
Copy link
Contributor

@flokli flokli commented Dec 17, 2019

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 ;-)

@aanderse
Copy link
Contributor

@aanderse aanderse commented Jan 11, 2020

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>
@vanyaklimenko vanyaklimenko force-pushed the vanyaklimenko:nginx-gitweb-more-options branch from dbdb3a3 to ed52a65 Jan 14, 2020
@vanyaklimenko
Copy link
Contributor Author

@vanyaklimenko vanyaklimenko commented Jan 14, 2020

@flokli @aanderse

Sorry for the long wait. I've squashed the commits.

Not sure if the tests are really needed as the package seems pretty straightforward to me, and, alas, at the moment I just do not have enough time to spare on this. Let's leave it as it is for now?

@aanderse
Copy link
Contributor

@aanderse aanderse commented Jan 16, 2020

@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!

@aanderse aanderse merged commit fc1bee5 into NixOS:master Jan 16, 2020
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.