jdauphant / ansible-role-nginx

Ansible role to install and manage nginx configuration
655 stars 302 forks source link

Add `nginx_http_extra_params` variable in default vars #149

Closed sysarcher closed 7 years ago

sysarcher commented 7 years ago

Hello Everyone, I would like to propose an additional variable in the defaults/main.yml file called nginx_http_extra_params.

Purpose

If someone wants to retain the defaults in jdauphant.nginx/defaults/main.yml and bring in additional variables to http_params, he should be able to use the nginx_http_extra_params variable. This would append to http_params retaining the defaults.

This would also help us in maintaining a common set of group_vars for nginx config and have additional params when needed for our deploys.

Example

# retain defaults and add additional `client_max_body_size` param
- role: jdauphant.nginx
      nginx_http_extra_params:
       - client_max_body_size 200M
jdauphant commented 7 years ago

Hello @shrmrf, Thanks for the proposition. What do you think by somethings like that:

By doing that people can use the power of ansible to combine or not parameters by doing something like that: nginx_http_params: {{ nginx_http_params_defaults + my_extra_params }}

sysarcher commented 7 years ago

Yeah, makes sense. This is what I proposed too but instead of concatenating the two lists to create the parameters, I just added another one. Both are good IMO. (though nginx_http_extra_params is a better name than my_extra_params hehe)

I can add it to the pull request if you like but I was scared that you will break code of people already abusing the nginx_http_params somehow.

It makes more sense to have the operation nginx_http_params: {{ nginx_http_params_defaults + my_extra_params }} done in /vars instead of /defaults if we take the addition approach.

Let me know what you think and I can update my pull request.

jdauphant commented 7 years ago

There is little misunderstand I think. When I was talking about nginx_http_params: {{ nginx_http_params_defaults + my_extra_params }}, it's a possible usage by the user and "my_extra_params" will be an user variable not a variable from this role.

By implementing just:

We normally have retrocompatibility, we limit the complexity of the project and we have possibility to manage extra params in the playbook easely (for people that need advance usage).

sysarcher commented 7 years ago

hmmm.... Okay. I think this is better. So a usage will look like:

vars:
  - my_extra_params:
      - client_max_body_size 200M
# retain defaults and add additional `client_max_body_size` param
- role: jdauphant.nginx
      nginx_http_params: {{ nginx_http_params_defaults + my_extra_params }}

This is a good solution too. Agreed. Should I update my pull request?

jdauphant commented 7 years ago

yes, it's exactly that ! If you can update the PR, it will be perfect :)

Thanks for your help

sysarcher commented 7 years ago

Hello @jdauphant updated the pull request to the best of my understanding.

sysarcher commented 7 years ago

woops... sorry. I'll fix these asap . Did a quick/dirty job that fixed my issue and pull requested it naively.

sysarcher commented 7 years ago

I updated the pull request. Also pulled in your changes from yesterday.

jdauphant commented 7 years ago

@shrmrf Thanks for your help