-
Notifications
You must be signed in to change notification settings - Fork 5
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
Bug fixes: Find mime.types file location and automatically find Openresty path #90
Bug fixes: Find mime.types file location and automatically find Openresty path #90
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, @davidjwbbc .
In addition, would it be possible to print out some informative lines to the console to confirm which paths have been detected for the binary and modules? This is then valuable feedback for the user to understand what's going on when there are multiple instances installed at different paths.
Added log output to log INFO messages about the path and mime.types file location. For example:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extra informative lines.
Hi David, I've reinstalled from your branch and I keep getting the lua_package_path error:
|
I believe it is useful that when doing a clean installation I make sure Openresty is installed... IGNORE the above. It works, now trying to disable nginx:
|
@davidjwbbc, after testing, I think we are good to go. My recollection on the instructions will be that the export PATH command is no longer needed and that |
I would suggest to move the installation of Openresty upwards and make it a more explicit step. I would integrate the paragraph
within "Install dependencies" rather than under running (you will try to execute 5gms-application-server but the paragraph telling you to install openresty just comes next). |
Ok, I've improved the OpenResty executable finding by verifying the nginx executable found was compiled to handle LUA. If no suitable executable is found then the following error is displayed:
"openresty" being the only web proxy module currently implemented (renamed from "nginx" to avoid confusion). If more web proxy modules are added and none find their executables are found then they are all listed here. If the openresty web proxy module found a suitable nginx executable then it is reported:
Note: I'm no longer adding the known openresty path to the PATH environment variable, instead the openresty web proxy module provides the known openresty path as an extra path to try to the function which finds executables. This allows the AS to be run with an alternative OpenResty in the PATH to use that one instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't spot an error message when no valid OpenResty installation can be found.
Could you point it out please?
@davidjwbbc same as Richard. I cannot find the line in the app.py file of the PR. I see the correction if I go directly to your branch though. Somehow the PR is not up to date? |
sorry, I see it now. Line 261 but somehow the change is not displayed by GitHub under "Files changed". I think this is good to go. |
This PR includes some minor bug fixes for #83 and #89.
Find mime.types file (#83):
mime.types
file and then for the default nginxmime.types
file and use the foundmime.types
in the nginx configuration./usr/local/openresty/nginx/conf/mime.types
and/etc/nginx/mime.types
./usr/share/nginx/modules
being present (Openresty automatically includes its own modules so doesn't need this extra include in the nginx configuration).Automatically find Openresty path (#89):
nginx/sbin
directories and if found prepends it to thePATH
environment variable for the AS environment./usr/local/openresty/nginx/sbin
.Fixes #83
Fixes #89