Normalize apt tasks

[tangrufus]

Because we always want the latest version.

[fullyint]

Thanks for this! I agree that for new servers, we essentially always want the latest version.

However, always updating existing servers' packages to the latest version could be an unwanted surprise, particularly in the rare case that the updates break something (like this thread about php?).

Ok, package updates rarely break things, but if playbooks occasionally update packages, users could develop general unease around the unpredictability of what may happen when they run Trellis playbooks. For instance, a user could modify one php param in group_vars and think: "I want to apply this php change by running server.yml but I'm scared of what else could change and break."

How about making apt module tasks use variables for the name and state parameters? (untested)

• state would default to present but if users want the always-update behavior, they may define nginx_package_state: latest
• nginx_package_name: nginx=1.13.3-0 would lock down the version but users could redefine it to a different version, or some variant like nginx/stable, or just nginx if using state: latest. Trellis would have to constantly update these default version numbers 🙁 but it would create a more predictable experience for users than an always-update default of state: latest.

- name: Install Nginx
  apt:
    name: "{{ nginx_package_name }}"
    state: "{{ nginx_package_state | default('present') }}"
    update_cache: true
    cache_valid_time: "{{ apt_cache_valid_time }}"
  notify: reload nginx

It would get a little more complicated for apt tasks that use with_items. For example, we could convert the apt_packages list into a dict:

# package-name=version: state
apt_packages_default:
  python-software-properties=0.96.20.7: present
  python-pycurl=7.43.0-1: present
  build-essential=12.1: present
  python-mysqldb=1.3.7-1: present
  curl=7.47.0-1: present
  git-core=1:2.7.4-0: present
  dbus=1.10.6-1: present
  libnss-myhostname=229-4: present

apt_packages_custom: {}
apt_packages: "{{ apt_packages_default | combine(apt_packages_custom) }}"

Then the related "Checking essentials" install task would use with_dict:

- name: Checking essentials
  apt:
    name: "{{ item.key }}"
    state: "{{ item.value }}"
    update_cache: true
    cache_valid_time: "{{ apt_cache_valid_time }}"
  with_dict: "{{ apt_packages }}"

Edit: Changing the various apt_packages variables from lists to dicts would be a breaking change for people who have defined custom values for such variables. Locking down versions is probably worth it nonetheless. We could consider detecting the old list format and print a helpful message.

• in vars.py or
• using type_debug in a fail task (requires bumping Ansible version_requirement from 2.2 to 2.3)

[tangrufus]

How about making apt module tasks use variables for the name and state parameters?

Agree

nginx_package_name: nginx=1.13.3-0 would lock down the version... Trellis would have to constantly update these default version numbers 🙁 but it would create a more predictable experience for users...

Disagree. Main reason: We haven't taught developers to update with roots/Trellis (this repo).
Leaving them hanging with outdated packages is dangerous.

using type_debug in a fail task

Agree: Bumping Ansible requirement to v2.3 and fail if old old list format detected.

[tangrufus]

Normalize apt tasks so they look like this:

- name: Install XXX
  apt:
    name: XXX
    state: "{{ XXX_package_state | default(apt_package_state) }}"
    update_cache: yes
    cache_valid_time: "{{ apt_cache_valid_time }}"

- name: Install XXX
  apt:
    name: "{{ item.key }}"
    state: "{{ item.value }}"
    update_cache: yes
    cache_valid_time: "{{ apt_cache_valid_time }}"
  with_dict: "{{ XXX_packages }}"

# varaiables
XXX_packages_default:
  YYY: "{{ apt_package_state }}"
  ZZZ: "{{ apt_package_state }}"

XXX_packages_custom: {}
XXX_packages: "{{ XXX_packages_default | combine(XXX_packages_custom) }}"

Check whether apt_packages and php_extensions are dicts:
Example error message:

`php_extensions` is a StrictUndefined not a dict. See:
https://github.com/roots/trellis/pull/881

`apt_packages` is a list not a dict. See:
https://github.com/roots/trellis/pull/881

It doesn't change package versions, except:

• bumps Ansible requirement to v2.3
• always update_cache: yes and respects apt_cache_valid_time
• requires apt_packages and php_extensions to be dicts

Update: Also checks apt_packages_custom and php_extensions_custom

Question: When does a apt tasks need force: yes?

[partounian]

Commenting on the PHP issue, is there a reason we use 7.1 instead of official 7?

[swalkinshaw]

Question: When does a apt tasks need force: yes?

best I could find on it: https://forums.solydxk.com/viewtopic.php?t=6148. Still not exactly sure when we have to use it though.

[swalkinshaw]

Looks good overall. My only question is if we should keep the memcached and SSH related packages as a list? Maybe we should just split them out into individual tasks. It's only two packages each. Not sure we need to have them with the treatment of default + custom.

[partounian]

Imo replace memcached with redis and/or APCu.

[tangrufus]

default + custom for memcached and SSH related packages

There is a actual use case for this:
When I first using Trellis, I didn't add caching right from the beginning. Trellis installed php-memcached for me by default. Later on, I wanted to use wp-memcached which requires php-memcached(without ending d).

To do so, I have to run $sudo apt-get uninstall php-memcached by hand first.

With this patch, we can:

memcached_packages_custom:
  php-memcached: absent
  php-memcache: latest

force: yes on apt tasks

I remove all force: yes from apt tasks. From @swalkinshaw's link, force: yes seems useful only when there are conflicts.

Replace memcached with redis and/or APCu

For now, you can remove - { role: memcached, tags: [memcached] } from server.yml and dev.yml. Then add your own redis role.

Just curious, which object cache backend do you use with redis?

[swalkinshaw]

Interesting benefit with this is we get granular statuses per package.

Before:

TASK [common : Checking essentials] ********************************************
changed: [default] => (item=[u'python-software-properties', u'python-pycurl', u'build-essential', u'python-mysqldb', u'curl', u'git-core', u'dbus', u'libnss-myhostname'])

After:

TASK [common : Checking essentials] ********************************************
changed: [default] => (item=python-software-properties)
changed: [default] => (item=build-essential)
changed: [default] => (item=python-mysqldb)
changed: [default] => (item=libnss-myhostname)
ok: [default] => (item=dbus)
changed: [default] => (item=git-core)
ok: [default] => (item=python-pycurl)
ok: [default] => (item=curl)

[fullyint]

@tangrufus Thanks again for all your work on this!

This PR adds to the php role a couple tasks verifying the dict format for php_extensions_custom and php_extensions. I wonder if it would be preferred to move these validations into the common role because it runs at the beginning of the playbook. Some users have expressed frustration with validation tasks that occur and fail late in the playbook. Strictly conceptually, the tasks belong in the php role but maybe they're better for users in the common role.

I tried to produce a single task to verify the dict format of all the vars at once, but it wasn't as simple as I hoped.

I think I see why you used a second separate task to validate vars like apt_packages that use the combine filter. If one of the component vars combined isn't a dict, Ansible fails with |combine expects dictionaries, got u'text_string'. Thus Ansible's error message hijacks your ability to print out the help message in time. So, the first step checks the format of component vars, then the second checks the combined vars.

Just for brainstorming sake...

# roles/common/tasks/main.yml

- name: Verify dict format for apt package component variables
  fail:
    msg: "{{ package_vars_wrong_format_msg }}"
  when: package_vars_wrong_format | count
  vars:
    package_vars:
      apt_packages_default: "{{ apt_packages_default }}"
      apt_packages_custom: "{{ apt_packages_custom }}"
      memcached_packages_default: "{{ memcached_packages_default }}"
      memcached_packages_custom: "{{ memcached_packages_custom }}"
      php_extensions_default: "{{ php_extensions_default }}"
      php_extensions_custom: "{{ php_extensions_custom }}"
      sshd_packages_default: "{{ sshd_packages_default }}"
      sshd_packages_custom: "{{ sshd_packages_custom }}"
    package_vars_wrong_format: "[{% for k,v in package_vars.iteritems() if v | type_debug != 'dict' %}'{{ k }}',{% endfor %}]"

- name: Verify dict format for apt package combined variables
  fail:
    msg: "{{ package_vars_wrong_format_msg }}"
  when: package_vars_wrong_format | count
  vars:
    package_vars:
      apt_packages: "{{ apt_packages }}"
      memcached_packages: "{{ memcached_packages }}"
      php_extensions: "{{ php_extensions }}"
      sshd_packages: "{{ sshd_packages }}"
    package_vars_wrong_format: "[{% for k,v in package_vars.iteritems() if v | type_debug != 'dict' %}'{{ k }}',{% endfor %}]"

# roles/common/defaults/main.yml

package_vars_wrong_format_msg: |
  The following variables must be formatted as dicts:
    {{ package_vars_wrong_format | to_nice_yaml | indent(2) }}

  See: https://github.com/roots/trellis/pull/881

The example above checks a few more vars. Your validations checked the vars that were essential to check. My extra vars shouldn't need to be checked, but I've learned to never assume what users will and won't do. Still, maybe it's overkill to check all the vars I listed above.

[tangrufus]

why you used a second separate task to validate vars like apt_packages that use the combine filter ... Ansible's error message hijacks your ability to print out the help message in time.

Correct!

Move checking to common role: Good idea!

Extra vars checks: Also good idea!

Question: How to ensure common role is ran when users provision with Ansible tags?

[tangrufus]

Solution found: Run the checks twice.

About that ugly fileglob with_items: ansible/ansible#23265

When required variable is undefined, error message is also ugly. Tried adding mandatory filter, more less the same:

TASK [common : Verify dict format for apt package component variables] *********
System info:
  Ansible 2.4.0.0; Vagrant 2.0.0; Darwin
  Trellis at "Ansible 2.4 compatibility"
---------------------------------------------------
The conditional check 'package_vars_wrong_format | count' failed. The error
was: error while evaluating conditional (package_vars_wrong_format | count):
[{% for k,v in package_vars.iteritems() if v | type_debug != 'dict' %}'{{ k
}}',{% endfor %}]: {u'apt_packages_custom': u'{{ apt_packages_custom }}',
u'apt_packages_default': u'{{ apt_packages_default }}'}:
'apt_packages_custom' is undefined

The error appears to have been in
'/Users/tangrufus/Desktop/trellis/roles/common/tasks/validate.yml': line 2,
column 3, but may
be elsewhere in the file depending on the exact syntax problem.

The offending line appears to be:

---
- name: Verify dict format for apt package component variables
  ^ here

[fullyint]

Question: How to ensure common role is ran when users provision with Ansible tags?

If we want the two validation tasks I proposed in the common role to also run when a user specifies --tags other-role, we can just assign to those two tasks the tags corresponding to the relevant roles, like how the "Validate format of site_hosts" task uses tags: [letsencrypt, wordpress]. The two example tasks I posted would have the extra parameter
tags: [sshd, memcached, php]

The simplicity of two new tags parameters seems preferable to the new commit "Include dict format checks in common role" which adds 6 new tasks and 4 new include files.

---

When required variable is undefined, error message is also ugly.

Given that all these vars are defined in role defaults, it seems very unlikely any of these vars would ever be undefined. Although users sometimes fail to update group_vars when updating Trellis, I don't remember any reports of people failing to update a role's defaults/main.yml.

but I've learned to never assume what users will and won't do

I'm skeptical that we must be ready for undefined in this case, but here's an adjustment to the example tasks I posted (untested).

# roles/common/tasks/main.yml

  - name: Verify dict format for apt package component variables
    fail:
-     msg: "{{ package_vars_wrong_format_msg }}"
+     msg: |
+       The following variables must be formatted as dicts:
+         {{ package_vars_wrong_format | to_nice_yaml | indent(2) }}
+ 
+       See: https://github.com/roots/trellis/pull/881
    when: package_vars_wrong_format | count
    vars:
      package_vars:
-       apt_packages_default: "{{ apt_packages_default }}"
+       apt_packages_default: "{{ apt_packages_default | default({}) }}"
-       apt_packages_custom: "{{ apt_packages_custom }}"
+       apt_packages_custom: "{{ apt_packages_custom | default({}) }}"
        ... (etc.)
      package_vars_wrong_format: "[{% for k,v in package_vars.iteritems() if v | type_debug != 'dict' %}'{{ k }}',{% endfor %}]"

  - name: Verify dict format for apt package combined variables
    fail:
-     msg: "{{ package_vars_wrong_format_msg }}"
+     msg: |
+       The following variables must be defined and formatted as dicts:
+         {{ package_vars_wrong_format | to_nice_yaml | indent(2) }}
+ 
+       See: https://github.com/roots/trellis/pull/881
    when: package_vars_wrong_format | count
    vars:
    package_vars:
-       apt_packages: "{{ apt_packages }}"
+       apt_packages: "{{ apt_packages | default({}) }}"
-       memcached_packages: "{{ memcached_packages }}"
+       memcached_packages: "{{ memcached_packages | default({}) }}"
        ... (etc.)
-     package_vars_wrong_format: "[{% for k,v in package_vars.iteritems() if v | type_debug != 'dict' %}'{{ k }}',{% endfor %}]"
+     package_vars_wrong_format: "[{% for k,v in package_vars.iteritems() if v | type_debug != 'dict' or vars[k] is not defined %}'{{ k }}',{% endfor %}]"

[partounian]

@tangrufus I actually don't use Redis. I use wp-lcache.

[tangrufus]

• rebased
• moved checks into common
• tagged checks with [sshd, memcached, php] as @fullyint suggested

[swalkinshaw]

I'm good with this 👍

@fullyint ?

[fullyint]

This looks good to me, with a couple minor nits. 👍 from me if you've tested that this current iteration runs without error.

I think this PR should also add a CHANGELOG.md entry prefixed by [BREAKING] <msg>, given that this changes the format of some vars from list to dict.

[fullyint]

I think this include_tasks is no longer needed, right? (there's no more validate.yml)

[tangrufus]

Removed

[fullyint]

I'd prefer that this package_vars_wrong_format_msg var were defined in roles/common/defaults/main.yml, where users won't have to think about it unless they are really digging in. I think of this var as more of a helper/utility than a variable users are likely to want to modify. This change would keep group_vars/all/main.yml more streamlined.

An alternative that may be more consistent with other validation tasks could be to put the message in a template lookup (example).

[tangrufus]

Moved into roles/common/templates/package_vars_wrong_format_msg.j2

[tangrufus]

CHANGELOG.md entry added

[fullyint]

this one should be lookup('template'... too?

[tangrufus]

done

[fullyint]

@tangrufus Let me say again, THANK YOU for all your patient work on this! 💚

As I scrutinize it a bit more, I have a couple more comments.

update_cache parameter

This PR normalizes the apt module tasks to always use both the parameters update_cache and cache_valid_time. Regarding cache_valid_time I just noticed this docs note:

As of Ansible 2.4, this implicitly sets update_cache

As I tested with -vvv, if the cache is not older than cache_valid_time, the task outputs "cache_updated": false even when update_cache: yes. This seems good. If the cache was older than cache_valid_time, the task outputs "cache_updated": true. Also good. In any case, the update_cache seems superfluous.

Given the recent Trellis requirement of Ansible 2.4, I think we can drop all instances of update_cache: yes for the apt module tasks (but don't remove it for the two apt_repository module tasks). What do you think?

package name indicates package version

The dict format for vars such as apt_packages_default makes it possible for users to redefine the dicts in a manner that specifies package versions, however uncommon the scenario may be:

apt_packages_default:
  # package_name: state
  # package_name_and_version: state
  python-software-properties=0.96.20.7: present
  python-pycurl=7.43.0-1: present
  ...

The above formatting takes advantage of how the apt module uses the name parameter to set the package version:

NAME: A package name, like foo, or package specifier with version, like foo=1.0

However, some the apt tasks "hard code" the name parameter. Contrast the nginx-related name: "{{ nginx_package }}" with the fail2ban-related name: fail2ban. In the case that a user wants a specific version of nginx, the user may redefine the nginx_package variable. However, the user would not be able to define the package name/version for the following:

• ferm
• mariadb-client
• mariadb-server
• fail2ban
• ssmtp
• php-xdebug

I honestly don't know the answer to the following question: Would it be desirable to make vars such as ferm_package for each of these additional packages so that users could potentially specify versions, or would that be over-developing things?

[tangrufus]

remove update_cache parameter

Would it be desirable to make vars such as ferm_package for each of these additional packages so that users could potentially specify versions

All set.
Extracted package names into variables so that users can specify versions.

[fullyint]

Awesome, @tangrufus!

Looks good to me! 👍