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

Support omit within lists #58587

Closed
agowa opened this issue Jul 1, 2019 · 10 comments
Closed

Support omit within lists #58587

agowa opened this issue Jul 1, 2019 · 10 comments
Labels
affects_2.9 This issue/PR affects Ansible v2.9 feature This issue/PR relates to a feature request. support:core This issue/PR relates to code supported by the Ansible Engineering Team.

Comments

@agowa
Copy link
Contributor

agowa commented Jul 1, 2019

SUMMARY

omit should also be supported within lists to allow jinja2 expressions to add elements to lists.

ISSUE TYPE
  • Feature Idea
COMPONENT NAME

omit
core

ADDITIONAL INFORMATION

As an administrator I want to have conditional elements in a task, so that I can simplify my playbook and roles and have less module invocations and a faster run.

- win_feature:
    name:
      - "{% if not fs_installed_smb_v1|default(false) %}FS-SMB1{% else %}{{ omit }}{% endif %}"
      - "foo"
      - "bar"
    state: absent
- win_feature:
    name:
      - "{% if not fs_installed_smb_v1|default(false) %}FS-SMB1{% endif %}"
      - foo
      - bar
    state: absent
- win_feature:
    name: >-
      {{ '[' + (fs_installed_smb_v1) | ternary('"FS-SMB1",','') + '"foo", "bar"' ']' }}
    state: absent

The first example currently produces the following python list: [ '__omit_place_holder__375d4b174292829ef0a2d0878acbe8842696711d' ], therefore module execution will fail, as the omit place holder string gets passed onto the module.
The second example instead produces [''], and module execution will fail, because of the empty string element.
And the third one works as expected, but is not very intuitive and lists with more parameters tend to get messy really quickly, like:

- name: GitLab | bootstrap container
  docker_container:
    name: '{{ gitlab_container_name }}'
    state: present
    pull: yes
    purge_networks: yes
    image: '{{ gitlab_base_image }}'
    restart_policy: '{{ gitlab_restart_policy }}'
    # TODO: Fully fix, if the first element has ", " at the beginning, it fails.
    # Therefore if no ssh port is exposed it will fail.
    ports: >-
      {{ '[' + (gitlab_expose_ssh) | ternary(' "' + gitlab_ssh_host_ip + ':' ~
      gitlab_ssh_host_port +':22"','') + (gitlab_expose_http) | ternary(', "' +
      gitlab_http_host_ip + ':' ~ gitlab_http_host_port +':80"','') +
      (gitlab_expose_https) | ternary(', "' + gitlab_https_host_ip + ':' ~
      gitlab_https_host_port +':443"', '') + (gitlab_expose_prometheus) |
      ternary(', "' + gitlab_prometheus_host_ip + ':' ~
      gitlab_prometheus_host_port +':9090"', '') + ']' }}
    networks: '{{ gitlab_docker_networks }}'
    volumes:
      - '{{ gitlab_project_directory }}/etc:/etc/gitlab:rw'
      - '{{ gitlab_project_directory }}/opt:/var/opt/gitlab:rw'
      - '{{ gitlab_project_directory }}/log:/var/log/gitlab:rw'

Also they are prown for race conditions like in this example if gitlab_expose_ssh is false but any of the following gitlab_expose_http, gitlab_expose_https, gitlab_expose_prometheus is true, it will fail, as all of these elements have a prepended semicolon. Sure, that one could also be fixed with e.g. (gitlab_expose_ssh and gitlab_expose_http) | ternary(',',''), but even that would not cover all cases and more or less considerable as write only code (you can write it, but shortly after you're unable to read it again).

@ansibot
Copy link
Contributor

ansibot commented Jul 1, 2019

Files identified in the description:

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. labels Jul 1, 2019
@sivel
Copy link
Member

sivel commented Jul 1, 2019

We are conflicted about adding this feature, and it has come up in the past.

Should you want to pursue this further, please add this to the core IRC meeting agenda for additional discussion: https://github.com/ansible/community/issues?utf8=✓&q=is%3Aissue+is%3Aopen+label%3Ameeting_agenda+label%3Acore

Example PR from a previous attempt: #39179

@sivel
Copy link
Member

sivel commented Jul 1, 2019

!component =lib/ansible/executor/task_executor.py

@ansibot
Copy link
Contributor

ansibot commented Jul 1, 2019

Files identified in the description:

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibot ansibot added support:core This issue/PR relates to code supported by the Ansible Engineering Team. and removed support:community This issue/PR relates to code supported by the Ansible community. labels Jul 1, 2019
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Jul 2, 2019
@bcoca
Copy link
Member

bcoca commented Jul 2, 2019

You can already do this:

- win_feature:
    name: '{{ namestuff|reject('equalto', omit)|list }}'
  state: absent
  vars:
    namestuff:
      - "{% if not fs_installed_smb_v1|default(false) %}FS-SMB1{% else %}{{ omit }}{% endif %}"
      - "foo"
      - "bar"

@felixfontein
Copy link
Contributor

From IRC discussion; I think this should work:

- win_feature:
    name: "{{ name_list | filter("equal_to", omit) | list }}"
    state: absent
  vars:
    name_list:
      - "{% if not fs_installed_smb_v1|default(false) %}FS-SMB1{% else %}{{ omit }}{% endif %}"
      - "foo"
      - "bar"

@bcoca
Copy link
Member

bcoca commented Jul 2, 2019

another way, construct it w/o omited

- set_fact:
     namestuff: ' {{ namestuff|default([])|union([item]) }}'
  when: item != omit
  loop:
      - "{% if not fs_installed_smb_v1|default(false) %}FS-SMB1{% else %}{{ omit }}{% endif %}"
      - "foo"
      - "bar"

- win_feature: name={{namestuff}}

@bcoca
Copy link
Member

bcoca commented Jul 2, 2019

closing as per meeting minutes, several ways to achieve this already exist and only feature missing would be recursive data structure filtering, which would be a reason for a filter, but not putting this in core.

@felixfontein
Copy link
Contributor

Would be nice if some of the tricks mentioned here could end up somewhere in the docs. I mean in particular the |reject('equal_to', omit)|list one. That could improve some people's playbooks/roles significantly.

@bcoca
Copy link
Member

bcoca commented Jul 2, 2019

#46979

@bcoca bcoca closed this as completed Jul 2, 2019
@ansible ansible locked and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.9 This issue/PR affects Ansible v2.9 feature This issue/PR relates to a feature request. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

No branches or pull requests

5 participants