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

chriszarate commented Jun 15, 2015

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

swalkinshaw commented Jun 16, 2015

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

@chriszarate

This comment has been minimized.

Copy link
Contributor Author

chriszarate commented Jun 17, 2015

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

@swalkinshaw

This comment has been minimized.

Copy link
Member

swalkinshaw commented Jun 18, 2015

@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

This comment has been minimized.

Copy link
Member

swalkinshaw commented Jun 21, 2015

@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

This comment has been minimized.

Copy link
Contributor Author

chriszarate commented Jun 21, 2015

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

@fullyint

This comment has been minimized.

Copy link
Member

fullyint commented Jul 10, 2015

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

This comment has been minimized.

Copy link
Member

fullyint commented Jul 12, 2015

@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

This comment has been minimized.

Copy link
Contributor Author

chriszarate commented Jul 12, 2015

I'll take a look at this today!

@chriszarate

This comment has been minimized.

Copy link
Contributor Author

chriszarate commented Jul 13, 2015

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 chriszarate:nginx-provide-includes-framework branch Jul 13, 2015
@fullyint

This comment has been minimized.

Copy link
Member

fullyint commented Jul 13, 2015

@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 chriszarate:nginx-provide-includes-framework branch to 309a5d8 Aug 8, 2015
@chriszarate

This comment has been minimized.

Copy link
Contributor Author

chriszarate commented Aug 8, 2015

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

This comment has been minimized.

Copy link
Member

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
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chriszarate chriszarate deleted the chriszarate:nginx-provide-includes-framework branch Aug 10, 2015
@chriszarate

This comment has been minimized.

Copy link
Contributor Author

chriszarate commented Aug 10, 2015

Yes! Thanks!

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

@fullyint

This comment has been minimized.

Copy link
Member

fullyint commented Aug 10, 2015

@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

This comment has been minimized.

Copy link
Member

austinpray commented Aug 10, 2015

@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
5 participants
You can’t perform that action at this time.