ansible / lightbulb

Lightbulb has been deprecated and replaced by Ansible Workshops
https://ansible.github.io/workshops/
MIT License
479 stars 307 forks source link

Apache and nginx basic and role examples should allow for the port to defined. #239

Closed bbaassssiiee closed 6 years ago

bbaassssiiee commented 6 years ago

This implements the changes for https://github.com/ansible/lightbulb/issues/92

Apache and Nginx basic and role examples should allow for the port to defined.

tima commented 6 years ago

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.

tima commented 6 years ago

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.

bbaassssiiee commented 6 years ago

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.

bbaassssiiee commented 6 years ago

Resolved conflicts (trailing whitespace)

tima commented 6 years ago

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."

bbaassssiiee commented 6 years ago

So this PR implements the changes for #92 after reviews it can be merged IMHO.

tima commented 6 years ago

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.

gundalow commented 6 years ago

Closing/Reopening to cause Travis to run.

tima commented 6 years ago

ping @bbaassssiiee -- this PR is waiting on your input.

tima commented 6 years ago

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.