Check nginx config before reload that may come from other roles

[gnarf]

No description provided.

[jdauphant]

Hello @gnarf ,

Thanks for your pull request.
I can't accept it at this state :
Files managed by this role, should not be modify by external role.
You can use instead this role multiple time just to add the needed configuration (Sometime as dependancy).
If you read this ticket, you will have multiple way to do that :
#66

Best regards,
Julien

[jdauphant]

Also, I forgot but don't hesitate to respond if you have questions or remarks.

Julien

[gnarf]

I was adding my own templated .j2 conf to sites-enabled in a later task and wanted to notify the reload from your base nginx plugin.

[gnarf]

also, because of the way it is now, the config check failing stops you from having any unmanaged config files that MIGHT have an error in them... I would think the notify method of checking config is in general cleaner anyway

[jdauphant]

If you use this role, all nginx config files should be managed by the role, it's how the role is constructed (and intentionally prevent you to have missconfigured state).
You can still use your templates with the role :

  - role: jdauphant.nginx
    nginx_installation_type: "configuration-only"
    # global nginx params
    nginx_http_params: {}
    nginx_configs:
      webserver:
        - "{{ lookup('template', 'templates/webserver_config.j2' )}}"
    nginx_sites:
      main_https:
        - "{{ lookup('template', 'templates/main_config.j2' )}}"

At each call of the role, the configuration is 100% valid.
We can normally do that in a majority of cases.

Best regards,
Julien

PS : If your templates produce only one line or if you have problem with the lookup template, you could need this syntax :

    nginx_configs:
      webserver:
        - |
          {{ lookup('template', 'templates/webserver_config.j2' )}}
    nginx_sites:
      main_https:
        -  |
           {{ lookup('template', 'templates/main_config.j2' )}}

[gnarf]

Okay so my situation is I have a variable something like:

simple_site_configs:
  - { name: test1.com, root: /var/www/test1.com }
  - { name: test2.com, root: /var/www/test2.com }
  - { name: test3.com, root: /var/www/test3.com }
  - { name: test4.com, root: /var/www/test4.com }

And I want to setup dynamically the nginx_sites (as well as some other things) using with_items and template

Can you suggest how I'd convert from my array of sites to nginx_sites ?

[gnarf]

Also - I'm trying to do this from within another role, which makes defining each site manually twice not as nice of an option

[jdauphant]

I think I have a solution in mind, I will write here when I got the time.
If not, I will made the technical review of the PR before accept it.

[jdauphant]

Hello @gnarf ,
I don't have solution with the templating to do that :(

So review of the PR :

• It could be better for code clarity to add "check nginx configuration" in all needed place in code, it will be better for code clarity (and this will remove the need of "really restart nginx" and "really reload nginx").

- name: Copy the nginx configuration file
  template: src=nginx.conf.j2 dest={{nginx_conf_dir}}/nginx.conf
  notify:
   - check nginx configuration
   - restart nginx
  tags: [configuration,nginx]

... 

- name: Create the configurations for sites
  template: src=site.conf.j2 dest={{nginx_conf_dir}}/sites-available/{{ item }}.conf
  with_items: nginx_sites.keys()
  notify:
   - check nginx configuration
   - restart nginx
  tags: [configuration,nginx]

...

• We can clean tags in "check nginx configuration", normally they are not necessary.

Thanks for your help and this discution,
Best regards,
Julien

[gnarf]

Internally updating every use case to also notify "check nginx configuration" works, but I was hoping to avoid forcing consumers of the module to remember to also notifiy: ["check nginx configuration", "reload nginx"] if they were taking this sort of approach. It seems pretty standard practice in the community to name the notify "reload service" or "restart service", and if you are like me, I don't want to have to remember if the service also requires a check service config notify.

If anyone is already using your module (like I am) and only notifies reload nginx we will also give them the check nginx config handler without requiring them to know to notify both.

I could rename those second stage "really" to "reload nginx - after config check" or something else if you like.

---

In further thought - there is a bit of an issue in that every time this runs, it will leave the config files on the server in a potentially bad state (though ansible will exit with a failure, and you will know) it doesn't actually restore a working configuration when the checks fail.

I have thoughts about how to actually accomplish this "restore working config" if you're interested in a followup PR.

[jdauphant]

Ok, "reload nginx - after config check" will be better in effect.
After that and the tags cleanup, it's ok to merge.

I will have prefered without "debug" task but apparently we can't do without that.

---

Could be interesting. What approach do you have in mind to do that ? (Backup files before apply rules ?)

---

Thanks again for your help @gnarf ,
Best regards

[jdauphant]

Hello @gnarf if you need help for PR or lack of time, don't hesitate to tell

[gnarf]

Thanks for the ping - I had a really busy week(end) hosting an event in NYC, I'll try to remember tomorrow but if you're in a hurry feel free to add the changes you wanted on top here.

[jdauphant]

Thanks @gnarf