Apache and nginx basic and role examples should allow for the port to defined. #239
Conversation
bbaassssiiee
changed the title
|
Thanks for your contribution @bbaassssiiee. Before we could merge this, some of the workshops and their solutions docs need to be modified to be consistent with the examples being modified here.
|
|
Thanks for this @bbaassssiiee. Looks good at a glance. I'm going to try put this thru its paces with a few people just to verify since this is part of the mainline ongoing Red Hat workshops. |
|
For beginner training you might consider the value of having a variable 'webserver_port' vs. 'apache_webserver_port' and 'nginx_webserver_port'. Best practices call for a prefix as currently used. |
|
Resolved conflicts (trailing whitespace) |
|
Thanks for your suggestion. The potential for a namespace collision is more likely with a name being as generic as 'webserver_port". I've seen many a play with apache and nginx or tomcat running on them. I'm of the mind that we should set a good example that gets students/users into habits that avoid such eventualities. When I speak about Ansible best practices, I not only encourage them to use descriptive, unique human-meaningful variable names (one can argue webserver_port does or doesn't there) but to also prefix variables with it’s “owner” such as a role name or package. I think something named "apache" or "nginx" are more common than naming a role "webserver." |
|
So this PR implements the changes for #92 after reviews it can be merged IMHO. |
|
Got it @bbaassssiiee. I'll try to find some time to test this out myself. I've pinged some of the contributors to give this a go also. Thanks for your cooperation and help here. |
|
Closing/Reopening to cause Travis to run. |
There was a problem hiding this comment.
I did a test run of all the examples this touched a found a few small bugs. I've left comments as close as could. Github code review only allows comments on lines that changed. I came across a couple of other minor issues that I will open separate tickets in the interest of getting this one merged as soon as you can fix address the issues I found here.
| # The following lines prevent .htaccess and .htpasswd files from being | ||
| # viewed by Web clients. | ||
| # The following lines prevent .htaccess and .htpasswd files from being | ||
| # viewed by Web clients. |
There was a problem hiding this comment.
Below this around line 183 (Github won't let me leave a comment on a line you didn't change) there is a MaxKeepAliveRequests {{ apache_max_keep_alive_requests }} in this template that wasn't removed and is giving me an error in my tests.
| @@ -10,7 +10,7 @@ | |||
| - python-devel | |||
| - gcc | |||
| nginx_test_message: This is a test message | |||
| nginx_keepalive_timeout: 115 | |||
| nginx_webserver_port: 80 | |||
There was a problem hiding this comment.
This is fine but the smoke test task below starting on line 48 fails if you change this default value during your run. You need url: http://localhost:{{ nginx_webserver_port }}/ there.
| @@ -1,4 +1,4 @@ | |||
| --- | |||
| # defaults file for nginx | |||
| nginx_test_message: This is a test message | |||
| nginx_keepalive_timeout: 115 | |||
| nginx_webserver_port: 80 | |||
There was a problem hiding this comment.
Github code review doesn't allow you to leave comments on any line that was not touched (missed) in a change. This example fails if you change this default port value during your run. The smoke test task needs url: http://localhost:{{ nginx_webserver_port }}/ in the task/main.yml file there.
|
ping @bbaassssiiee -- this PR is waiting on your input. |
|
Thanks for doing this @bbaassssiiee. I found a few issues I suspect where introduced in #266 that you merged in. I'm going to merge this PR and fix that immediately. |
This implements the changes for #92
Apache and Nginx basic and role examples should allow for the port to defined.