docker icon indicating copy to clipboard operation
docker copied to clipboard

Remove the example NGINX confs

Open aentwist opened this issue 1 year ago • 4 comments

The example nginx confs are outdated (w.r.t. .htaccess on nextcloud/server) - I suggest we remove the nginx confs from here since they are a maintenance problem and instead link to the example nginx conf(s) in the nextcloud documentation and say, go get it from there. The examples here led me somewhat astray while I was porting it to Caddy.

Although there is no guarantee that the docs are up to date (they probably aren't 100%), it is more likely they will be as that is where more people go (Docker and metal users), and it leaves one less place to maintain it. Then we can have a single source of truth.


Example of out-of-sync - Cache-Control is not being set on quite a lot of important things

https://github.com/nextcloud/docker/blob/289f0bb8a3f1bd24d1633bbd01798c5476368827/.examples/docker-compose/with-nginx-proxy/postgres/fpm/web/nginx.conf#L152

on nextcloud/server .htaccess

Example of how the confs might not even be completely correct - NGINX regexps do not need forward slash to be escaped. Note that they are also this way in the docs - so if it is incorrect all of the sudden there are now 2 places it has to be fixed.

https://github.com/nextcloud/docker/blob/289f0bb8a3f1bd24d1633bbd01798c5476368827/.examples/docker-compose/with-nginx-proxy/postgres/fpm/web/nginx.conf#L132

aentwist avatar Mar 22 '23 04:03 aentwist

Related to https://github.com/nextcloud/docker/issues/928

aentwist avatar Mar 22 '23 04:03 aentwist

In the spirit of ain't nobody got time for this, I heavily suggest removing them unless good counter-motivation, for keeping them, is provided

aentwist avatar Mar 22 '23 04:03 aentwist

I agree with the general idea and the goal, but the issue is that those aren't just standalone example config files for nginx - they're dependencies for the docker-compose build example in the parent folder(s)[1]. So any consideration to removing the nginx stuff will require a solution to that.

[1] e.g.

  • https://github.com/nextcloud/docker/blob/289f0bb8a3f1bd24d1633bbd01798c5476368827/.examples/docker-compose/with-nginx-proxy/postgres/fpm/docker-compose.yml
  • https://github.com/nextcloud/docker/blob/289f0bb8a3f1bd24d1633bbd01798c5476368827/.examples/docker-compose/with-nginx-proxy/postgres/fpm/web/Dockerfile

joshtrichards avatar Mar 22 '23 16:03 joshtrichards

I made this suggestion with full awareness of that 🙂 It is certainly the main consideration. It might not be worth much, but in my opinion the additional steps users would need to take in order to run examples are outweighed by the maintenance benefits.

and say, go get it from there

This does require more of users. Would involve adding documentation to the examples on the additional step to get them running.

The main problem with this is it runs the risk of the linked NGINX conf not working / becoming out-of-sync. Which would be surprising, but is certainly an issue.


Honestly I'd really like to wget from a permalink ~but of course that isn't trivial since the confs from the docs are wrapped into that page? Would be really nice if there was a GitHub project for them since they are out-of-scope of nextcloud/server, where we could get raw files... e.g. nextcloud/configs?~ Maybe nginx-root.conf.sample! So then something like the raw link.

Additionally with the wget idea, we

  1. probably avoid out-of-sync
  2. do not add additional steps for users

Might still want to document what is going on, so they can clearly follow things to the conf sources.

If my other pull requests are resolved I'll pick this up.

aentwist avatar Mar 25 '23 21:03 aentwist