www/nginx: plugins, bug fixes #1198
Conversation
| @@ -5,7 +5,7 @@ | |||
| <items> | |||
| <general> | |||
| <enabled type="BooleanField"> | |||
| <default>0</default> | |||
| <default>1</default> | |||
There was a problem hiding this comment.
why not? It there to configure if it should serve configured services.
There was a problem hiding this comment.
I don’t understand. Is this global context or not? There’s no rationale in the description to make deviation from our defaults reasonable.
There was a problem hiding this comment.
Yes, it is global but it only affects configured things. For example, if a plugin adds a custom vhost, this one is still served.
There was a problem hiding this comment.
Hmm, sorry, clicked resolved by accident.
This suggest the template does things in a way that makes this more convenient, but people will only take advantage of this on new plugin installs and I don’t like the idea of opt-out even though nginx must be installed willingly.
I’d rather see some other solution to the problem that is quicker to deploy and more in line with what we have elsewhere. Or dropped if that is a minor issue that is just as well documented to be the way it works.
There was a problem hiding this comment.
@fichtner marked it as unresolved.
Currently the flag is silently ignored - 1.8 will enforce it so for new installation it will keep the behaviour. With this patch it allows the user to disable all his web pages and streams (the services itself is always enabled like haproxy because nginx is meant to become infrastructure for other plugins which need one and it will not bind anything if nothing is configured).
There was a problem hiding this comment.
@fichtner @fabianfrz I don't like the idea of opt-out either, if this is a migration issue, it's probably better to solve it there. Before attaching a lot of new plugins upon this one, it might be a good idea to discuss plans first. I'm a bit worried about a whole new dependancy chain around some plugins.
There was a problem hiding this comment.
Should I revert this line now?
| @@ -97,6 +101,16 @@ server { | |||
| set_real_ip_from {{ trusted_proxy }}; | |||
| {% endfor %} | |||
| {% endif %} | |||
| {% if server.trusted_proxies_alias is defined and server.trusted_proxies_alias != '' %} | |||
| {% for trusted_proxy_uuid in server.trusted_proxies_alias.split(',') %} | |||
There was a problem hiding this comment.
@fichtner this way we can switch it to a select multiple if needed
|
@fichtner fixed the typos - the enabled flag is also reverted since your last review. |
see release note
partial of: https://forum.opnsense.org/index.php?topic=8877.msg52677#msg52677