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/dokuwiki: add caddy webserver support #132551

Merged
merged 1 commit into from Sep 18, 2021

Conversation

onny
Copy link
Contributor

@onny onny commented Aug 3, 2021

Motivation for this change

To be able to combine DokuWiki with other modules that are using Caddy. This concept is unfluenced by this pull request.

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 nixpkgs-review --run "nixpkgs-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.

@onny
Copy link
Contributor Author

onny commented Aug 3, 2021

@1000101: It seems all tests have passed now, do you think this is good now?

@1000101
Copy link
Member

1000101 commented Aug 3, 2021

@1000101: It seems all tests have passed now, do you think this is good now?

Good job! I'm going to be honest as I don't want this PR to become stale - I'm currently in a rush and I'll have no access to my PC for next few weeks, so if anyone feels like testing/reviewing this, I'd be more than grateful. Otherwise, it'll take me some time to test this properly.

@onny onny requested a review from nlewo August 4, 2021 14:02
@happysalada
Copy link
Contributor

Sorry to be adding more work for you.
Caddy has now support for a virtualHosts attribute in order to not override other configs.
When you have a moment, could you use that attribute ? (it would enable people to use caddy for other things)

@onny
Copy link
Contributor Author

onny commented Sep 17, 2021

Sorry to be adding more work for you.
Caddy has now support for a virtualHosts attribute in order to not override other configs.
When you have a moment, could you use that attribute ? (it would enable people to use caddy for other things)

Thanks for the info and thank you for this important work! I rebased and updated my PR. It now includes:

  • Support for Nginx and Caddy as webserver
  • Updated test script
  • Updated release notes

Works fine for me now :)

@happysalada
Copy link
Contributor

The diff looks fine.
Was there some formatting changes included in there ? Perhaps you have an automated formatter ? Or were the formatting changes just the result of the refactoring. That one commit is a little intense.
Just want to confirm.

@onny
Copy link
Contributor Author

onny commented Sep 17, 2021

The diff looks fine.
Was there some formatting changes included in there ? Perhaps you have an automated formatter ? Or were the formatting changes just the result of the refactoring. That one commit is a little intense.
Just want to confirm.

I used the concept and module template from the Wordpress PR here #84446
It already supported multiple web servers, so I adapted it

@happysalada
Copy link
Contributor

The diff is harder to review properly, but it looks good.

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.

None yet

3 participants