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

Make nginx support multiple servers entries #868

Closed
wants to merge 4 commits into from
Closed

Make nginx support multiple servers entries #868

wants to merge 4 commits into from

Conversation

@ccesario ccesario changed the title Make nginx support multiuple servers entries Make nginx support multiple servers entries Sep 25, 2018
@fabianfrz fabianfrz self-requested a review September 25, 2018 04:36
@fabianfrz fabianfrz self-assigned this Sep 25, 2018
@fabianfrz fabianfrz added the bug Production bug label Sep 25, 2018
@fabianfrz
Copy link
Member

There is also a file for the web interface which is currently not included which includes this listen directives as well.

@ccesario
Copy link
Contributor Author

Hi @fabianfrz , if the file is wegbui.conf, it seems the listen directive is OK. There are no problem with it.

Copy link
Member

@fabianfrz fabianfrz left a comment

Choose a reason for hiding this comment

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

needs some adjustments

@@ -87,7 +87,7 @@ location {{ location.matchtype }} {{ location.urlpattern }} {
{% if location.upstream is defined and (location.php_enable is not defined or location.php_enable != '1') %}
{% set upstream = helpers.getUUID(location.upstream) %}
proxy_set_header Host $host;
proxy_pass http{% if upstream.tls_enable == '1' %}s{% endif %}://upstream{{ location.upstream.replace('-','') }};
proxy_pass http{% if upstream.tls_enable == '1' %}s{% endif %}://upstream{{ location.upstream.replace('-','') }}{% if location.new_urlpattern != '' %}{{ location.new_urlpattern }};{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

this will throw a NPE because it may not be set. Guard with an is defined check first

@@ -205,6 +205,9 @@
<Required>N</Required>
<multiple>N</multiple>
</upstream>
<new_urlpattern type="TextField">
<Required>N</Required>
</new_urlpattern>
Copy link
Member

Choose a reason for hiding this comment

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

the name suggest something different - how about path_prefix?

<id>location.new_urlpattern</id>
<label>New location path</label>
<type>text</type>
<help>Select a new path for upstream to proxy.</help>
Copy link
Member

Choose a reason for hiding this comment

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

this is a textbox, you cannot select. Enter or Define would be a lot better. Also explain what is doing.

@@ -205,6 +205,9 @@
<Required>N</Required>
<multiple>N</multiple>
</upstream>
<new_urlpattern type="TextField">
<Required>N</Required>
Copy link
Member

Choose a reason for hiding this comment

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

needs a mask regex pattern - usually no whitespace allowed which is \S+, maybe you also want to limit the length to something useful by using {,100} for a maximum length of 100 instead of +.

@@ -61,6 +61,12 @@
<style>selectpicker</style>
<help>Select an upstream to proxy to or connect via FastCGI if chosen.</help>
</field>
<field>
<id>location.new_urlpattern</id>
<label>New location path</label>
Copy link
Member

Choose a reason for hiding this comment

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

see other comments

@ccesario
Copy link
Contributor Author

@fabianfrz thanks by your comments, I will split it into two pull requests and make the changes as you suggest

@ccesario ccesario closed this Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Production bug
Development

Successfully merging this pull request may close these issues.

2 participants