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

Fix ansible deprecations #187

Merged
merged 5 commits into from
May 22, 2019

Conversation

thedanbob
Copy link
Contributor

Ansible 2.8.0 added several deprecation warnings which affect this playbook, this fixes them.

@spantaleev
Copy link
Owner

spantaleev commented May 22, 2019

Some of this breaks compatibility for versions earlier than Ansible 2.8.

For example, adding source: pull to docker_image module calls generates the following error on Ansible 2.7:

Unsupported parameters for (docker_image) module: source Supported parameters include: api_version, archive_path, buildargs, cacert_path, cert_path, container_limits, debug, docker_host, dockerfile, force, http_timeout, key_path, load_path, name, nocache, path, pull, push, repository, rm, ssl_version, state, tag, timeout, tls, tls_hostname, tls_verify, use_tls

The source argument was added in Ansible 2.8. Using it on anything older than 2.8 causes an error.

As stated in our Ansible documentation page, we currently target Ansible 2.5 as our minimum supported version. We also provide an alternative (running via Docker) for people who can't get that. Even our alternative method doesn't support Ansible 2.8, since Alpine Linux doesn't have it yet (not even on alpine:edge).

I guess this forces us to either:

  • adopt this all and drop support for Ansible < 2.8. This sounds bad as it's just been released and not generally available yet. We also don't have a "run it via Docker" alternative.
  • force people on 2.8+ to live with the deprecation until we're willing to adopt 2.8 as our minimum supported Ansible version. This sucks, but doesn't break anything.
  • have 2 docker_image module invocations, one for each version of Ansible (<2.8 and >=2.8). This is ugly, but it means no breakage and no deprecations.

It's incredibly silly how Ansible makes it hard to migrate.. New arguments would be introduced and it would cry like a baby if you don't use them.. But then if you add extra arguments, old versions which don't recognize them would completely break.

It looks like something like this might work:

- name: Ensure postgres Docker image is pulled (Ansible < 2.8)
  docker_image:
    name: "{{ matrix_postgres_docker_image_to_use }}"
  when: "matrix_postgres_enabled and (ansible_version.major <= 2 and ansible_version.minor < 8)"

- name: Ensure postgres Docker image is pulled (Ansible >= 2.8)
  docker_image:
    name: "{{ matrix_postgres_docker_image_to_use }}"
    source: pull
  when: "matrix_postgres_enabled and (ansible_version.major > 2 or ansible_version.minor > 7)"

That's a little ugly though.. We have 10+ invocations of the docker_image module that we need to carefully duplicate.


I don't see anything about not being able to use dashes in group names (matrix-servers -> matrix_servers) on the Ansible 2.8 porting guide.

Thankfully, your commit message pointed me to TRANSFORM_INVALID_GROUP_CHARS](https://docs.ansible.com/ansible/latest/reference_appendices/config.html#transform-invalid-group-chars). It seems like it is a thing.


I noticed that you're casting a lot of the when statements using |bool.

Reading about CONDITIONAL_BARE_VARS (defaulting to true on Ansible 2.8) tells me that:

  • it's not easy to understand what it says
  • it's easy to shoot yourself in the foot (explicit casting is required sometimes, but not others)
  • re-reading the documentation just yields more confusion

I suppose with Ansible 2.8+, anyone sane will always use |bool everywhere, as it's very easy to cause unintentional breakage.

I guess we should keep what you've done so far as a good initial thing which pleases the finicky Ansible 2.8, and we should proceed to add |bool to everything else for our own sanity and future safety.


Wow, I'm appalled at how awful Ansible 2.8 is.. More magical than ever, having bad porting guide and (as always) giving the middle finger to easily making playbooks backward compatible.

Hopefully, they know what they're doing and this is all for the better.

Unfortunately, we are using Ansible (for lack of better alternative), so we have to follow it wherever it goes.
Thank you for leading the way with this pull request! 👍
And thank you for nicely separating your commits, making it easier to understand (especially given Ansible's incomplete 2.8 porting guide).

I guess it's just the source: pull thing that trips our backward-compatibility and makes this unmergeable. We should either revert that one or use conditional docker_image invocations?

@thedanbob
Copy link
Contributor Author

thedanbob commented May 22, 2019

Wow, I had no idea ansible's backwards compatibility was so bad. I found an alternative solution for the source issue that's slightly less ugly:

source: "{{ 'pull' if ansible_version.major > 2 or ansible_version.minor > 7 else omit }}"

I tested it this time on both 2.8.0 and 2.7.10.

@spantaleev spantaleev merged commit 45c67db into spantaleev:master May 22, 2019
@spantaleev
Copy link
Owner

Oh, that omit thing is a nice workaround!
I guess Ansible is not quite that bad after all ;)

Merged! 👍
Thank you very much!


I've added a changelog note about the necessary manually changes to inventory/hosts in 7a08c9b

@thedanbob thedanbob deleted the fix-ansible-deprecations branch July 25, 2019 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants