Assign executable in popen only if it is explicitly set

[ganeshrn]

SUMMARY

Fixes #30836

• As default executable is None assign executable value
  only if it set explicitly.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

module_utils/basic.py

ANSIBLE VERSION

2.4

ADDITIONAL INFORMATION

Playbook:

- name: Test playbook
  vars:
    output: [u'Port      Name               Status       Reason               Err-disabled Vlans\\nFa1/0/1   EDGE_IF_VLAN_341   err-disabled bpduguard\\nFa1/0/33  EDGE_IF_VLAN_341   err-disabled bpduguard']
  tasks:
  - name: Test task
    shell: echo {{ output }} | grep -Po '.*bpduguard' | awk '{ print $1 }'
    register: disabled_ports
  - debug: msg={{ disabled_ports }}

Before:

 {
    "changed": true,
    "cmd": "echo [u'Port      Name               Status       Reason               Err-disabled Vlans\\nFa1/0/1   EDGE_IF_VLAN_341   err-disabled bpduguard\\nFa1/0/33  EDGE_IF_VLAN_341   err-disabled bpduguard'] | grep -Po '.*bpduguard' | awk '{ print $1 }'",
    "delta": "0:00:00.004178",
    "end": "2017-10-05 04:53:20.899535",
    "failed": false,
    "invocation": {
        "module_args": {
            "_raw_params": "echo [u'Port      Name               Status       Reason               Err-disabled Vlans\\nFa1/0/1   EDGE_IF_VLAN_341   err-disabled bpduguard\\nFa1/0/33  EDGE_IF_VLAN_341   err-disabled bpduguard'] | grep -Po '.*bpduguard' | awk '{ print $1 }'",
            "_uses_shell": true,
            "chdir": null,
            "creates": null,
            "executable": null,
            "removes": null,
            "stdin": null,
            "warn": true
        }
    },
    "rc": 0,
    "start": "2017-10-05 04:53:20.895357",
    "stderr": "",
    "stderr_lines": [],
    "stdout": "[uPort",
    "stdout_lines": [
        "[uPort"
    ]
}

After

{
    "changed": true,
    "cmd": "echo [u'Port      Name               Status       Reason               Err-disabled Vlans\\nFa1/0/1   EDGE_IF_VLAN_341   err-disabled bpduguard\\nFa1/0/33  EDGE_IF_VLAN_341   err-disabled bpduguard'] | grep -Po '.*bpduguard' | awk '{ print $1 }'",
    "delta": "0:00:00.005992",
    "end": "2017-10-05 06:19:45.140702",
    "failed": false,
    "invocation": {
        "module_args": {
            "_raw_params": "echo [u'Port      Name               Status       Reason               Err-disabled Vlans\\nFa1/0/1   EDGE_IF_VLAN_341   err-disabled bpduguard\\nFa1/0/33  EDGE_IF_VLAN_341   err-disabled bpduguard'] | grep -Po '.*bpduguard' | awk '{ print $1 }'",
            "_uses_shell": true,
            "chdir": null,
            "creates": null,
            "executable": null,
            "removes": null,
            "stdin": null,
            "warn": true
        }
    },
    "rc": 0,
    "start": "2017-10-05 06:19:45.134710",
    "stderr": "",
    "stderr_lines": [],
    "stdout": "Fa1/0/1\nFa1/0/33",
    "stdout_lines": [
        "Fa1/0/1",
        "Fa1/0/33"
    ]
}

[bcoca]

this is incorrect, the caller is specifically asking to use shell

[bcoca]

not sure why you are removing this, i can see issues with invalid shell/blocker shells set (nologin, /bin/false) but we can work around that.

[bcoca]

so reread what $SHELL means .. seems i made a mistake, i assumed it was 'current shell' but it is the 'default shell', probably removing these 2 lines will fix most issues, the else: shell = True below should stay.

[abadger]

Removing these lines will re-break: #24169

[abadger]

OTOH, we might need to find a new fix for #24169 as bcoca is quoting the POSIX definition of SHELL above and the POSIX definition of popen http://pubs.opengroup.org/onlinepubs/9699919799/ seems to be why this is hardcoded to sh in the first place.

[bcoca]

not really, this works 'by accident', only when default shell matches the executable requested, as my previous comment states, i misinterpreted what the value of $SHELL represents

After rereading, actual fix of the ticket would require we pass ansible_shell_executable information to all modules and fallback to that

[abadger]

Do not move this. You may want to remove setting shell = True in the above conditional block, though.

[abadger]

Another note, when you cherry-pick this you'll probably want to cherry pick 5f22b4f as well (it wasn't deemed important enough to backport but it shouldn't change any behaviour so it should be safe to do so.)

[abadger]

Closing this as we will need something like #31361 to fix both bugs. The ios_command bug has been fixed in 2.4.1 by reverting the original commit.