Simplify and improve package name squashing

[willthames]

ISSUE TYPE

• Bugfix Pull Request

ANSIBLE VERSION

ansible 2.2.0 (devel 7af47a3886) last updated 2016/04/29 11:28:56 (GMT +1000)
  lib/ansible/modules/core: (detached HEAD e78ee3b128) last updated 2016/04/29 11:28:58 (GMT +1000)
  lib/ansible/modules/extras: (detached HEAD ca310f3d15) last updated 2016/04/29 11:28:58 (GMT +1000)
  config file = 
  configured module search path = Default w/o overrides

SUMMARY

• Allow variables that contain a dict lookup

• Squash names built from dictionary values e.g.

  package: name="{{item.name}}-{{item.version}}"
  with_items:
  - name: hello
    version: 1.2.3
  - name: goodbye
    version: 2.3.4
  

  now works the same as

  package: name="{{item}}"
  with_items:
  - hello-1.2.3
  - goodbye-2.3.4
  

• Fix some erroneous test assumptions e.g.

  package: name="{{pkg_mgr}}"
  with_items:
  - hello-1.2.3
  - goodbye-2.3.4
  

  should just install a single package with the name pkg_mgr (in effect item is not a bound variable)

Fixes #15649

[bcoca]

I wanted to go one step further and make it configurable as shown in #12086

ignore how we went with loop_control

[willthames]

@bcoca, the tests broke on loop_control with my first push, but it's now working with loop_control: { 'loop_var': 'another_name_for_item' } too.

It would be good to get the fix for #15649 backported to 2.0 and 2.1. Looks like it can be trivially cherry-picked onto stable-2.1 and reasonably easily cherry-picked onto stable-2.0.

[willthames]

Also, this is mislabelled feature_pull_request.

[kustodian]

Could we please merge this into devel, and not wait for 2.2, since this is a bugfix pull request, not a feature request. Currently in devel modules which use squashing of with loops + dict lookup are broken as it was reported in #15649 (which was closed even though it wasn't fixed).

I tested this pull request on the latest devel and it worked like a charm.

[abadger]

Okay, I cudgeled my brain and finally recalled the reason that we template the name field twice in the old code. Things like this are caught by the old code but not by the proposed code:

---
- hosts: localhost
  vars:
    list_of_pkgs:
      - python-q
      - python-epdb
  tasks:
  # Uninstall package and then make sure it is the latest to work around
  # improper deps inside old package
  - dnf:
      name: "{{ list_of_pkgs }}"
      state: "{{ items }}"
    with_items:
      - absent
      - latest

Templating the name field twice catches the fact that list_of_pkgs is templated to the same value in both instances of the loop. Simply comparing the templated name field to the original name field does not catch this case.

[abadger]

I don't think that's a deal breaker, though. I don't think the problem being solved was in the double templating so we should be able to drop that portion back into the new code without issues (with a better comment as to why we're doing things that way).

[abadger]

How does this new code handle things that aren't strings? For instance,

dnf:
  name: "{{ item }}"
  state: present
with_items:
  - [ "python", "python-q"]
  - [ "python3", "python3-q" ]

It looks like name will end up being

name: [ ["python", "python-q"], ["python3", "python3-q"] ]

[willthames]

The old code only coped with strings - is that last use case actually a regression?

[abadger]

if the new code puts the items together like that, it is a regression... The old code would have skipped squashing in that case.

[willthames]

@abadger squashing now squashes lists of lists. I've "fixed" a bunch of tests where the test was broken for the results returned (usually because the squash happened where it wasn't before, and the conditional evaluation removed items)

[abadger]

Okay, update -- will and I talked on IRC last night. I didn't think some of the test cases were right so I worked on those for a while and merged a new set today. The cleanups here don't pass the new tests so also decided to push a fix jsut for #15649. Those changes are in and cherry-picked to stable-2.1 and stable-2.0 branches.

Aside from the tests, I think we still need to template the name field twice in order to catch whether it contains a loop_var or not. Checking against the original string wouldn't catch things like a non-loop-var being templated into name.

when in doubt about the validity of squashing we should err on the side of safety and not squash. Squashing is really a dead-end feature now that every module that squashes has to take a list type (as users can just pass a list directly instead of having to use with_items to create their list).

[ansibot]

@willthames This PR was tested by travis-ci.org, which is no longer used. Please rebase your branch to trigger running of current tests.

click here for bot help