Skip to content
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

Copy files/includes and populate templates/includes and add to includes.d. #242

Merged

Conversation

chriszarate
Copy link
Contributor

Picking up from #241, here's something that implements @louim's option 1, with both files and templates as @swalkinshaw suggested.

Be warned: This isn't pretty, and it relies on the regex_replace filter. Pretty sure this is safe for Ansible >= 1.8, though.

Is there a better way to do this? I couldn't figure out how to nest a with_fileglob inside a with_dict (or a with_items/wordpress_sites.keys()), so someone could break the task if one of their folder names doesn't match one of their site keys.

@louim
Copy link
Contributor

louim commented Jun 15, 2015

You are right, there is no way to currently do that. For the files, you could go with the synchronize module, but that wouldn't work for the templates. You cloud try with with_pipe. Something like (untested):

- template: src="{{ item }}" dest="/etc/nginx/includes.d/{{ item }}"
  with_pipe: find ../templates/includes/ -type f

If that works, I would find that more easy to understand than the regex, but that may be just a personal preference. 😉

@swalkinshaw
Copy link
Member

@chriszarate I'll check if I can think of a better method of doing this without the regex_replace.

@chriszarate
Copy link
Contributor Author

Thanks. I checked a few angles, and the problem is that anything that globs is going to pass the full local path. :(

@swalkinshaw
Copy link
Member

@chriszarate I got nothing. basename is useful but no way to get the parent directory alone. Think I'm okay with the regex_replace solution. The regex itself isn't too bad.

@swalkinshaw
Copy link
Member

@chriszarate I get failures on this because the subdirectories of /etc/nginx/includes aren't created first:

Destination directory /etc/nginx/includes.d/example.dev does not exist

@chriszarate
Copy link
Contributor Author

Ok, let me take another pass at this. I also want to improve the regex to leave less room for error,

@fullyint
Copy link
Contributor

Here's an approach inspired by @louim's with_pipe. It passed some minimal testing.

- name: Copy templates/includes to includes.d
  template: src="{{ item }}" dest="/etc/nginx/{{ item[:-3] }}"
  with_lines: "cd ../templates && find includes.d -type f 2>/dev/null || :"

Notes:

  • Still a bit messy.
  • One task only. I figured the template module could also handle regular files, so long as all filenames end in .j2. See any problems with this?
  • If there are no templates, the template task just skips.

Potential problems:

  • Fails on incorrectly named path: wordpress-setup/templates/includes.d/NOT-A-SITE/myconf.conf.j2.
  • Removing a template from local wordpress-setup/templates/includes.d does not remove from remote server. Rather than remove a local template file, users may have to update the file's contents to be empty so that that template module would pick up on the changes.

@fullyint
Copy link
Contributor

@chriszarate if you're interested, I'd love your feedback on this revised approach. By the way, thanks for mentioning wordpress_sites.keys(). I didn't even know that was possible.

Features

  • no need to list templates in configs, just add template to local templates/includes.d/example.com/
  • no problem if local machine is missing templates/includes.d directory or subdirectories
  • no problem if template names have spaces
  • skips files not matching *.conf.j2 (e.g., doesn't process your .DS_Store as template)
  • if you remove a template from local, the corresponding conf file will be removed from remote
  • only adds or removes templates corresponding to sites active in wordpress_sites. So, if for some reason your local machine had templates/includes.d/extra-site.com/myconf.conf.j2 but extra-site.com was not active in your wordpress_sites, this template wouldn't be processed. You don't need to remove templates or directories for inactive sites.
  • reloads Nginx if a conf file is added or removed
  • fully idempotent
# roles/wordpress-setup/tasks/nginx.yml

- name: Template files out to includes.d
  template: src="includes.d/{{ item }}" dest="/etc/nginx/includes.d/{{ item[:-3] }}"
  with_lines: "cd ../templates/includes.d && find {{ template_paths }} -type f 2>/dev/null || :"
  register: includes
  notify: reload nginx

- name: Retrieve list of existing files in includes.d
  shell: "find {{ include_paths }} -type f 2>/dev/null || :"
  args:
    chdir: /etc/nginx/includes.d
  register: includes_existing
  changed_when: false

- name: Remove undesired files from includes.d
  file: path="/etc/nginx/includes.d/{{ item }}" state=absent
  with_items: includes_existing.stdout_lines | difference(includes_to_keep)
  notify: reload nginx

Several supporting variables:

# roles/wordpress-setup/defaults/main.yml

template_paths: "{% for site in wordpress_sites.keys() %}{{ site }}/*.conf.j2 {% endfor %}"
include_paths: "{{ template_paths | regex_replace('(.j2)', '') }}"
includes_to_keep: '{% if includes.results is defined -%}
                     [{% for item in includes.results %}
                       "{{ item.item[:-3] }}",
                     {% endfor %}]
                   {%- else -%}
                     []
                   {%- endif %}'

The last two tasks could have been combined in one. I chose against it because it might look more complicated and not print to the output exactly which files it removed. Roughly, the task would have been

- name: Remove undesired files from includes.d
  shell: for file in */*; do case "$file" in {{ includes_to_keep }}) ;; *) rm "$file" && echo "$file was removed";; esac; done
  args:
    chdir: /etc/nginx/includes.d
  register: includes_removed
  changed_when: includes_removed.stdout != ''

It would need a variable like this in the defaults file:

includes_to_keep: "{% for item in includes.results %}'{{ item.item[:-3] }}' | {% endfor %}''"

@chriszarate
Copy link
Contributor Author

I'll take a look at this today!

@chriszarate
Copy link
Contributor Author

This is great! Thanks right back at you for mentioning with_lines—I hadn't seen that before. It looks like an alias for lookup('lines' ...)? Makes inline templating easier.

Works great. I was inspired to simplify the supporting variables, and I was eventually able to eliminate them altogether. I also removed the regexes and restricted the templates to a folder depth of 1 (e.g., templates/includes.d/foo/bar/include.conf.j2 will be ignored, and /etc/nginx/includes.d/foo/bar/include.conf will not be deleted).

I'll push that here in a minute, but feel free to open your own pull request if you like your approach better. I was just tinkering.

@chriszarate chriszarate force-pushed the nginx-provide-includes-framework branch from e9a7dac to 819e675 Compare July 13, 2015 00:44
@fullyint
Copy link
Contributor

@chriszarate Smart! Lots of superior solutions in here, especially eliminating the supporting variables. I finally understand the map() filter after seeing your demonstration. There were a couple issues I noticed.

"Remove unmanaged files" with_items

Results undefined. Suppose you remove all templates in order to remove all conf files. With no templates, the template module skips and nginx_includes_managed.results is undefined. This causes the with_items logic to fail in the "Remove unmanaged files" task. The task skips and existing files are not removed. To resolve this, you could set a default empty list for nginx_includes_managed.results:

- with_items: nginx_includes_existing.stdout_lines | difference(nginx_includes_managed.results | map(attribute='path') | list)
+ with_items: nginx_includes_existing.stdout_lines | difference(nginx_includes_managed.results | default([]) | map(attribute='path') | list)

Missing path attribute. The path attribute is only present for items in nginx_includes_managed.results for which "changed": false. So, if the current run of the playbook has files that were templated, those "changed": true items will lack the path attribute and the with_items logic will fail for the "Remove unmanaged files" task. Unmanaged files are not removed.

Although the "changed": true items lack path, they do have dest, which has the path value we desire. It's possible to have an instance of nginx_includes_managed.results where some items have only path and others have only dest (mix of changed true/false). Maybe we could dynamically pick path or dest, but it might be easier to use the item attribute that is present for all items. This item attribute lists the relative path to template items, so we could use regex_replace to make it a full path and strip the .j2.

- with_items:     nginx_includes_existing.stdout_lines | difference(nginx_includes_managed.results | default([]) | map(attribute='path') | list)
+ with_items: "{{ nginx_includes_existing.stdout_lines | difference(nginx_includes_managed.results | default([]) | map(attribute='item') | map('regex_replace', '(.*)\\.j2', '/etc/nginx/includes.d/\\\\1') | list) }}"

I haven't yet figured out why it only works enclosed in "{{ ... }}". I tried Ansible v1.9.0.1 and v1.9.2.

"Supporting variable" option. I was happy that you'd freed us from "supporting variables," but we could abstract the long convoluted look by creating a supporting variable nginx_includes_managed_paths:

with_items: nginx_includes_existing.stdout_lines | difference(nginx_includes_managed_paths)

or just put everything in nginx_includes_unmanaged:

with_items: nginx_includes_unmanaged

Other Notes

when: item.startswith('/etc/nginx/includes.d/'). I didn't understand the need for this conditional. Could it be removed after adding the default() and regex_replace() changes described above?

Folder depth. I'm not opposed to the folder depth of 1 limitation, but I don't understand why it would be a concern. In fact, my first inclination would have been a recursive approach. I haven't used Nginx includes, so I don't know what comes up.

Thanks again for your work on this!

Put files in /etc/nginx/includes.d, mirroring directory structure
recursively.
@chriszarate chriszarate force-pushed the nginx-provide-includes-framework branch from 819e675 to 309a5d8 Compare August 8, 2015 05:45
@chriszarate
Copy link
Contributor Author

Sorry for the long delay in circling back to this. I wasn't able to do testing until recently. Thanks for following up and catching those problems.

I removed the maxdepth option at your suggestion. Nginx will not recursively include files, but we should leave the option to the user to potentially enumerate includes in subdirectories.

I added when: item.startswith('/etc/nginx/includes.d/') as a failsafe because Ansible's file module with state: absent will delete recursively. I wanted to make extra sure we weren't deleting anything we weren't supposed to. After extensive testing, though, I feel comfortable removing it.

Thanks again! I'd welcome more feedback but it feels ready to merge from my perspective.

@fullyint
Copy link
Contributor

fullyint commented Aug 9, 2015

Nice! I agree: Looks ready to merge. I just ran it again against the tests I could think of.

  • No problem if roles/wordpress-setup/templates has no includes.d dir
  • No problem if roles/wordpress-setup/templates/includes.d exists but is empty
  • Templates added on control machine are successfully added as conf files on server
  • Templates removed from control machine are successfully removed from server
  • Skips template files that don't match *.conf.j2
  • No problem if template filenames have spaces
  • Reloads Nginx if conf file added, or if conf file removed
  • Ignores templates/includes.d/not-in-wordpress-sites-list
  • No problem if all templates are removed -- all conf file includes will be removed from server

@chriszarate Thanks for this!
Could I interest you in adding a simple wiki for this? Might be nice for people to know of this feature. A few points that came to mind for me were just versions of testing points above:

  • a phrase or two about why nginx conf includes
  • place template files at roles/wordpress-setup/templates/includes.d/site-key/filename.conf.j2 (could clarify that site-key corresponds to wordpress-sites)
  • template filenames must match *.conf.j2 (ok to have non-template files in there, named otherwise, they'll just be ignored)
  • etc.

fullyint added a commit that referenced this pull request Aug 9, 2015
…work

Copy files/includes and populate templates/includes and add to includes.d.
@fullyint fullyint merged commit 57e9d39 into roots:master Aug 9, 2015
@chriszarate chriszarate deleted the nginx-provide-includes-framework branch August 10, 2015 00:25
@chriszarate
Copy link
Contributor Author

Yes! Thanks!

Took a stab at a Wiki page at Nginx-includes.

@fullyint
Copy link
Contributor

@chriszarate That wiki reads so well, with great info. Thank you!
And thanks again for the great work and time spent on this PR.

@austinpray
Copy link
Contributor

@chriszarate 🍉 🍉 🍉

I second @fullyint on that wiki being well written. Great job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants