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/cgit: init #167319

Merged
merged 2 commits into from
Mar 13, 2023
Merged

nixos/cgit: init #167319

merged 2 commits into from
Mar 13, 2023

Conversation

schnusch
Copy link
Contributor

@schnusch schnusch commented Apr 5, 2022

Description of changes

Add cgit behind nginx using fcgiwrap.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@Lassulus
Copy link
Member

How is this different from #110270 ?

@Ma27
Copy link
Member

Ma27 commented Apr 21, 2022

Also, what's wrong with services.cgit?

@schnusch
Copy link
Contributor Author

I put it at services.nginx.cgit to make it consistent with services.nginx.gitweb or services.lighttpd.cgit.

@Ma27
Copy link
Member

Ma27 commented Apr 22, 2022

OTOH we have a lot of other services that are not part of services.{nginx,httpd}, but configure vhosts for these reverse proxies. IMHO the services.nginx namespace should only be used for options that are specific to the nginx service and not for other services that happen to use nginx.

@schnusch
Copy link
Contributor Author

schnusch commented Jun 5, 2022

I added support for a location prefix. This should now be a -- I think -- cleaner version of #110270, with the order of options in the configuration file, fastcgiParams, and git clone support.

I moved the options to services.nginx.virtualHost.….cgit, this allows one to run cgit on multiple vhosts.

@Ma27
Copy link
Member

Ma27 commented Jun 6, 2022

I'd like to repeat that it's IMO a bad idea to put options for a service under the nginx namespace.

@schnusch
Copy link
Contributor Author

schnusch commented Jun 6, 2022

I understand, but if multiple parallel instances of cgit are desired, I think, it's the best place to enable them.

@Ma27
Copy link
Member

Ma27 commented Jun 6, 2022

why is it necessary to (ab)use the nginx namespace for that purpose though? this can also be implemented under e.g. the namespace services.cgit.

@schnusch
Copy link
Contributor Author

schnusch commented Jun 6, 2022

I moved it all to services.cgit.*. services.cgit is a list now, which seems uncommon in nixpkgs, but allows us to configure multiple instances of cgit.

@schnusch schnusch changed the title nixos/nginx/cgit: init nixos/cgit: init Jun 6, 2022
{
options = {
services.cgit = mkOption {
type = types.listOf (types.submodule ({ config, ... }: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use types.attrsOf? with they key being the default value for nginx.virtualHost?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way currently gives the ability to run multiple cgit instances on the same vhost.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could do the same with attrsOf.

services.cgit."git.example.com" = { ... };
services.cgit."git.example.com2" = { nginx.virtualHost = "git.example.com"; nginx.location = "/git"; }

also I'm not sure if having multiple cgits on one instance is a common usecase.
attrs are generally preferred because they can be referenced again at some other point or can be overridden which is not possible with lists

nixos/tests/cgit.nix Outdated Show resolved Hide resolved
Copy link
Member

@Lassulus Lassulus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, guess I'm happy with it, do you want to squash the changes? or should I do it when merging?

nixos/modules/services/networking/cgit.nix Outdated Show resolved Hide resolved
Copy link
Member

@yrd yrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schnusch Thank you!

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase that we in the end only have the init commit left or should we squash merge?

nixos/modules/services/networking/cgit.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/cgit.nix Outdated Show resolved Hide resolved
@softinio
Copy link
Member

As this is approved, any chance it can be merged? Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants